-
Notifications
You must be signed in to change notification settings - Fork 0
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]: 커뮤니티 게시글 생성 API 코드 리팩토링 #560
Conversation
PostSaveRequest로 받는 데이터 validation 추가 Related to: #559
repository/community -> community/repository 폴더로 이동 Related to: #559
community domain 패키지로 이동 validation 필요한 필드 추가 Related to: #559
Related to: #559
import문 변경있음 Related to: #559
Service 계층에 밀집된 비즈니스 로직 분리 하위 도메인 정의 및 코드 분리 Related to: #559
Related to: #559
Related to: #559
CommunityController.java에 있는 val 삭제 Related to: #559
Related to: #559
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
작업량이 꽤 많았을텐데 테스트코드까지 꼼꼼하게 작업해주셔서 감사합니다!
@@ -97,10 +98,12 @@ public ResponseEntity<Map<String, Boolean>> upPostHit( | |||
@PostMapping("/posts") | |||
public ResponseEntity<PostSaveResponse> createPost( | |||
@Parameter(hidden = true) @AuthenticationPrincipal InternalMemberDetails memberDetails, | |||
@RequestBody PostSaveRequest request | |||
@RequestBody @Valid PostSaveRequest request |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
요거 @Valid가 앞쪽에 와야 제대로 동작할 것 같습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
제가 알기로는 @Valid 위치는 상관없이 동작하는 것으로 알고 있습니다.
해당 어노테이션 테스트도 해본 상태입니다!
import javax.validation.constraints.NotBlank; | ||
import javax.validation.constraints.NotNull; | ||
|
||
public record PostSaveRequest( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spring Validator 쓸 때 메시지도 같이 작성해주면 좋을 것 같아요!!
fyi. 다른 도메인의 Request Dto
Lines 16 to 17 in 2d3bd79
@NotNull(message = "수신자 정보는 필수 입력 값입니다.") | |
Long receiverId, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
좋은 의견 감사합니다! 바로 반영하겠습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
현재 prod 테이블에도 not null 컬럼들 반영이 필요할 것 같아요! (혹시 null인 데이터가 있다면 백필이 필요할 것 같구요)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 부분은 해당 PR에 달린 코드리뷰 반영이 다 되고 같이 진행하면 좋을 것 같습니다!
|
||
@Entity | ||
@Getter | ||
@Builder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Builder는 클래스 전역에 선언했을 때, id값의 변경도 허용되기 때문에 클래스 내부에서 최소한의 변경되는 범위에만 사용하는 게 좋을 것 같습니다!
AnonymousNickname.builder().id(id).build(); >> 가 가능해짐
그래서 되도록이면 @AllArgsConstructor 도 private으로 제한하거나 아예 사용하지 않고, 필요한 필드들만 생성자 정의를 해주는 게 좋아보입니다! (e.g. MemberBlock 처럼 통일하면 좋을 것 같습니닷 ㅎㅎ)
@Builder | ||
@NoArgsConstructor(access = AccessLevel.PROTECTED) | ||
@AllArgsConstructor(access = AccessLevel.PROTECTED) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기도 위와 동일합니다!! (현재 이런 구조가 많은데 유지보수를 위해 하나로 통일하는 게 좋을 것 같아서요!)
private final AnonymousPostProfileRepository anonymousPostProfileRepository; | ||
private final AnonymousNicknameRepository anonymousNicknameRepository; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기도 마찬가지로 retriever나 modifier로 사용하면 좋을 것 같습니다~!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
현재는 커뮤니티 생성 API만 코드 리팩토링을 했기 때문에, 그외의 부분은 추후 리팩토링시에 변경하는게 안전하다고 판단했습니다!
} | ||
Member member = memberRetriever.findMemberById(writerId); | ||
CommunityPost post = communityPostModifier.createCommunityPost(member, request); | ||
anonymousPostProfileService.createAnonymousPostProfile(request.isBlindWriter(), member, post); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기서 기존의 if문은 서비스 로직에 살려두는 게 좋다고 생각하는데 어떻게 생각하실까요?? 새로운 게시글 등록 시, 항상 익명 프로필 생성이 필요하기 보다 유저의 익명 여부 선택에 따라 달라진다는 의미가 코드에서 읽히면 더 좋을 것 같아요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
동의합니다!
List<AnonymousPostProfile> lastFourAnonymousPostProfiles = anonymousPostProfileRetriever.getTopByOrderByCreatedAt(4); | ||
List<AnonymousPostProfile> lastFiftyAnonymousPostProfiles = anonymousPostProfileRetriever.getTopByOrderByCreatedAt(50); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
매직넘버 상수로 선언하는 게 좋을 것 같아요 ~!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
동의합니다!
private final AnonymousProfileImageRetriever anonymousProfileImageRetriever; | ||
private final AnonymousNicknameRetriever anonymousNicknameRetriever; | ||
|
||
public void createAnonymousPostProfile(Boolean isBlindWriter, Member member, CommunityPost post) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
어차피 얘를 다른 서비스에서 호출하고 있어 트랜잭션이 적용된 상태일테지만.. 내부에도 @transactional을 선언하는 게 좋을 것 같습니다.
나중에 이 메서드를 직접 호출했을 때나 단위테스트를 짜는 상황에서 트랜잭션이 적용되지 않아 발생하는 문제들을 예방하는 차원에서요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
헉 제가 @transactional을 빼먹었군요 꼼꼼하게 봐주셔서 감사합니다.
// AnonymousPostProfile의 최근 4개 및 50개의 프로필 Mock 데이터 생성 | ||
List<AnonymousPostProfile> lastFourProfiles = List.of( | ||
AnonymousPostProfile.builder() | ||
.profileImg(AnonymousProfileImage.builder().id(1L).imageUrl("Image1").build()) | ||
.nickname(AnonymousNickname.builder().id(1L).nickname("Nickname1").build()) | ||
.build(), | ||
AnonymousPostProfile.builder() | ||
.profileImg(AnonymousProfileImage.builder().id(2L).imageUrl("Image2").build()) | ||
.nickname(AnonymousNickname.builder().id(2L).nickname("Nickname2").build()) | ||
.build() | ||
); | ||
|
||
List<AnonymousPostProfile> lastFiftyProfiles = List.of( | ||
AnonymousPostProfile.builder() | ||
.profileImg(AnonymousProfileImage.builder().id(1L).imageUrl("Image1").build()) | ||
.nickname(AnonymousNickname.builder().id(1L).nickname("Nickname1").build()) | ||
.build(), | ||
AnonymousPostProfile.builder() | ||
.profileImg(AnonymousProfileImage.builder().id(2L).imageUrl("Image2").build()) | ||
.nickname(AnonymousNickname.builder().id(2L).nickname("Nickname2").build()) | ||
.build(), | ||
AnonymousPostProfile.builder() | ||
.profileImg(AnonymousProfileImage.builder().id(3L).imageUrl("Image3").build()) | ||
.nickname(AnonymousNickname.builder().id(3L).nickname("Nickname3").build()) | ||
.build() | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(사소) 테스트 더미데이터 생성도 private 함수(e.g. createAnonymousProfileImageDummy()
)로 따로 빼거나 클래스 레벨에서 미리 생성해두면 테스트 메서드 내에서 전달하고자 하는 바가 더 명확하고 깔끔하게 드러날 것 같습니당!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
테스트코드 또한 앞으로 유지보수를 해야하는 부분이라고 생각해서 의견주신 방법대로 가독성을 높이는게 정말 필요하다고 생각합니다!
감사합니당!!
🐬 요약
커뮤니티 게시글 생성 API 코드 리팩토링을 진행했습니다.
하위 도메인인 익명 게시글 프로필 코드를 따로 분리하고 Retriever, Modifier를 이용하여 중복 코드를 방지했습니다.
👻 유형
PR의 유형에 맞게 체크해주세요!
🍀 작업 내용
PR에 담긴 작업 내용을 작성해주세요!
🌟 관련 이슈
PR과 관련된 이슈 번호를 작성해주세요!
close: #559