GET: /api/v1/traffics/{trafficId} 컨트롤러 구현 #15
Conversation
<description> state 삭제 및 시간을 나타내는 필드의 타입을 Float으로 변경
<description> 신호등 사이클, 잔여시간 정보 Long -> Float 잔여시간 네이밍 remainTime -> timeLeft 로 변경
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| .viewName(trafficEntity.getName()) | ||
| .build(); | ||
| } | ||
| } |
There was a problem hiding this comment.
코드를 살펴보니 다음과 같은 개선점과 버그 위험이 있을 수 있습니다:
-
TrafficDetailConverter클래스:TrafficEntity에서 반환된 ID는null일 수 있으므로id(trafficEntity.getId())부분에서null체크가 필요합니다.- 음수 주기가 올바른 값인지 확인하고 해당 사항에 대한 검증 로직을 추가해야 합니다.
- 예측된 데이터가 null인 경우의 처리 로직이 필요할 수 있습니다.
-
코드 품질 및 가독성:
- Javadoc 주석이 잘 작성되어 있습니다.
- 변수 및 메소드 이름이 명확하며 깔끔합니다.
- 추상화 수준이 적합하며 불필요한 복잡성이 없습니다.
-
일반적인 개선사항:
- 코드 성능이나 안전성을 고려하여
null값의 처리 방법을 고려해야 합니다. TrafficDetail생성을 담당하는 클래스와 관련된 테스트 케이스를 작성하는 것이 좋습니다.- API 요청 또는 기타 연동 시 발생 가능한 에러 처리에 대한 고려가 필요할 수 있습니다.
- 코드 성능이나 안전성을 고려하여
-
확장성:
- 추후 데이터 구조 변경에 유연하게 대응할 수 있도록 설계되었는지 확인해야 합니다.
- 추가적인 변환 로직이 필요할 때 유지보수 용이성을 고려하여 설계되었는지 확인해야 합니다.
개선사항 및 버그 리스크를 고려하거나 추가적인 예외 처리 등을 고려하면 좋겠습니다.
|
|
||
| return trafficDetailInfo; | ||
| } | ||
| } |
There was a problem hiding this comment.
이 코드는 주로 안전성 측면에서 개선이 필요합니다.
-
에러 처리 개선
JsonMappingException및JsonProcessingException을 처리할 때 구체적인 오류 메시지를 출력하는 것이 유용합니다.- 에러 발생 시, 어떤 입력 데이터가 문제인지 더 자세한 정보를 제공하면 디버깅과 해결이 쉬워집니다.
-
Try-with-resources 문 사용
ObjectMapper는 자원이 있으므로 try-with-resources 문으로 감싸서 자원을 명시적으로 닫도록 합니다.
-
Null 처리
trafficEntity.getDetail()의 반환 값이 null일 경우 예외가 발생할 수 있습니다. 이에 대한 처리도 고려되어야 합니다.
-
테스트
- 해당 코드에 대한 단위 테스트를 작성하여 JSON 파싱이 예상대로 작동하는지 확인하는 것이 좋습니다.
api/src/main/java/com/walking/api/service/TrafficIntegrationPredictService.java
Show resolved
Hide resolved
| public List<TrafficFavoritesEntity> executeByMemberFk(MemberEntity member) { | ||
| return trafficFavoritesRepository.findByMemberFk(member); | ||
| } | ||
| } |
There was a problem hiding this comment.
이 코드 조각은 작고 깔끔합니다. 하지만 몇 가지 주의해야 할 점이 있습니다:
- Null 처리:
member매개변수가 null인 경우에 대한 예외처리가 필요할 수 있습니다. - 에러 처리: 데이터베이스에서 반환되는 예외 처리나 비즈니스 레이어에서 발생하는 에러를 적절히 처리할 수 있도록 수정이 필요할 수 있습니다.
- 코드 주석: 메소드나 클래스 수준에서 간단한 주석을 추가하여 코드를 이해하기 쉽게 해주는 것이 좋습니다.
- 테스트 코드: 단위 테스트 코드를 작성하여 서비스 계층의 기능이 올바르게 동작하는지 확인할 수 있습니다.
- 더 많은 비즈니스 논리 및 예외 처리: 가능한 다양한 시나리오에 대비하기 위해 더 많은 비즈니스 로직 및 예외 처리를 추가할 수 있습니다.
이외에는 코드 품질이 높고 목적에 맞는 구현이라고 할 수 있습니다.
| public List<TrafficEntity> executeByIds(List<Long> trafficIds) { | ||
| return trafficRepository.findByIds(trafficIds); | ||
| } | ||
| } |
There was a problem hiding this comment.
이 코드는 대체로 깔끔하며 효율적으로 보입니다. 그러나 개선을 위해 몇 가지 제안 사항이 있습니다:
-
orElseThrow()대신에 좀 더 구체적인 예외를 throw하는 것이 좋습니다. 아니면 사용자가 이해할 수 있는 메시지를 첨부해주세요. -
executeByIds메서드의 이름이 복수형으로 되어 있지만, 여러 ID에 해당하는 엔티티를 반환하는 대신 단일 ID에 해당하는 엔티티를 반환하고 있습니다. 메서드 이름을executeById로 변경하는 것을 고려해보세요. -
Null 처리를 고려해 주세요.
trafficRepository가 null이 될 가능성이 없다는 확신이 없다면 null 체크하는 코드 추가 추천드립니다. -
각 메서드에 대한 Javadoc 주석을 작성하여, 메서드의 입력과 출력을 명확히 문서화해주세요.
-
의존성 주입에 대한 테스트 코드 작성을 고려해주세요.
여기까지가 간단한 코드 리뷰입니다. 부분적인 응용 및 수정은 필요에 따라 적용하시면 좋을 것입니다.
| .greenCycle(30L) | ||
| .timeLeft(10f) | ||
| .redCycle(30f) | ||
| .greenCycle(30f) | ||
| .build())) | ||
| .paths( | ||
| List.of( |
There was a problem hiding this comment.
이 코드 조각의 변경 내용을 간략히 검토하겠습니다:
버그 위험:
- 'remainTime', 'redCycle', 'greenCycle' 속성을 'Long'에서 'Float'으로 변경함으로써, 이전 값과 변수 형식이 변경되었습니다. 기존 사용 사례 및 함수와의 호환성을 확인해야 합니다.
개선 제안:
- 주석이나 문서에 실제 시간 단위를 명시하는 것이 좋을 수 있습니다. 예를 들어, 'timeLeft'이 분 또는 초인지 확인해야 합니다.
- 상수나 매직 넘버 대신에 이름 지어진 상수를 사용하여 magic number를 없애는 것이 좋습니다.
- 코드 이상을 방지하기 위해 단위 테스트가 추가될 수 있습니다.
부가적 의견:
한국어로 작성된 부분들은 한글 코드 리뷰를 제공하기 위해 효과적으로 사용되었습니다. 코드 리뷰 중 언어를 변경할 경우에도 해당 언어의 컨텍스트를 고려하여 평가할 필요가 있습니다.
| .greenCycle(30L) | ||
| .timeLeft(10f) | ||
| .redCycle(30f) | ||
| .greenCycle(30f) | ||
| .build()) | ||
| .build(); | ||
|
|
There was a problem hiding this comment.
이 코드는 다음과 같은 개선 사항 및 버그 위험에 직면할 수 있습니다:
IntegrationPredictResponseDto에서 예측 데이터를 가져올 때 Null 여부를 확인하는 추가적인 안전장치가 없으므로, Null 포인터 예외의 위험이 있습니다.- 정적 변수들(
TF_BACK_DOOR_LAT,TF_BACK_DOOR_LNG등)을 클래스 수준에서 선언해야 할 수도 있고, 값이 변하지 않을 경우final키워드 또는 상수로 처리하는 것이 좋습니다. - 예측된 데이터 매핑 과정에서 오류 발생 시의 예외 처리 로직이 누락되어 있습니다.
- 일부 메서드의 이름이 오타일 가능성이 있습니다(
searchTraffi->searchTraffic). 메서드명 일관성이 중요합니다. - 시간 관련 값인데 소수로 표현된
timeLeft,redCycle,greenCycle등에 대한 유효성 검사 및 단위 통일이 필요합니다.
미리 감사합니다.
| private Long redCycle; | ||
| private Float timeLeft; | ||
| private Float greenCycle; | ||
| private Float redCycle; | ||
| private PointDetail point; | ||
| private String color; | ||
| private TrafficDetailInfo detail; |
There was a problem hiding this comment.
코드 리뷰:
- 'remainTime' 변수를 Float 타입인 'timeLeft'로 변경했습니다. 일관성있게 데이터형을 변경해 좋은 선택입니다.
- 'greenCycle' 및 'redCycle' 변수를 Float 형으로 변경했습니다. 이것은 정수형이 아닌 소수점 값을 처리할 때 더 적합합니다.
- 주석은 보이지 않습니다. 코드에 주석 추가가 필요할 수 있습니다.
- 'TrafficDetailInfo' 클래스에 대한 완전한 이해가 없으므로, 해당 부분의 사용 확인이 필요할 수 있습니다.
- 요구 사항에 따라 Null 값 처리가 중요할 수 있습니다. 현재 코드는 Null 값 처리에 대한 로직이 없어 보입니다.
| @@ -246,7 +246,7 @@ void browseTraffic() throws Exception { | |||
| new ParameterDescriptorWithType("trafficId") | |||
| .type(SimpleType.NUMBER) | |||
| .description("신호등 ID") | |||
| .defaultValue(1L)) | |||
| .defaultValue(3L)) | |||
| .responseSchema(Schema.schema("BrowseTrafficResponse")) | |||
| .responseFields( | |||
| Description.common( | |||
There was a problem hiding this comment.
후기:
- URL 경로에서
trafficId를 3으로 설정하는 것은 기존의 1에서 변경한 것으로 보입니다. 주석이나 JIRA 티켓 등을 통해 왜 이 변경이 필요한지 명확하게 문서화하는 것이 도움이 될 수 있습니다. defaultValue()를 3으로 수정함에 따라 해당 API가 어떤 신호등을 조회하는 것인지 팀 내에서 충분히 검토되었는지 확인하는 것이 중요합니다.- 코드 변경 후 테스트 케이스도 업데이트가 필요할 수 있습니다.
- 이외에는 주요한 거버넌스 또는 설계적인 문제가 보이지 않습니다.
<description> gpt 리뷰 반영
|
|
||
| return trafficDetailInfo; | ||
| } | ||
| } |
There was a problem hiding this comment.
이 코드 페치는 JSON 데이터를 파싱하여 TrafficDetailInfo 객체로 변환하는 역할을 합니다. 코드 리뷰 및 개선 제안은 다음과 같습니다:
-
개선 사항:
- ObjectMapper 인스턴스를 메서드 호출 시마다 생성하는 것은 비용이 소모될 수 있습니다. 따라서 ObjectMapper를 전역으로 선언하고 재사용할 수 있도록 하면 효율적일 수 있습니다.
- JSON 데이터 파싱에 실패한 경우 예외가 발생하며 그대로 런타임 예외로 전환되어 처리됩니다. 이것은 적절한 처리 방법이 될 수 있지만, 상황에 따라 실패를 허용하고 기본값으로 대체하는 방식 등 유연한 처리도 고려할 수 있습니다.
-
오류 위험:
- trafficEntity.getDetail()에서 반환된 JSON 데이터의 형식이 변경되면서 TrafficDetailInfo 클래스와 매핑이 실패할 가능성이 있습니다. 이러한 경우 예외 처리를 추가로 강화하는 것이 좋을 수 있습니다.
-
추가 개선 사항:
- Javadoc 주석을 더 자세히 작성하여 해당 메소드의 목적과 사용 방법을 명확하게 설명할 수 있습니다.
- Java 8 이상을 사용하는 경우 Builder 패턴 대신 @JsonCreator 및 @JsonProperty를 사용하여 디시리얼라이즈 할 수 있습니다. Builder 패턴보다는 가독성을 높일 수 있습니다.
위 개선점을 참고하여 코드를 보완하거나 수정하실지 고려해보세요.
즐겨찾기 여부에 따른 응답 데이터의 차이를 고려하지 않은 구현으로 일단 제작
이번주(24/05/26(일))에 이야기 해보겠습니다