-
Notifications
You must be signed in to change notification settings - Fork 0
[DP-561] 픽픽픽 메인, 픽픽픽 나고픽 API V2 #193
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
Conversation
ssosee
left a comment
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.
고생하셨습니다. ^0^
| .popularScore(pick.getPopularScore()) | ||
| .pickOptions(mapToPickOptionsResponse(pick, member)) | ||
| .isVoted(PickResponseUtils.isVotedMember(pick, member)) | ||
| .isNew(PickResponseUtils.isNewPick(pick)) |
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.
PickResponseUtils.isNewPick(pick) 를 생성자인
this.isNew = PickResponseUtils.isNewPick(pick); 에 작성하면 중복을 줄일 것 같아요!
대신 생성자에서 pick 을 인자로 받아야겠죠?
소영님은 아래와 같은 방식을 어떻게 생각하시나요?
public PickMainResponseV2(Pick pick, ....) {
this.id = id;
....
this.isNew = PickResponseUtils.isNewPick(pick);
....
}// 회원 전용
public static PickMainResponseV2 of(Pick pick, Member member) {
return PickMainResponseV2.builder()
....
.isNew(pick)
....
}// 익명 회원 전용
public static PickMainResponseV2 of(Pick pick, AnonymousMember anonymousMember) {
return PickMainResponseV2.builder()
....
.isNew(pick)
....
}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.
오! 이렇게 하면 중복을 줄일 수 있어서 좋을 것 같아요.
다만 생성자에 로직이 포함되는 것이 dto 보다는 도메인스럽다는 느낌이 들기도 하는 것 같은데 세웅님은 어떻게 생각하시나용?!
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.
클래스의 필드에 어떠한 메소드로 인해서 가공이 들어가는 순간 순수한 dto의 성격은 없어지는 것 같아요!
저의 경우에는 클래스의 생성자가 순수하게 필드 값 자체로만 유지되어야 하는 경우가 필요하면 생성자를 더 만들 것 같아요!
소영님 취향에 따라서 작성하면 좋을 것 같아요! 👍
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.
세웅님 요거 작업하다가 다시 원복시켰는데요! 이유는 다음과 같슴니다
return PickMainResponseV2.builder()
.id(pick.getId()) // pick
.title(pick.getTitle()) // pick
.voteTotalCount(pick.getVoteTotalCount()) // pick
.isVoted(PickResponseUtils.isVotedMember(pick, member)) // pick + member 필요
.pick(pick) // isNew 계산을 위해 또 pick을 넘김
.build();isNew 말고도 pick을 사용하는 필드들이 많은 상황에서, pick을 생성자로 넘겨서 isNew만 계산하자니 isVoted 같은 경우엔 케이스에 따라 호출하는 PickResponseUtils의 메서드가 상이해서(member, anonymousMember), 결국 이 필드는 빌더로 주입해줘야 하는 값이 되더라구요!
그러면 생성자 안에서 pick으로부터 계산되는 값(isNew)과 빌더로 직접 주입되는 값 (isVoted)
이 섞여있게 되는데, 이게 오히려 추상화 레벨이 일관되지 않는(?) 느낌이 들어서 원복했습니다!
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.
넵 고생하셨습니다!! 🙌
고민해주셔서 감사합니다!~~~~!!!
| import com.dreamypatisiel.devdevdev.domain.entity.PickOption; | ||
| import com.dreamypatisiel.devdevdev.global.utils.TimeUtils; | ||
|
|
||
| public class PickResponseUtils { |
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.
이번 기회에 클래스를 abstract 로 선언하는 것도 좋을 것 같아요.!
|
|
||
| @Test | ||
| @DisplayName("회원이 픽픽픽 메인을 조회한다.") | ||
| void getPicksMainByMember() throws Exception { |
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.
이번 기회에 service 를 mockBean 으로 사용하는 것도 좋을 것 같아요!
| */ | ||
| public static boolean isWithinOneWeek(LocalDateTime createdAt) { | ||
| LocalDateTime oneWeekAgo = LocalDateTime.now().minusWeeks(1); | ||
| return createdAt.isAfter(oneWeekAgo); |
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.
isAfter 만 적용하셨는데, 정책이 equals 도 포함해야하는지 확인해보면 좋을 것 같아요! 😊
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.
임하님이 날짜 기준으로 일주일이고, 7일전 날짜도 포함이라고 하셔서 그렇게 변경하겠습니다!
| /** | ||
| * 시간 관련 유틸리티 클래스 | ||
| */ | ||
| public class TimeUtils { |
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.
static 메소드만 존재할 것 같은데요!
그렇다면 abstract 로 선언하면 좋을 것 같아요!
📝 작업 내용
픽픽픽 메인 API(GET: /devdevdev/api/v2/picks)
나도 고민 했는데 픽픽픽 API(GET: /devdevdev/api/v2/picks/{pickId}/similarties)