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: member 바운디드 컨텍스트에서 job, hobby, profile, selfintro를 분리한다 #78

Merged
merged 4 commits into from
Oct 12, 2024

Conversation

eom-tae-in
Copy link
Owner

@eom-tae-in eom-tae-in commented Oct 8, 2024

📄 Summary

  • member에 속해 있던 job, hobby, profile, selfintro를 모두 분리하였습니다.
  • job은 api를 통해 상태가 관리되도록 변경했습니다.

🙋🏻 More

수정 범위가 너무 넓어서 제가 작업한 부분 이외에 실패하는 테스트는 주석 처리를 했습니다. 전반적인 수정사항을 참고하셔서 수정하시면 될 것 같습니다!

close #77

Copy link
Collaborator

@devholic22 devholic22 left a comment

Choose a reason for hiding this comment

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

너무 수고하셨습니다!

파일이 많다 보니 코멘트도 살짝 많은 것 같습니다 🙇

추가로 쿼리에 대한 부분이 꽤 쿼리 최적화가 많이 필요하겠다는 생각이 들었습니다. 특히 ProfileQueryRepository 부분이 그런 것 같아요

지금 생각으로는 빠른 개발을 먼저 하고 이후에 리팩터링을 하는 게 더 좋을 것 같습니다

이번 리뷰는 말씀해주신 것 처럼 프로덕션 코드에 대해서만 드렸습니다


import jakarta.validation.constraints.NotBlank;

public record StyleCreateRequest (
public record JobCreateRequest(
@NotBlank(message = "스타일 이름을 입력해주세요.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

레코드가 Style에서 Job으로 바뀌었으나 필드 설명은 계속 스타일로 되어 있는 것 같습니다


import jakarta.validation.constraints.NotBlank;

public record StyleUpdateRequest(
public record JobUpdateRequest(
@NotBlank(message = "스타일 이름을 입력해주세요.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Style -> Job에 대한 설명으로 수정해야 할 것 같습니다!


@ExceptionHandler(JobNameAlreadyExistedException.class)
public ResponseEntity<String> handleJobNameAlreadyExistedException(final JobNameAlreadyExistedException e) {
return getErrorMessageWithStatus(e, HttpStatus.CONFLICT);
Copy link
Collaborator

Choose a reason for hiding this comment

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

중복 에러 차원에서 conflict를 선택한 게 더 좋아보입니다! 그런데 혹시 이렇게 존재 유무에 대한 예외를 다른 곳들에 많이 작성했었을텐데 그 부분들에서는 bad request로 했던 것으로 기억합니다. 이 부분은 향후 bad request들을 모두 지금처럼 conflict로 바꾸는 작업이 필요할 것 같습니다 :)

Copy link
Owner Author

Choose a reason for hiding this comment

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

네 이 부분은 예외처리 리팩터링 진행하실 때 추가해주시면 감사하겠습니다 :)

public class JobNameAlreadyExistedException extends RuntimeException {

public JobNameAlreadyExistedException() {
super("이미 존재하는 스타일 이름입니다");
Copy link
Collaborator

Choose a reason for hiding this comment

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

직업에 관한 것으로 하는 게 더 맞아보입니다

public class JobCodeAlreadyExistedException extends RuntimeException {

public JobCodeAlreadyExistedException() {
super("이미 존재하는 스타일 코드입니다.");
Copy link
Collaborator

Choose a reason for hiding this comment

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

직업으로 변경 부탁드립니다 :)

@AllArgsConstructor(access = AccessLevel.PRIVATE)
@NoArgsConstructor(access = AccessLevel.PUBLIC)
@Entity
public class UserHobbies {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Member에서 User로 수정하신 이유가 있나요?

Copy link
Owner Author

Choose a reason for hiding this comment

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

이 부분은 회원 바운디드 컨텍스트의 루트 애그리거트(Member) 이름과 겹치는 것이 혼란을 줄 수도 있겠다는 생각에 Member가 아닌 User를 사용하였습니다. 다른 바운디드 컨텍스트에서도 이런 경우에 대해 다른 이름으로 지정하는 것이 어떨까요?

Copy link
Collaborator

Choose a reason for hiding this comment

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

이게 말씀하신 바운디드 컨텍스트에 해당되는 것이군요! 좋은 생각인 것 같습니다.

@Builder
@EqualsAndHashCode(of = "id")
@AllArgsConstructor(access = AccessLevel.PRIVATE)
@NoArgsConstructor(access = AccessLevel.PUBLIC)
Copy link
Collaborator

Choose a reason for hiding this comment

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

NoArgs가 이전에 쓰던 protected에서 public으로 되었는데 의도하신 것인지 궁금합니다.

Copy link
Owner Author

Choose a reason for hiding this comment

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

이 부분은 제가 인지를 못했던 거 같네요... 수정하겠습니다!

// .build();
// }

// TODO: 회원 가입 및 로그인 진행 시 해당 메서드를 추가하여 최근 방문일을 갱싱해줘야 한다.
Copy link
Collaborator

Choose a reason for hiding this comment

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

주석 오타있습니다 :)

return profileRecommendationResponses;
}

// TODO: 우선 가장 인기 있는 회원 한 명만 조회되도록 설정함. 추후에 이야기 해서 상위 5%를 어떻게 설정할지 논의해봐야함.(쿼리 최적화도)
Copy link
Collaborator

Choose a reason for hiding this comment

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

ProfileQueryRepository가 진짜 조회가 어마어마 하네요.. 꼭 논의해봐야 할 듯 합니다!

Copy link
Owner Author

Choose a reason for hiding this comment

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

네 회의 일정 맞춰서 한번 논의해보면 좋을 거 같아요!

.where(profile.memberId.eq(memberId))
.fetchFirst();
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

불필요한 공백 있습니다

Copy link
Collaborator

@devholic22 devholic22 left a comment

Choose a reason for hiding this comment

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

수고하셨습니다!

@eom-tae-in eom-tae-in merged commit fa18650 into develop Oct 12, 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.

member 바운디드 컨텍스트에서 job, hobby, profile, selfintro를 분리한다
2 participants