-
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 구현 #607
Conversation
Test Results 44 files 44 suites 18s ⏱️ Results for commit 78e4fb0. ♻️ This comment has been updated with latest results. |
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 ApiResponse<FindPlaygroundDetailResponse> findById(@Auth Long memberId, @PathVariable Long id) { | ||
FindPlaygroundDetailResponse response = playgroundQueryService.findDetail(memberId, id); | ||
return ApiResponse.ofSuccess(response); |
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 ApiResponse<FindPlaygroundDetailResponse> findById(@Auth Long memberId, @PathVariable Long id) { | |
FindPlaygroundDetailResponse response = playgroundQueryService.findDetail(memberId, id); | |
return ApiResponse.ofSuccess(response); | |
public ApiResponse<FindPlaygroundDetailResponse> findById(@Auth Long memberId, @PathVariable Long playgroundId) { | |
FindPlaygroundDetailResponse response = playgroundQueryService.findDetail(memberId, playgroundId); | |
return ApiResponse.ofSuccess(response); |
혼란을 최대한 줄여보면 어떨까요?!
@NamedEntityGraph( | ||
name = "graph.PlaygroundMember", | ||
attributeNodes = { | ||
@NamedAttributeNode("member") | ||
} | ||
) |
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.
저거 제가 썻다가 괜히 이상한 약팔이가 된 것 같네요. 모임쪽에서도 지울 예정입니다.
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.
개인적으로 @NamedEntityGraph
사용은 피하는걸 선호합니다.
연관관계를 맺고있는 엔티티를 한 번에 조회한다는 사실 확인을 위해 repository 뿐만 아니라 도메인까지 확인해야 합니다.
특히 PlaygroundMember 엔티티의 경우 Member 단 하나의 엔티티와 연관관계를 맺고있는데, EntityGraph를 사용하고 싶다면 @NamedEntityGraph
대신 @EntityGraph
만으로도 충분한 것 같아요.
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.
PlaygroundMember는 쿼리 조회시 member는 어떤 상황(기능)에서도 필요한 정보라고 생각했습니다.
그렇기에, JPQL로 항상 쿼리를 작성하는 것보다 한번 엔티티 그래프 지정해주고 repository에서 해당네임만 붙이는 것이 편리하다 생각해서 엔티티그래프 사용했습니다.
중복되는 JPQL fetch join문을 상수화 했다고 생각하면 될 것 같습니다.(80%)
+내 손으로 엔티티그래프 코드 작성해보고 싶었다.(20%)
ㄴ 공부는 했는데 한번도 안쓰면 까먹으니까ㅋㅋ
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.
1개가 있는데 NamedEntityGraph를 왜썼냐고 물어보면 Playground까지 있었는데 삭제했는데 그대로 Named로 사용된 잔해네요
현재는 아래 처럼 바꿨습니다
@EntityGraph(attributePaths = "member")
List<PlaygroundMember> findByPlaygroundId(Long playgroundId);
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.
저거 제가 썻다가 괜히 이상한 약팔이가 된 것 같네요. 모임쪽에서도 지울 예정입니다.
우테코 4레벨 정도 됐으면, 약팔이에는 당하지 않습니다!
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.
관심사의 차이라고 이해하시면 좋을 것 같아요
저희는 엔티티늘 도메인으로 보고 있으니..
boolean isParticipating = playgroundMembers.stream() | ||
.anyMatch(playgroundMember -> playgroundMember.equalsMemberId(callMemberId)); |
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.
Playground
내부로 도메인 로직 넣을 수 있을 것 같네요!
playground.hasMember(member);
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 class Playground {
@Id
@GeneratedValue(strategy = GenerationType.IDENTITY)
private Long id;
@Embedded
private Location location;
}
playground는 member를 가지고 있지않아서, playground에 넣을 수는 없을 것 같습니다
대신 지금도 비교하는 로직은 도메인내부에 있습니다!
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.
고생하셧슴다~~~~
코멘트 확인해주세용. 꼭 반영해야 할 건 아니긴해요.
시간이 널널한건 아니니깐. .ㅠ
@NamedEntityGraph( | ||
name = "graph.PlaygroundMember", | ||
attributeNodes = { | ||
@NamedAttributeNode("member") | ||
} | ||
) |
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 List<PlaygroundPetDetail> getPlaygroundPetDetails( | ||
List<Pet> pets, | ||
PlaygroundMember petOwner, | ||
boolean isMyPet | ||
) { | ||
return pets.stream() | ||
.map(pet -> PlaygroundPetDetail.of( | ||
petOwner.getId(), | ||
pet, | ||
petOwner.getMessage(), | ||
petOwner.isInside(), | ||
isMyPet | ||
)) | ||
.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.
이거 dto인데 내부로 밀어 넣을 수 있을 것 같아요.
playgroundPetDetails.addAll( | ||
getPlaygroundPetDetails(pets, playgroundMember, isMyPet) | ||
); |
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.
좋은 생각이네요 service클래스 private 메서드 하나 지웠네요!
@NamedEntityGraph( | ||
name = "graph.PlaygroundMember", | ||
attributeNodes = { | ||
@NamedAttributeNode("member") | ||
} | ||
) |
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.
개인적으로 @NamedEntityGraph
사용은 피하는걸 선호합니다.
연관관계를 맺고있는 엔티티를 한 번에 조회한다는 사실 확인을 위해 repository 뿐만 아니라 도메인까지 확인해야 합니다.
특히 PlaygroundMember 엔티티의 경우 Member 단 하나의 엔티티와 연관관계를 맺고있는데, EntityGraph를 사용하고 싶다면 @NamedEntityGraph
대신 @EntityGraph
만으로도 충분한 것 같아요.
@NoArgsConstructor(access = AccessLevel.PROTECTED) | ||
@Getter | ||
public class PlaygroundMember { |
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.
@ManyToOne(fetch = FetchType.LAZY)
@JoinColumn(name = "member_id", nullable = false)
private Member member;
한 회원은 한 번에 하나의 playground에 참여할 수 있도록 정책을 잡은걸로 기억해요.
그럼 PlayGround와 PlaygroundMember는 일대다 관계가 맞지만, PlaygroundMember와 Member는 일대일 관계가 되어야하지 않을까요?
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.
중요한 부분 캐치 감사합니다 다행이다
@EntityGraph(value = "graph.PlaygroundMember") | ||
List<PlaygroundMember> findByPlaygroundId(Long playgroundId); |
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.
@EntityGraph(value = "graph.PlaygroundMember") | |
List<PlaygroundMember> findByPlaygroundId(Long playgroundId); | |
@EntityGraph(value = "graph.PlaygroundMember") | |
List<PlaygroundMember> findAllByPlaygroundId(Long playgroundId); |
protected Member saveMember(String name) { | ||
return memberRepository.save( | ||
new Member(name, "tag", "imageurl") | ||
); | ||
} | ||
|
||
protected Pet savePet(Member member) { | ||
return petRepository.save( | ||
new Pet( | ||
member, | ||
"petName", | ||
"description", | ||
LocalDate.of(2023, 10, 02), | ||
SizeType.LARGE, | ||
Gender.FEMALE, | ||
"imgaeUrl" | ||
) | ||
); | ||
} |
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.
무슨 의미일까요? 테스트에서 사용중이라 필요합니다.
놀이터 상세 정보에서 전체 강아지 마리수, 도착한 강아지 마리수 때문에 강아지 정보도 저장해줘야합니다.
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 클래스인걸 확인하지 못했네요!
해당 클래스 내부에서 사용되지 않고있어 궁금해서 여쭤보았습니다~
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 title 컨벤션에 맞게 수정했어요~ |
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.
도도 고생하셨습니다~~~~
protected Member saveMember(String name) { | ||
return memberRepository.save( | ||
new Member(name, "tag", "imageurl") | ||
); | ||
} | ||
|
||
protected Pet savePet(Member member) { | ||
return petRepository.save( | ||
new Pet( | ||
member, | ||
"petName", | ||
"description", | ||
LocalDate.of(2023, 10, 02), | ||
SizeType.LARGE, | ||
Gender.FEMALE, | ||
"imgaeUrl" | ||
) | ||
); | ||
} |
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 클래스인걸 확인하지 못했네요!
해당 클래스 내부에서 사용되지 않고있어 궁금해서 여쭤보았습니다~
78e4fb0
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.
good
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 final int PLAYGROUND_EXCEPTION_DISTANCE_THRESHOLD = 300; | ||
|
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.
말이 좀 어려운 것 같은데 PLAYGROUND_RADIUS_METER = 150
이렇게 해놓고 밑에서는 *2 해서 쓰는게 어떨까요
"예외가 발생하는 거리의 경계"는 결국 RADIUS * 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.
RADIUS * 2가 하나의 수식처럼 느껴저서 변경한 부분이었는데요,
RADIUS * 2 라는 것이 왜 예외가 발생하는 숫자인가를 생각하게 하는 것이 좋을지,
PLAYGROUND_EXCEPTION_DISTANCE_THRESHOLD = 300 로 그냥 300m넘으면 예외가 발생하는구나 라고 느끼게 하는 것이 좋을지를 생각했습니다.
도메인 규칙을 이해하는 것이 더 중요하다고 생각하여 트레의 추천대로 바꿨습니다
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 final int PLAYGROUND_EXCEPTION_DISTANCE_THRESHOLD = 300; | |
private static final int PLAYGROUND_RADIUS = 150; | |
private static final int MAX_NON_OVERLAP_DISTANCE = PLAYGROUND_RADIUS * 2; |
Location location = new Location(latitude, longitude); | ||
double startLatitude = GeoCalculator.calculateLatitudeOffset(latitude, | ||
-PLAYGROUND_EXCEPTION_DISTANCE_THRESHOLD); | ||
double endLatitude = GeoCalculator.calculateLatitudeOffset(latitude, | ||
PLAYGROUND_EXCEPTION_DISTANCE_THRESHOLD); | ||
double startLongitude = GeoCalculator.calculateLongitudeOffset(latitude, longitude, | ||
-PLAYGROUND_EXCEPTION_DISTANCE_THRESHOLD); | ||
double endLongitude = GeoCalculator.calculateLongitudeOffset(latitude, longitude, | ||
PLAYGROUND_EXCEPTION_DISTANCE_THRESHOLD); |
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.
Location한테 물어봐도 좋을 것 같아요
// Location
public double plusLatitude(double meter) {
return GeoCalculator.calculateLatitudeOffset(latitude, meter);
}
public double minusLatitude(double meter) {
return plusLatitude(-meter);
}
public double plusLongitude(double meter) {
return GeoCalculator.calculateLongitudeOffset(latitude, longitude, meter);
}
public double minusLongitude(double meter) {
return plusLongitude(-meter);
}
Location location = new Location(latitude, longitude); | ||
double startLatitude = GeoCalculator.calculateLatitudeOffset(latitude, | ||
-PLAYGROUND_EXCEPTION_DISTANCE_THRESHOLD); | ||
double endLatitude = GeoCalculator.calculateLatitudeOffset(latitude, | ||
PLAYGROUND_EXCEPTION_DISTANCE_THRESHOLD); | ||
double startLongitude = GeoCalculator.calculateLongitudeOffset(latitude, longitude, | ||
-PLAYGROUND_EXCEPTION_DISTANCE_THRESHOLD); | ||
double endLongitude = GeoCalculator.calculateLongitudeOffset(latitude, longitude, | ||
PLAYGROUND_EXCEPTION_DISTANCE_THRESHOLD); |
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.
Location location = new Location(latitude, longitude); | |
double startLatitude = GeoCalculator.calculateLatitudeOffset(latitude, | |
-PLAYGROUND_EXCEPTION_DISTANCE_THRESHOLD); | |
double endLatitude = GeoCalculator.calculateLatitudeOffset(latitude, | |
PLAYGROUND_EXCEPTION_DISTANCE_THRESHOLD); | |
double startLongitude = GeoCalculator.calculateLongitudeOffset(latitude, longitude, | |
-PLAYGROUND_EXCEPTION_DISTANCE_THRESHOLD); | |
double endLongitude = GeoCalculator.calculateLongitudeOffset(latitude, longitude, | |
PLAYGROUND_EXCEPTION_DISTANCE_THRESHOLD); | |
Location location = new Location(latitude, longitude); | |
double startLatitude = location.minusLatitude(PLAYGROUND_RADIUS_METER * 2); | |
double endLatitude = location.plusLatitude(PLAYGROUND_RADIUS_METER * 2); | |
double startLongitude = location.minusLongitude(PLAYGROUND_RADIUS_METER * 2); | |
double endLongitude = location.plusLongitude(PLAYGROUND_RADIUS_METER * 2); |
그럼 이렇게 쓸 수 있을듯! Location
정의해놓은 이상 최대한 활용하면 좋을 것 같아서요
// then | ||
assertThat(playgrounds.size()).isOne(); |
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.
assertThat(playgrounds).hasSize(1)
이렇게 하면 어떨까요? 점 하나라도 줄이기
// given | ||
playgroundRepository.save( | ||
new Playground(new Location(10, 10)) | ||
); | ||
playgroundRepository.save( | ||
new Playground(new Location(10, 12)) | ||
); | ||
|
||
// when | ||
List<Playground> playgrounds = playgroundRepository | ||
.findAllByLocation_LatitudeBetweenAndLocation_LongitudeBetween(9, 11, 9, 11); |
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.
위도 경도가 int가 아닌만큼
더 작은 단위로 경계값 챙겨가도 좋을 것 같아요
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.
정확히 오차가 어느만큼 나는 지도 알기 어려운 뿐더러, 1m의 오차범위로 검증하는 것은 충분하다고 생각합니다.
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.
meter가 아니네요..,
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.
이 테스트는 쿼리메소드가 잘 작동하는 지에 대한 테스트이고 구현이 제가 한게 아니니까 경계값 테스트 안해도 되겠네요!
} | ||
|
||
|
||
@DisplayName("멤버가 놀이터를 등록하면 해당 멤버는 놀이터에 자동으로 참가") |
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.
new line 2개네요!
double latitude = 37.516382; | ||
double longitudeA = 127.120040; | ||
double longitudeFar299mFromA = 127.123430; |
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.
longitudeFar299mFromA
는 GeoCalculator
로 만들어도 좋겠네요
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.
latitude가 299m 차이 나는 테스트 케이스도 만들면 어떨까요?
|
||
@DisplayName("놀이터들의 위치를 조회할 때, 내가 참여했는 지 알 수있다(true)") | ||
@Test | ||
void findLocationsWithIsParticipatingFalse() { |
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.
true -> false
package com.happy.friendogly.utils; | ||
|
||
public class GeoCalculator { | ||
|
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.
요긴하게 쓰일 수 있을 것 같아요.
Location
에 있는 calculateDistanceInMeters()
의 세부 로직도 여기로 넣어도 될 것 같네요.
public static SavePlaygroundResponse from(Playground playground) { | ||
return new SavePlaygroundResponse( | ||
playground.getId(), | ||
playground.getLocation().getLatitude(), | ||
playground.getLocation().getLongitude() | ||
); | ||
} |
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 playgrounds.stream() | ||
.map(playground -> new FindPlaygroundLocationResponse( | ||
playground.getId(), | ||
playground.getLocation().getLatitude(), | ||
playground.getLocation().getLongitude(), | ||
playgroundMemberRepository.existsByPlaygroundIdAndMemberId(playground.getId(), memberId) |
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.
스트림에서 쿼리를 날리고 있네요!
조회된 모든 playgrounds 수만큼 쿼리가 나갈텐데, N+1 문제를 직접 만든 상황 아닐까요? 🥺
먼저 현재 Member가 참여한 Playground를 조회해온 다음, 스트림에서는 현재 보고있는 Playground가 이전에 조회된 Playgound인지 확인하도록 개선할 수 있을 것 같아요~
public List<FindPlaygroundLocationResponse> findLocations(Long memberId) {
List<Playground> playgrounds = playgroundRepository.findAll();
Playground joinedPlayground = playgroundMemberRepository.findByMemberId(memberId);
return playgrounds.stream()
.map(playground -> new FindPlaygroundLocationResponse(
playground.getId(),
playground.getLocation().getLatitude(),
playground.getLocation().getLongitude(),
playground.isSameWith(joinedPlayground)
)).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.
와 그렇네요~! 두번의 쿼리로 해결될 문제였네요!! 너무 좋은 의견입니다!
이슈
개발 사항
리뷰 요청 사항
전달 사항