-
Notifications
You must be signed in to change notification settings - Fork 3
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
[BE] feat: 모임 상세 조회 API 구현 및 검색 필터링 조건 추가 #269
Conversation
Co-authored-by: J-I-H-O <jeongjiho0731@gmail.com> Co-authored-by: jimi567 <repday0609@gmail.com>
Co-authored-by: J-I-H-O <jeongjiho0731@gmail.com> Co-authored-by: jimi567 <repday0609@gmail.com>
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 ApiResponse.ofSuccess(clubQueryService.findById(memberId, id)); | ||
} | ||
|
||
// TODO: @ModelAttribute |
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.
@ModelAttribute
이건 어디에 쓸 예정인가요??
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 객체로 한 번에 받고 있습니다.
이 경우 @ModelAttribute
를 생략할 수 있지만, 해당 요청에 필요한 값을 path parameter로 받는지, query parameter로 받는지, 혹은 body로 받는지 혼동이 있을 수 있다고 생각했어요. 실제로 저희도 개발 중에 헷갈리기도 했구요 ㅎㅎ;
쿼리 파라미터로 요청 받음을 명확하게 표현하기 위해 해당 어노테이션을 붙이는 것을 고려하고 있어요! 트레와 도도의 의견도 궁금합니다~
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 (isAlreadyJoined(member)) { | ||
throw new FriendoglyException("이미 참여 중인 모임입니다."); | ||
} |
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 (canNotJoin(pet)) { | ||
throw new FriendoglyException("모임에 데려갈 수 없는 강아지가 있습니다."); | ||
} | ||
} | ||
|
||
private boolean canNotJoin(Pet pet) { | ||
return !allowedGenders.contains(pet.getGender()) || !allowedSizes.contains(pet.getSizeType()); | ||
} |
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 (canNotJoin(pet)) { | |
throw new FriendoglyException("모임에 데려갈 수 없는 강아지가 있습니다."); | |
} | |
} | |
private boolean canNotJoin(Pet pet) { | |
return !allowedGenders.contains(pet.getGender()) || !allowedSizes.contains(pet.getSizeType()); | |
} | |
if (!canJoin(pet)) { | |
throw new FriendoglyException("모임에 데려갈 수 없는 강아지가 있습니다."); | |
} | |
} | |
private boolean canJoin(Pet pet) { | |
return allowedGenders.contains(pet.getGender()) && allowedSizes.contains(pet.getSizeType()); | |
} |
not 절이 많아서 읽기 어려울 수 있겠네요
not이 2개인 것 보다는 하나인 게 나을 것 같은데, 이렇게 바꾸는 건 어떨까요?
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.
이건 그냥 제안인데
자연어에 가까운 메서드 네이밍을 해보자면
club.canBeJoinedBy(pet)
이런 이름도 괜찮을 것 같아요
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.
canJoinWith로 변경했습니다.
public static FilterCondition toFilterCondition(String filterCondition) { | ||
try { | ||
return valueOf(filterCondition); | ||
} catch (IllegalArgumentException e) { | ||
throw new FriendoglyException("존재하지 않는 FilterCondition 입니다."); | ||
} | ||
} |
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.
간단하게 가도 좋을듯요
public static FilterCondition toFilterCondition(String filterCondition) { | |
try { | |
return valueOf(filterCondition); | |
} catch (IllegalArgumentException e) { | |
throw new FriendoglyException("존재하지 않는 FilterCondition 입니다."); | |
} | |
} | |
public static FilterCondition from(String filterCondition) { | |
try { | |
return valueOf(filterCondition); | |
} catch (IllegalArgumentException e) { | |
throw new FriendoglyException("존재하지 않는 FilterCondition 입니다."); | |
} | |
} |
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 컨밴션이랑 헷갈렸습니다. 반영완료요
if (FilterCondition.toFilterCondition(request.filterCondition()) == FilterCondition.ABLE_TO_JOIN) { | ||
return clubs.stream() | ||
.filter(club -> club.isJoinable(member, pets)) | ||
.map(club -> new FindSearchingClubResponse(club, collectOverviewPetImages(club))) | ||
.toList(); | ||
} |
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 (FilterCondition.toFilterCondition(request.filterCondition()) == FilterCondition.ABLE_TO_JOIN) { | |
return clubs.stream() | |
.filter(club -> club.isJoinable(member, pets)) | |
.map(club -> new FindSearchingClubResponse(club, collectOverviewPetImages(club))) | |
.toList(); | |
} | |
if (FilterCondition.from(request.filterCondition()) == ABLE_TO_JOIN) { | |
return clubs.stream() | |
.filter(club -> club.isJoinable(member, pets)) | |
.map(club -> new FindSearchingClubResponse(club, collectOverviewPetImages(club))) | |
.toList(); | |
} |
filter condition이라는 말이 너무 많아서 헷갈리는데
메서드명 개선하고 static import 하면 어떨까요?
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.
동일성 비교를 enum 내부로 보내면서 개선해봤습니다.
public ApiResponse<List<FindSearchingClubResponse>> findSearching( | ||
@Auth Long memberId, | ||
@Valid FindSearchingClubRequest request | ||
) { | ||
return ApiResponse.ofSuccess(clubQueryService.findSearching(memberId, 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.
이거 findByFilter 하면 안 되나요 (5트)
찾다 찾다
검색을 찾다
와닿으시는지...
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.
수정 완료 했습니다.!!
FindSearchingClubRequest request = new FindSearchingClubRequest( | ||
FilterCondition.ALL.name(), | ||
address, |
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.
오 굿
public boolean isJoinable(Member member, List<Pet> pets) { | ||
boolean hasJoinablePet = pets.stream() | ||
.anyMatch(pet -> !canNotJoin(pet)); | ||
boolean isNotFull = !this.memberCapacity.isCapacityReached(countClubMember()); | ||
|
||
return hasJoinablePet && isNotFull && isAlreadyJoined(member) && isOpen(); | ||
} |
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.
public boolean isJoinable(Member member, List<Pet> pets) { | |
boolean hasJoinablePet = pets.stream() | |
.anyMatch(pet -> !canNotJoin(pet)); | |
boolean isNotFull = !this.memberCapacity.isCapacityReached(countClubMember()); | |
return hasJoinablePet && isNotFull && isAlreadyJoined(member) && isOpen(); | |
} | |
public boolean isJoinable(Member member, List<Pet> pets) { | |
boolean hasJoinablePet = pets.stream() | |
.anyMatch(pet -> canJoin(pet)); | |
boolean isNotFull = !this.memberCapacity.isCapacityReached(countClubMember()); | |
return hasJoinablePet && isNotFull && isAlreadyJoined(member) && isOpen(); | |
} |
아까 리뷰 반영하신다면 이렇게 쓸 수 있어서 더 깔끔할 것 같아요!
Co-authored-by: J-I-H-O <jeongjiho0731@gmail.com> Co-authored-by: jimi567 <repday0609@gmail.com>
Co-authored-by: J-I-H-O <jeongjiho0731@gmail.com> Co-authored-by: jimi567 <repday0609@gmail.com>
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 clubRepository.findAll(spec).stream() | ||
List<Club> clubs = clubRepository.findAll(spec); | ||
|
||
if (FilterCondition.toFilterCondition(request.filterCondition()) == FilterCondition.ABLE_TO_JOIN) { |
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문안에 많은 괄호가 있어서 if문의 조건이 흐려지는 것 같아요!
if (FilterCondition.toFilterCondition(request.filterCondition()) == FilterCondition.ABLE_TO_JOIN) { | |
FilterCondition filterConditon = FilterCondition.toFilterCondition(request.filterCondition()); | |
if (filterCondition == FilterCondition.ABLE_TO_JOIN) { | |
// OR if (filterCondition.isJoinable()) { |
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.
오 enum class 안에 로직을 넣어버리는게 제일 깔끔하겠네요
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.
오 트레 피드백 반영하면서, 적용했습니다.
Co-authored-by: J-I-H-O <jeongjiho0731@gmail.com> Co-authored-by: jimi567 <repday0609@gmail.com>
Co-authored-by: J-I-H-O <jeongjiho0731@gmail.com> Co-authored-by: jimi567 <repday0609@gmail.com>
Co-authored-by: J-I-H-O <jeongjiho0731@gmail.com> Co-authored-by: jimi567 <repday0609@gmail.com>
Co-authored-by: J-I-H-O <jeongjiho0731@gmail.com> Co-authored-by: jimi567 <repday0609@gmail.com>
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.
👍 LGTM
@@ -36,7 +37,7 @@ public ClubQueryService( | |||
this.petRepository = petRepository; | |||
} | |||
|
|||
public List<FindSearchingClubResponse> findSearching(Long memberId, FindSearchingClubRequest request) { | |||
public List<FindSearchingClubResponse> findFindByFilter(Long memberId, FindSearchingClubRequest 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.
public List<FindSearchingClubResponse> findFindByFilter(Long memberId, FindSearchingClubRequest request) { | |
public List<FindSearchingClubResponse> findByFilter(Long memberId, FindSearchingClubRequest request) { |
😉
Co-authored-by: jimi567 <repday0609@gmail.com>
Co-authored-by: jimi567 <repday0609@gmail.com>
이슈
개발 사항
전달 사항
ClubQueryServie#findSeaching()
의 분기문을 어떻게 리팩토링 할지 함께 고민 부탁드립니다.