Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: 연애고사 매칭 결과를 수정한다 #49

Merged
merged 19 commits into from
Aug 13, 2024
Merged

Conversation

devholic22
Copy link
Collaborator

@devholic22 devholic22 commented Aug 12, 2024

📄 Summary

  • 연애고사 매칭 결과를 수정하여 더 자세한 정보 (기존의 id에서 확장하여 닉네임, 주소, 나이 포함 조회)가 조회되도록 했습니다.
  • JDBC 기반의 조회에서 Querydsl로 변경하였습니다.
  • 기존 연애고사 매칭은 동성이 나올 수 있었기에 이성만 조회되도록 변경하였습니다.
  • 소울메이트 (연애고사 매칭 결과 30개 이상 같은 사람)를 어떻게 보여줄 것인지에 대한 정책이 향후 변경 가능성이 크다고 판단이 되어, application에 SoulmatePolicy 인터페이스를 두고 infrastructure에 프론트의 현재 요구사항인 랜덤 인덱스로 한 명만 조회하는 정책 구현체를 두었습니다.

🙋🏻 More

  • 연애고사 조회 매핑 결과는 48시간 동안 클라이언트의 로컬 스토리지에 보관됩니다.
  • 처음에는 조회 매핑 결과의 정합성을 완전히 지키기 위해 클라이언트가 API를 호출할 때 48시간 유지가 아니라 백엔드에서 스케줄링을 통해 각각의 매칭 결과를 캐시로 저장하는 방식을 고민했는데, 일단 MVP 단계에서는 단순히 API를 호출하면 로컬 스토리지에 보관하고 정합성은 고려하지 않는 방향으로 선택하였습니다.

더 개선할 점이 있다면 반영하겠습니다!

close #39

- 회원 연애고사 구현체 방식 변경
- 회원 연애고사 구현체 방식 변경
- 동성 조회되지 않도록 수정
- 더 많은 정보를 제공하도록 수정
- SoulmatePolicy에 의해 필터링되도록 수정
- 관련된 테스트 코드 수정
- pull된 fixture 메서드 이용으로 변경
- 매칭 조회 인수 테스트 수정
- 필요한 Fixture 정의
- soulmatepolicy 패키지 수정
- MemberSurveysQueryRepository 테스트 작성
- 관련 Fixture 등록
- 소울메이트 조회 페이크 로직 구현 방법 변경
- 관련된 fixture 수정
- 외부 페이크 리포지터리를 의존하는 방식으로 수정
- SoulmatePolicy 변수 인라인화 적용
- MemberSurveys 빌더 사용 제거 및 생성자 이용하도록 수정 (빌더 어노테이션은 유지)
- MemberSurvey는 빌더 사용하도록 수정
Copy link
Owner

@eom-tae-in eom-tae-in left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

고생하셨습니다. 몇가지만 수정해주세요~

Comment on lines 124 to 127
.where(
memberSurveys1.memberId.eq(memberId)
.and(memberSurvey.surveyId.eq(memberId))
)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

조건절에 .and()를 쓰지 않고 ,(콤마)로 구분하는건 어떨까요?
그리고 .and(memberSurvey.surveyId.eq(memberId)) 이 부분의 로직이 .and(memberSurvey.surveyId.eq(surveyId))여야 맞는 것 같습니다!

Comment on lines 102 to 110
List<SurveySoulmateResponse> soulmates = memberSurveysQueryService.findSoulmates(member.getId());

// then
assertThat(matchMembers.contains(otherMemberId)).isTrue();
SoftAssertions.assertSoftly(softly -> {
softly.assertThat(soulmates.size()).isEqualTo(1);
softly.assertThat(soulmates.get(0).id()).isEqualTo(visibleFemaleMember.getId());
softly.assertThat(soulmates.get(0).id()).isNotEqualTo(notVisibleMaleMember.getId());
softly.assertThat(soulmates.get(0).id()).isNotEqualTo(notVisibleFemaleMember.getId());
});
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이렇게 테스트를 진행할 수도 있지만 SurveySoulmateResponseFixture를 이용해보는건 어떨까요?
SurveySoulmateResponseFixture.설문_조사_소울메이트_응답_생성_회원(final Member member);
이렇게 메서드 만들고 설문_조사_소울메이트_응답_생성_회원(visibleFemaleMember); 이렇게 쓰면 usingRecursiveComparison()로 손쉽게 테스트 가능해집니다!

추가적으로 notVisibleMaleMember가 어떤 회원을 의미하는 건가요?

Copy link
Collaborator Author

@devholic22 devholic22 Aug 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

말씀하신 대로 SurveySoulmateResponseFixture로 회원을 받아 소울메이트 조회 결과로 만들면 visibleFemaleMember에 대해서는 usingRecursiveComparison으로 테스트 할 수 있지만, 본 목적은 재귀적으로 모든 속성을 비교하기보다는 조회되어야 할 회원 (visibleFemaleMember)과 조회되지 않아야 할 회원 (notVisibleMaleMember, notVisibleFemaleMember)을 모두 놓았을 때 조회되어야 할 회원이 보이고 조회되지 않아야 할 회원이 보이지 않는지 비교하는 데 중점을 두었습니다.
만약 회원을 받아 소울메이트 조회 결과를 fixture로 이용한다면 제공받은 회원 세 명에 대한 픽스처를 전부 생성하게 되는데, 이러한 측면에서 회원을 식별할 수 있는 id만 비교하는 게 충분하고 테스트 측면에서 더 빠르지 않을까라는 생각이 들었었는데 태인님은 어떻게 생각하시나요? (notVisibleMaleMember는 테스트 할 회원의 성별이 남성이기에 연애고사 결과가 30개 이상 같지만 조회되지 않아야 한다는 걸 명시하기 위해 만들었습니다!)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixture로 테스트를 하는 방식은 모든 회원에 대해 SurveySoulmateResponse를 만드는 것이 아니라 예상되는 값 하나만 만들고 테스트를 진행하는 방식을 말씀드린거였습니다!
현준님 말씀대로 회원만 판단하는 거라면 id값으로만 비교하는 것도 괜찮을 수도 있겠네요! 현준님 방식으로 진행해도 좋을 것 같네요

@@ -3,6 +3,7 @@
import com.atwoz.survey.domain.membersurvey.MemberSurveys;

import static com.atwoz.survey.fixture.MemberSurveyFixture.회원_연애고사_응시_필수_과목_30개;
import static com.atwoz.survey.fixture.MemberSurveyFixture.회원_연애고사_응시_필수_과목_30개_과목_문제_답안;
import static com.atwoz.survey.fixture.MemberSurveyFixture.회원_연애고사_응시_필수_과목_두개;

public class MemberSurveysFixture {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

해당 클래스에 @SuppressWarnings("NonAsciiCharacters") 어노테이션 붙여주시면 좋을거 같아요!

@@ -21,4 +22,11 @@ public class MemberSurveysFixture {
.memberSurveys(회원_연애고사_응시_필수_과목_30개())
.build();
}

public static MemberSurveys 회원_연애고사_필수_과목_질문_30개_응시_저장_id_과목_답안(final Long memberId, final Long surveyId, final Integer answer) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이건 가독성 측면 이야기인데 파라미터가 3개 이상인 경우

public static MemberSurveys 회원_연애고사_필수_과목_질문_30개_응시_저장_id_과목_답안(
final Long memberId,
final Long surveyId,
final Integer answer
) {

이렇게 작성하는건 어떤가요? 저희끼리 정해보면 좋을거 같아요

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

좋습니다 파라미터가 3개 이상일 때에는 말씀하신 것 처럼 각각 개행 추가해서 진행하는 걸로 하면 좋을 것 같습니다!

Comment on lines 104 to 110
return new SurveySoulmateResponse(
member.getId(),
member.getNickname(),
member.getMemberProfile().getProfile().getLocation().getCity(),
member.getMemberProfile().getProfile().getLocation().getSector(),
member.getMemberProfile().getProfile().getPhysicalProfile().getAge()
);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 부분을 SurveySoulmateResponseFixture로 제공하는건 어떤가요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixture 방식보다는 (별도의 클래스가 생성되기에) 정적 팩터리 메서드로 이용하면 좋을 것 같은데 어떻게 생각하시는지 궁금합니다! (ex: SurveySoulmateResponse.from(member) 방식은 어떨까요?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

앗 그런데 생각해보니 이렇게 하면 테스트를 위해서 프로덕션 코드를 수정하는 행위가 되겠네요. 말씀하신 대로 Fixture 방식으로 하는 게 더 나은 것 같습니다!

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

그럴경우 프로덕션 코드의 추가가 발생하지 않나요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

네 맞습니다 Fixture 방식으로 하겠습니다!


private Gender extractMemberGender(final Member member) {
MemberProfile memberProfile = member.getMemberProfile();
return memberProfile.getProfile().getPhysicalProfile().getGender();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

체이닝은 한 줄에 하나만 있도록 개행 추가해주세요~

- MemberSurveysFixture에 SuppressWarnings 추가
- MemberSurveysFakeRepository 체이닝 개행 추가
- where 조건식 수정
- where 표현 콤마로 수정
- 가독성을 위해 파라미터 개행 추가
- SoftAssertions static import 적용
- 회원을 기반으로 소울메이트 결과를 추출한 fixture 등록
@devholic22 devholic22 merged commit 8740582 into develop Aug 13, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

연애고사 매칭 결과를 수정한다
2 participants