-
Notifications
You must be signed in to change notification settings - Fork 0
지도 범위 내 클러스터형, 마커형 나의 다이어리 조회 API 구현 #34
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
|
Claude의 전체 변경사항 및 관련 파일에 대한 리뷰: 개선된 사항:
주요 이슈:
관련 파일에 대한 영향 분석:
전반적인 의견: |
|
Claude의 전체 변경사항 및 관련 파일에 대한 리뷰: Error: API returned status code 529 |
TTaiJin
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.
고생하셨습니다! 지도 관련 부분은 다뤄본적이 없어서 완벽하게 이해는 조금 힘드네요..ㅎ
다이어리 생성할 때 해당 지역에 대한 다이어리 개수 증가는 추가하신거같은데 삭제할 때는 해당 지역의 다이어리 개수 감소가 없는 거 같은데 한번 확인 부탁드립니다!
dnzp75
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.
감소는 생각치 못하고 있었네요. 올라와있는 PR들 머지 후 새로운 브랜치에서 작업 진행하겠습니다!
InJunKangW
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.
환경 설정도 복잡하고 코드도 많아서 어려우셨을텐데 고생 많으셨습니다.
리팩토링 관점에서만 몇 가지 리뷰 남겨서, 그 부분 확인해주시면 될 것 같아요!
| @RequestParam double west, | ||
| @RequestParam double east, | ||
| @RequestParam int zoom, | ||
| @RequestParam Long userId // 실무에서는 인증 기반으로 가져오겠지만 지금은 param 사용 |
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.
@AuthenticationPrincipal CustomOAuth2User customOAuth2User,이거로 받고 getUserId() 호출 하기로 했었던 걸로 기억을 해요. 기존 건희님 코드 (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.
스웨거에서 바로 테스트 진행하고자 임시로 이렇게 했었습니다. 다시 수정해두겠습니다~
| return siggAreasRepository.findSiggAreaClusters(south, north, west, east); | ||
| } | ||
|
|
||
| public void increaseRegionDiaryCount(Double lat, Double lon) { |
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 DiaryClusterResponseDto { | ||
| } | ||
|
|
||
| public static DiaryClusterResponseDto of(SidoAreas region, Long diaryCount) { |
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.
SidoAreas랑 SiggAreas를 Areas라는 추상 클래스를 상속하도록 변경하고
public static <T extends Areas> DiaryClusterResponseDto of(T region, Long diaryCount){
return new DiaryClusterResponseDto(
region.getName(),
region.getId(),
region.getLat(),
region.getLon(),
diaryCount
);
}같이 추상화 할 수 있을 것 같아요!
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.
저도 entity 클래스 추상화를 고민하고 있었는데, 자세한 예시까지 작성해주셔서 훨씬 이해하기 쉬웠습니다. 감사합니다.
추상화를 주저했던 이유는, entity 내부에 오버라이드 메서드를 추가해야 하고 나중에 응답 데이터 형식이 바뀔 경우 해당 메서드들도 함께 수정해줘야 한다는 점에서 유지보수 부담이 생길 수 있겠다는 우려 때문이었던 것 같아요.
근데 어차피 of() 메서드에서 getter를 사용하는 구조라면 결국 책임이 어디에 위치하느냐의 차이일 수도 있다는 생각도 드네요. 꼼꼼한 리뷰 감사합니다
|
|
||
| Optional<T> findRegionByLatLon(double lat, double lon); | ||
|
|
||
| String extractAreaName(T area); |
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.
위의 DiaryClusterResponseDto 에서 남긴 것과 같은 맥락에서,
public interface AreaRegion<T extends Areas> {
List<T> findRegionsInBounds(double south, double north, double west, double east);
Optional<T> findRegionByLatLon(double lat, double lon);
default String extractAreaName(T area){
return area.getName()
};
default DiaryClusterResponseDto toDto(T area, Long count){
return DiaryClusterResponseDto.of(area, count);}
}
}로 만들면
밑의 두 매서드는 오버라이딩 안해도 될 것 같아요.
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.
그러네요. 제네릭 사용하는 것에 있어서 이해가 부족했던 것 같네요. 수정하면서 배워갑니다!
|
|
Claude의 전체 변경사항 및 관련 파일에 대한 리뷰: 개선된 사항:
주요 이슈:
관련 파일에 대한 영향 분석:
전반적인 의견: |



📌 PR 제목
-> 이전 브랜치 병합하여 계속 작업 진행
✨ 변경 사항
지도 범위 내 클러스터형 나의 다이어리 조회 API
지도 범위 내 마커형 나의 다이어리 조회 API
🔍 변경 이유
✅ 체크리스트
📸 스크린샷 (선택)
📌 참고 사항