[hotfix] Spot 저장방식 수정 및 추가 #155
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| .status(StatusEnum.OK.getStatusCode()) | ||
| .message(StatusEnum.OK.getCode()) | ||
| .data(contentId+"번 관광지 찜") | ||
| .build()); | ||
| } | ||
|
|
||
| public ResponseEntity<StatusResponse> cancelSpotLike(Long contentId, Member member) { |
There was a problem hiding this comment.
코드 리뷰를 해드리겠습니다.
- patch 코드는 없기 때문에 현재 코드 스니펫만을 기준으로 리뷰를 진행하겠습니다.
- 리뷰할 코드 부분은 "likeSpot" 메서드의 수정 부분입니다.
해당 코드의 문제점이나 개선 제안은 다음과 같습니다.
- 이미 좋아요한 내역을 확인하는 로직(
if(heartRepository.findByMemberAndSpot(member, spot).isPresent()))에서else분기가 제거되었습니다. 이로 인해 이미 좋아요한 경우와 좋아요하지 않은 경우 구분이 없어지게 됩니다. 예외 처리 부분을 잘 검토해보세요. - 좋아요 관련 정보를 저장하는 부분(
heartRepository.save(),spot.likeSpot())이 항상 실행됩니다. 이미 좋아요한 경우라도 무조건 처리되므로, 중복 저장 및 카운트 증가 등의 부작용이 있을 수 있습니다. 불필요한 DB 작업을 줄일 수 있는 방법을 고려해보세요. - 응답 결과로 반환되는
StatusResponse객체 생성 부분은 동일한 로직이throw전과throw후에 모두 존재합니다. 중복된 코드를 제거하고 한 곳에서 생성하여 사용하도록 개선해보세요.
이상이 코드 리뷰에 대한 제안사항입니다. 해당 부분들을 고려하여 코드를 수정하면 더욱 개선된 결과를 얻을 수 있을 것입니다.
| @@ -63,7 +63,7 @@ public ResponseEntity<StatusResponse> recommendFiveSpot(@PathVariable Long conte | |||
| @GetMapping("/{contentId}") | |||
| public ResponseEntity<StatusResponse> getSpotDetail(@PathVariable Long contentId) { | |||
| Spot spot = spotService.findSpotByContentId(contentId); | |||
| Object commonDetail = tourApiService.getCommonDetail(spot); | |||
| Object commonDetail = tourApiService.getCommonDetail(spot.getCategory().getCode(), spot.getContentId()); | |||
| Object detailInfo = tourApiService.getDetailInfo(spot); | |||
| JSONArray imageArr = tourApiService.getDetailImages(contentId); | |||
| return spotService.responseDetailInfo(commonDetail, detailInfo, imageArr, spot); | |||
There was a problem hiding this comment.
이 코드 패치를 간단히 검토해 드리겠습니다.
- Lombok에 @slf4j 어노테이션을 추가하여 로깅 기능을 사용하고 있습니다.
- '/api/v1/spots' 경로에 대한 POST 요청을 처리하는
/sync메서드가 추가되었습니다. 이 메서드는 페이지, 크기 및 날짜에 따른 관광 데이터 동기화를 수행합니다. - '/api/v1/spots/{contentId}' 경로에 대한 GET 요청을 처리하는
getSpotDetail메서드가 수정되었습니다. 이제tourApiService.getCommonDetail메서드를 호출할 때 카테고리 코드와 함께 호출합니다.
개선 제안:
- 이 코드 패치에서 보여지는 만큼 추가적인 버그 또는 문제는 보이지 않습니다. 하지만 실제 구현 상황과 요구 사항에 따라 개선할 수 있는 사항이 있을 수 있습니다.
- 코드를 개선하거나 추가하기 전에 주석을 제공하면 좋을 것 같습니다. 코드의 인텐션과 목적을 이해하는 데 도움이 됩니다.
- 단위 테스트를 작성하여 코드의 동작을 검증하는 것이 좋습니다.
참고: 이 답변은 지식 절단(latest 2021)을 기준으로 작성되었습니다. 코드 패치 내의 사용되는 외부 라이브러리나 프레임워크에 대한 자세한 내용은 최신 문서 또는 해당 라이브러리/프레임워크 공식 문서를 참조해야 합니다.
| if(!(imageArr == null)) { | ||
| for (Object object : imageArr) { | ||
| JSONObject item = (JSONObject) object; | ||
| String imageUrl = (String) item.get("originimgurl"); | ||
| imageList.add(imageUrl); | ||
| } | ||
| } | ||
| if(imageList.isEmpty() && !spot.getImageUrl().isEmpty()) { | ||
| imageList.add(spot.getImageUrl()); | ||
| } | ||
|
|
||
| //상세 정보 | ||
| JSONObject additionalInfo = (JSONObject) detailInfo; |
There was a problem hiding this comment.
이 코드는 SpotService 클래스의 일부분입니다. 아래는 몇 가지 개선 제안과 버그 확인 사항입니다:
-
스케줄링:
@EnableScheduling과@Scheduled어노테이션을 사용하여 매일 특정 시간에syncTourDataEveryDay()메서드가 실행되도록 예약하였습니다. 하지만 해당 메서드의 동작이 코드에 포함되어 있지 않아 작동 방식을 확인할 수 없습니다. -
로깅:
log변수를 사용하여 로깅을 위한 Slf4j 로거를 선언하였습니다. 로그 문장들은log.info()로 시작하며, 메서드의 수행 상태나 로그인한 데이터 등을 기록합니다. -
여러 개의 라이브러리 추가:
lombok.extern.slf4j.Slf4j,org.apache.commons.lang3.EnumUtils및java.time패키지에서 일부 유틸리티 클래스 및 API를 임포트하였습니다. -
changeAndSave()메서드 리네이밍:changeAndSave()메서드가synchronizeTourData()메서드로 변경되었습니다. 이 메서드는 JSON 응답값에서 필요한 정보를 추출하여 DB에 저장하는 역할을 합니다. -
synchronizeTourData()메서드 수정:synchronizeTourData()메서드에서 한 번의 루프를 사용하여 JSON 배열을 처리하고, 포함된 각 항목에 대한 조건부 로직을 통해 저장 또는 업데이트를 수행합니다. 이 메서드의 구조와 동작은 적합해 보입니다. -
updateSpot()메서드 수정:updateSpot()메서드는 기존 Spot을 업데이트하는 데 사용됩니다.Spot객체에서 정보를 가져와서 변경되지 않은 경우 데이터베이스에 대한 실제 업데이트 작업을 수행하지 않을 수 있습니다. -
saveSpot()메서드 추가:saveSpot()메서드는 새로운 Spot을 저장하기 위해 사용됩니다.synchronizeTourData()메서드 내에서 호출됩니다. -
getSpotAbout()메서드 추가: XML API response에서 spot에 대한 정보를 추출하기 위한 메서드입니다. -
addImage()메서드 추가: Spot에 이미지가 없을 경우 detail 이미지 중 첫 번째 사진을 썸네일로 추가하는 메서드입니다. -
responseDetailInfo()메서드 수정: 이미지 리스트를 확인하고 비어 있으면spot.getImageUrl()을 추가하는 부분이 추가되었습니다.
위의 내용을 고려하여 코드 리뷰를 진행할 수 있습니다.
| URL url = new URL(BASE_URL + "detailImage1?MobileOS=ETC&MobileApp=Kuddy&_type=json&contentId=" + | ||
| contentId + | ||
| URL url = new URL(BASE_URL + "detailImage1?contentId=" + | ||
| contentId + DEFAULT_QUERY_PARAM + | ||
| "&serviceKey=" | ||
| + SECRET_KEY); | ||
|
|
There was a problem hiding this comment.
아래는 코드 패치입니다. 코드 리뷰를 간략하게 도와드리겠습니다. 버그의 위험과/또는 개선 제안을 환영합니다.
DEFAULT_QUERY_PARAM변수를 추가하여 기본 쿼리 매개변수를 사용합니다.getApiDataList메서드가getSyncList로 변경되었습니다.areaBasedList1API 호출을areaBasedSyncList1로 변경하여 수정된 시간을 기준으로 동기화된 목록을 받아옵니다.getLocationBasedApi메서드가 삭제되었습니다.getCommonDetail메서드의 매개변수로Spot객체 대신category와contentId를 사용합니다.getDetailInfo메서드의 URL 생성 부분에서 기본 매개변수 값이 추가되었습니다.getDetailImages메서드의 URL 생성 부분에서 기본 매개변수 값이 추가되었습니다.
주의할 사항:
- 수정된 기능에 대한 테스트가 필요합니다.
- URL 조립 시 쿼리 매개변수 값을 인코딩하는 것이 좋습니다.
특정 버그를 식별하지 못하였으며, 리뷰를 통해 기능 및 성능 개선을 고려할 수 있을 것입니다.
|
|
||
| public static boolean hasValue(String code) { | ||
| return Arrays.stream(Category.values()).anyMatch(v -> v.code.equals(code)); | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
주어진 코드 패치에 대해 간단한 코드 리뷰를 도와드리겠습니다. 버그 위험 및 개선 제안을 환영합니다.
-
패치 전체적으로 보면, lombok 어노테이션을 사용하여 Getter와 RequiredArgsConstructor가 추가되었습니다. 이들은 자동으로 필요한 메서드를 생성해주는데, 현재 코드에서 필요한지 여부를 파악하기 어렵기 때문에 그대로 두겠습니다.
-
Category열거형 클래스에 새로운 기능이 추가되었습니다.HashMap을 사용하는 것보다CATEGORY_CODE_MAP이라는 불변의 맵을 사용하도록 변경되었습니다. 이를 통해 카테고리 코드와 이름을 매핑하는 방법이 개선되었으며, 코드 검색과 변환 작업의 성능을 향상시켰습니다. -
valueOfCode메서드는 주어진 코드에 해당하는Category값을 반환합니다. 이전엔values()메서드로 모든 열거형 객체를 반복하면서 일치하는 값을 찾았지만, 이제는CATEGORY_CODE_MAP을 사용하여 빠르게 값을 가져올 수 있습니다. -
hasValue메서드는 주어진 코드에 해당하는Category값이 있는지 여부를 확인합니다. 이를 위해 스트림을 사용하여 모든Category값을 반복하면서 일치하는지 검사합니다. -
파일의 끝에 개행 문자(Newline)가 없습니다. 코드 스타일 관련 이슈이므로, 코드 끝에 개행 문자를 추가해주면 좋을 것입니다.
위의 내용은 주어진 코드 패치에 대한 간단한 개선 사항입니다. 다른 문제가 있는지 자세히 파악하려면 전체 소스 코드 및 프로그램 요구 사항을 확인해야 합니다.
|
|
||
| public static District valueOfCode(final String code) { | ||
| return District.valueOf(DISTRICT_CODE_MAP.get(code)); | ||
| } | ||
| } |
There was a problem hiding this comment.
해당 코드 패치에 대해 간단한 코드 리뷰를 도와드리겠습니다.
이 코드의 목적은 지역 코드에 해당하는 District 열거형 값을 가져오는 것으로 보입니다. 코드에는 큰 문제점은 없어 보이나, 몇 가지 개선점과 버그 가능성을 제시해드리겠습니다.
-
버그 가능성:
DISTRICT_MAP에서area와 관련된 값이 없는 경우,NullPointerException이 발생할 수 있습니다. 이 경우 예외 처리를 추가하는 것이 좋습니다.
-
개선 제안:
DISTRICT_CODE_MAP을 생성하는 방법은 올바르게 보입니다.DISTRICT_CODE_MAP필드는 불변이므로,Collections.unmodifiableMap()에 의한 래핑이 적절합니다.- 그러나
DISTRICT_CODE_MAP은valueOfCode()메서드 하나에서만 사용되므로, 클래스 필드로 정의하는 대신 메서드 내부에서 지역 변수로 선언하여 사용하는 것이 좋습니다. 이렇게 하면 필드의 범위를 제한할 수 있습니다.
다른 사소한 문제나 개선 포인트가 있다면 알려주세요.
| public void setImageUrl(String imageUrl) { | ||
| this.imageUrl = imageUrl; | ||
| } | ||
|
|
||
| } No newline at end of file |
There was a problem hiding this comment.
패치된 코드의 주요 변경 사항은 다음과 같습니다:
Spot클래스에about필드가 추가되었습니다. 이 필드는 최대 10000자의 긴 문자열을 저장할 수 있습니다.modifiedTime필드가 추가되었습니다. 이 필드는 문자열로 표현된 수정 시간을 저장합니다.Spot생성자와update메서드에about및modifiedTime매개변수가 추가되었습니다.update메서드는Spot객체의 여러 속성을 한 번에 업데이트하는 데 사용됩니다.setImageUrl메서드가 추가되었습니다. 이 메서드는imageUrl필드를 단독으로 업데이트합니다.
버그 리스크:
- 특정 구체적인 버그 리스크는 코드 내에서 확인할 수 없습니다. 그러나 주의해야 할 몇 가지 지점이 있습니다.
about필드의 길이 제한이 10000으로 설정되어 있으므로 데이터베이스 스키마와 일치하는지 확인하는 것이 좋습니다.mapX및mapY필드의 길이 제한이 20으로 설정되어 있습니다. 이 값이 충분한지 여부를 확인해야 합니다. 좌표값이 잘리지 않도록 적절한 길이인지 확인해주세요.modifiedTime필드의 데이터 타입이 문자열로 설정되어 있는데, 이 값이 일반적인 형식으로 표현되는지 확인하고 가능한한 정규화하여 저장하는 것이 좋습니다.update메서드에서 필드들을 업데이트 할 때, 모든 필드를 갱신해야 하므로 실수로 누락하지 않도록 유의해야 합니다.
개선 사항:
- 코드 리뷰에서는 전체 시스템 및 의도에 대한 완전한 정보를 제공하기 어렵습니다. 그러나 추가할 수 있는 몇 가지 개선 사항이 있습니다:
- 예외 처리: 적절한 예외 처리가 이루어지고 있는지 확인하세요. 프로그램 안정성을 보장하기 위해서는 필요한 예외 처리를 구현하는 것이 중요합니다.
- 주석 추가: 코드의 의도와 기능에 대한 주석을 추가하여 코드를 이해하기 쉽게 만드세요.
- 코드 스타일 준수: 코드 스타일 가이드라인에 따라 코드를 일관되게 작성하는 것이 좋습니다.
|
동기화 부분 걱정했는데 최적의 방법으로 구현해주셔서 감사합니다!! 👍 짱짱! |
| @@ -54,7 +52,7 @@ public ResponseEntity<StatusResponse> getSpotsList(@RequestParam(value = "page") | |||
| @GetMapping("/{contentId}") | |||
| public ResponseEntity<StatusResponse> getSpotDetail(@PathVariable Long contentId) { | |||
| Spot spot = spotService.findSpotByContentId(contentId); | |||
| Object commonDetail = tourApiService.getCommonDetail(spot); | |||
| Object commonDetail = tourApiService.getCommonDetail(spot.getCategory().getCode(), spot.getContentId()); | |||
| Object detailInfo = tourApiService.getDetailInfo(spot); | |||
| JSONArray imageArr = tourApiService.getDetailImages(contentId); | |||
| return spotService.responseDetailInfo(commonDetail, detailInfo, imageArr, spot); | |||
There was a problem hiding this comment.
해당 코드 패치에 대한 간단한 코드 리뷰를 도와드리겠습니다. 버그 위험 및 개선 제안 사항이 있다면 언제든지 환영합니다:
lombok.extern.slf4j.Slf4j를 추가하여 로깅을 적용할 수 있습니다./api/v1/spots/test대신/api/v1/spots/sync로 엔드포인트 이름을 변경할 수 있습니다.saveTestData()메서드를syncTourData()로 변경하고, 메서드 파라미터로size와date를 추가하였습니다.tourApiService.getApiDataList(page, category)의 대신tourApiService.getSyncList(page, size, date)를 호출하여 데이터를 동기화할 수 있습니다./api/v1/spots/{contentId}로 GET 요청이 올 경우category정보를 함께 전달하도록tourApiService.getCommonDetail(spot.getCategory().getCode(), spot.getContentId())로 수정되었습니다.
주의할 점:
해당 코드에서 누락된 부분이 발견되지 않았으며, 기타 잠재적인 버그도 없어 보입니다. 그러나 완벽한 코드 리뷰를 위해서는 클래스 선언부가 보이지 않기 때문에 해당 부분은 확인할 수 없습니다.
| if(!(imageArr == null)) { | ||
| for (Object object : imageArr) { | ||
| JSONObject item = (JSONObject) object; | ||
| String imageUrl = (String) item.get("originimgurl"); | ||
| imageList.add(imageUrl); | ||
| } | ||
| } | ||
| if(imageList.isEmpty() && !spot.getImageUrl().isEmpty()) { | ||
| imageList.add(spot.getImageUrl()); | ||
| } | ||
|
|
||
| //상세 정보 | ||
| JSONObject additionalInfo = (JSONObject) detailInfo; |
There was a problem hiding this comment.
코드 리뷰 요청을 받았습니다. 다음은 해당 코드 패치의 리뷰입니다:
-
스프링 스케줄링 설정:
@EnableScheduling어노테이션을 사용하여 스프링 스케줄링을 활성화 했습니다.@Scheduled어노테이션을 사용하여 매일 아침 6시에syncTourDataEveryDay()메서드가 실행됩니다. -
불필요한 import 문:
org.apache.commons.lang3.EnumUtils와org.json.simple.JSONArrayimport 문이 추가되어 있지만 해당 클래스들이 사용되지 않고 있습니다. 이러한 불필요한 import 문은 제거할 수 있습니다. -
로깅 추가:
lombok.extern.slf4j.Slf4j어노테이션을 사용하여 log 변수를 추가하고,log.info()메서드를 호출하여 로그를 남기고 있습니다. 로깅은 코드 디버깅 및 유지보수에 도움이 되므로 유용합니다. -
올바른 데이터 타입 사용:
contentId는 Long 타입으로 변환해야 합니다.Long.valueOf((String) item.get("contentid"))를 사용하여 contentId를 Long 타입으로 변환하세요. -
열거형 비교 방법:
contenttypeid와sigungucode와 같은 값을 비교하는 부분에서for반복문 대신 EnumUtils를 활용하여 비교하고 있습니다. 이런 경우에는 EnumUtils 대신 열거형 클래스의 정적 메서드를 사용하는 것이 더 간결하고 명확합니다. -
업데이트 및 저장 로직 분리:
synchronizeTourData()메서드를 새로 추가하고,saveSpot()및updateSpot()메서드를 해당 메서드로 분리할 수 있습니다. 이렇게 하면 코드를 더 읽기 쉽고 유지 관리하기 쉽게 만들 수 있습니다. -
changeAndSave()메서드명 변경: 현재changeAndSave()메서드는 JSON 응답값에서 필요한 정보를 db에 저장하는 작업을 수행합니다. 그러나 메서드 명에서는 그 의도가 명확하지 않습니다. 역할과 의도를 잘 표현하는 이름으로 변경하는 것이 좋습니다. -
이미지 처리 로직 개선:
addImage()메서드에서 이미지 URL을 추가할 때, 이미 URL이 있는 경우에도 다시 추가하고 있습니다. 이미 URL이 없는 경우에만 추가해야 합니다. 이 부분을 개선하여 중복 추가되는 오류를 수정하세요. -
리턴 문 제한: 코드 상세 설명을 위해 너무 많은 주석이 사용되고 있습니다. 보통 간결한 코드와 명확한 네이밍은 자체 설명력을 가지고 있으므로, 필요한 경우에만 주석을 달도록 하는 것이 좋습니다.
이상이 코드 리뷰입니다. 해당 패치에서 발견된 각 위험한 버그 및 개선 사항에 대해 설명하였습니다.
기능 명세
결과
함께 의논할 점
close #133