Skip to content
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

GET: /api/v1/traffics/{trafficId} 컨트롤러 구현 #15

Merged
merged 14 commits into from
May 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
package com.walking.api.converter;

import com.walking.api.service.dto.PredictedData;
import com.walking.api.web.dto.response.detail.PointDetail;
import com.walking.api.web.dto.response.detail.TrafficDetail;
import com.walking.data.entity.traffic.TrafficEntity;

public final class TrafficDetailConverter {

private TrafficDetailConverter() {}

/**
* PredictedData를 기반으로 TrafficDetail를 생성합니다.
*
* @param predictedData 사이클 정보 와 현재 색상 및 잔여시간을 예측한 데이터
* @return 예측 값을 바탕으로 만든 TrafficDetail
*/
public static TrafficDetail execute(PredictedData predictedData) {

TrafficEntity trafficEntity = predictedData.getTraffic();

return TrafficDetail.builder()
.id(trafficEntity.getId())
.color(predictedData.getCurrentColor().toString())
.timeLeft(predictedData.getCurrentTimeLeft())
.point(
PointDetail.builder().lng(trafficEntity.getLng()).lat(trafficEntity.getLat()).build())
.redCycle(predictedData.getRedCycle())
.greenCycle(predictedData.getGreenCycle())
.detail(TrafficDetailInfoConverter.execute(trafficEntity))
.isFavorite(false)
.viewName(trafficEntity.getName())
.build();
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

코드를 살펴보니 다음과 같은 개선점과 버그 위험이 있을 수 있습니다:

  1. TrafficDetailConverter 클래스:

    • TrafficEntity에서 반환된 ID는 null일 수 있으므로 id(trafficEntity.getId()) 부분에서 null 체크가 필요합니다.
    • 음수 주기가 올바른 값인지 확인하고 해당 사항에 대한 검증 로직을 추가해야 합니다.
    • 예측된 데이터가 null인 경우의 처리 로직이 필요할 수 있습니다.
  2. 코드 품질 및 가독성:

    • Javadoc 주석이 잘 작성되어 있습니다.
    • 변수 및 메소드 이름이 명확하며 깔끔합니다.
    • 추상화 수준이 적합하며 불필요한 복잡성이 없습니다.
  3. 일반적인 개선사항:

    • 코드 성능이나 안전성을 고려하여 null 값의 처리 방법을 고려해야 합니다.
    • TrafficDetail 생성을 담당하는 클래스와 관련된 테스트 케이스를 작성하는 것이 좋습니다.
    • API 요청 또는 기타 연동 시 발생 가능한 에러 처리에 대한 고려가 필요할 수 있습니다.
  4. 확장성:

    • 추후 데이터 구조 변경에 유연하게 대응할 수 있도록 설계되었는지 확인해야 합니다.
    • 추가적인 변환 로직이 필요할 때 유지보수 용이성을 고려하여 설계되었는지 확인해야 합니다.

개선사항 및 버그 리스크를 고려하거나 추가적인 예외 처리 등을 고려하면 좋겠습니다.

Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
package com.walking.api.converter;

import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.JsonMappingException;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.walking.api.web.dto.response.detail.TrafficDetailInfo;
import com.walking.data.entity.traffic.TrafficEntity;

public final class TrafficDetailInfoConverter {

private TrafficDetailInfoConverter() {}

/**
* api.traffic_detail 의 detail 값(JSON)을 파싱하여 TrafficDetailInfo 로 변환합니다.
*
* @param trafficEntity
* @return
*/
public static TrafficDetailInfo execute(TrafficEntity trafficEntity) {
ObjectMapper objectMapper = new ObjectMapper();
TrafficDetailInfo trafficDetailInfo =
TrafficDetailInfo.builder().trafficId(-1L).apiSource("ERROR").direction("ERROR").build();
try {
trafficDetailInfo =
objectMapper.readValue(trafficEntity.getDetail(), TrafficDetailInfo.class);
} catch (JsonMappingException e) {
throw new RuntimeException("Convert to TrafficDetailInfo fail", e);
} catch (JsonProcessingException e) {
throw new RuntimeException("Convert to TrafficDetailInfo fail", e);
}

return trafficDetailInfo;
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 코드는 주로 안전성 측면에서 개선이 필요합니다.

  1. 에러 처리 개선

    • JsonMappingExceptionJsonProcessingException을 처리할 때 구체적인 오류 메시지를 출력하는 것이 유용합니다.
    • 에러 발생 시, 어떤 입력 데이터가 문제인지 더 자세한 정보를 제공하면 디버깅과 해결이 쉬워집니다.
  2. Try-with-resources 문 사용

    • ObjectMapper는 자원이 있으므로 try-with-resources 문으로 감싸서 자원을 명시적으로 닫도록 합니다.
  3. Null 처리

    • trafficEntity.getDetail()의 반환 값이 null일 경우 예외가 발생할 수 있습니다. 이에 대한 처리도 고려되어야 합니다.
  4. 테스트

    • 해당 코드에 대한 단위 테스트를 작성하여 JSON 파싱이 예상대로 작동하는지 확인하는 것이 좋습니다.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 코드 페치는 JSON 데이터를 파싱하여 TrafficDetailInfo 객체로 변환하는 역할을 합니다. 코드 리뷰 및 개선 제안은 다음과 같습니다:

  1. 개선 사항:

    • ObjectMapper 인스턴스를 메서드 호출 시마다 생성하는 것은 비용이 소모될 수 있습니다. 따라서 ObjectMapper를 전역으로 선언하고 재사용할 수 있도록 하면 효율적일 수 있습니다.
    • JSON 데이터 파싱에 실패한 경우 예외가 발생하며 그대로 런타임 예외로 전환되어 처리됩니다. 이것은 적절한 처리 방법이 될 수 있지만, 상황에 따라 실패를 허용하고 기본값으로 대체하는 방식 등 유연한 처리도 고려할 수 있습니다.
  2. 오류 위험:

    • trafficEntity.getDetail()에서 반환된 JSON 데이터의 형식이 변경되면서 TrafficDetailInfo 클래스와 매핑이 실패할 가능성이 있습니다. 이러한 경우 예외 처리를 추가로 강화하는 것이 좋을 수 있습니다.
  3. 추가 개선 사항:

    • Javadoc 주석을 더 자세히 작성하여 해당 메소드의 목적과 사용 방법을 명확하게 설명할 수 있습니다.
    • Java 8 이상을 사용하는 경우 Builder 패턴 대신 @JsonCreator 및 @JsonProperty를 사용하여 디시리얼라이즈 할 수 있습니다. Builder 패턴보다는 가독성을 높일 수 있습니다.

위 개선점을 참고하여 코드를 보완하거나 수정하실지 고려해보세요.

Original file line number Diff line number Diff line change
@@ -1,8 +1,13 @@
package com.walking.api.repository.member;
package com.walking.api.repository.traffic;

import com.walking.data.entity.member.MemberEntity;
import com.walking.data.entity.member.TrafficFavoritesEntity;
import java.util.List;
import org.springframework.data.jpa.repository.JpaRepository;
import org.springframework.stereotype.Repository;

@Repository
public interface TrafficFavoritesRepository extends JpaRepository<TrafficFavoritesEntity, Long> {}
public interface TrafficFavoritesRepository extends JpaRepository<TrafficFavoritesEntity, Long> {

List<TrafficFavoritesEntity> findByMemberFk(MemberEntity memberFk);
}
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import lombok.extern.slf4j.Slf4j;
import org.springframework.beans.factory.annotation.Value;
import org.springframework.stereotype.Service;
import org.springframework.transaction.annotation.Transactional;

/** 신호등의 현재 잔여 시간, 현재 색상, 각 색상별 사이클을 예측한 결과를 리턴합니다. */
@Service
Expand All @@ -25,6 +26,7 @@ public class TrafficIntegrationPredictService {
@Value("${walking.predict.dataInterval}")
private int dataInterval;

@Transactional(readOnly = true)
public IntegrationPredictResponseDto execute(IntegrationPredictRequestDto requestDto) {
List<Long> trafficIds = requestDto.getTrafficIds();
CyclePredictionRequestDto cyclePredictionRequestDto =
nove1080 marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,12 @@ public boolean isAllPredicted() {
&& currentTimeLeft > 0;
}

/**
* 색상에 따른 사이클을 반환합니다.
*
* @param color 사이클을 알고자 하는 신호등의 색상
* @return 파라미터로 전달받은 색상의 사이클
*/
public Float getCycleByColor(TrafficColor color) {
if (color.isGreen()) {
return greenCycle;
nove1080 marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
package com.walking.api.service.traffic;

import com.walking.api.repository.traffic.TrafficFavoritesRepository;
import com.walking.data.entity.member.MemberEntity;
import com.walking.data.entity.member.TrafficFavoritesEntity;
import java.util.List;
import lombok.RequiredArgsConstructor;
import org.springframework.stereotype.Service;

@Service
@RequiredArgsConstructor
public class ReadTrafficFavoritesService {

private final TrafficFavoritesRepository trafficFavoritesRepository;

public List<TrafficFavoritesEntity> executeByMemberFk(MemberEntity member) {
return trafficFavoritesRepository.findByMemberFk(member);
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 코드 조각은 작고 깔끔합니다. 하지만 몇 가지 주의해야 할 점이 있습니다:

  1. Null 처리: member 매개변수가 null인 경우에 대한 예외처리가 필요할 수 있습니다.
  2. 에러 처리: 데이터베이스에서 반환되는 예외 처리나 비즈니스 레이어에서 발생하는 에러를 적절히 처리할 수 있도록 수정이 필요할 수 있습니다.
  3. 코드 주석: 메소드나 클래스 수준에서 간단한 주석을 추가하여 코드를 이해하기 쉽게 해주는 것이 좋습니다.
  4. 테스트 코드: 단위 테스트 코드를 작성하여 서비스 계층의 기능이 올바르게 동작하는지 확인할 수 있습니다.
  5. 더 많은 비즈니스 논리 및 예외 처리: 가능한 다양한 시나리오에 대비하기 위해 더 많은 비즈니스 로직 및 예외 처리를 추가할 수 있습니다.

이외에는 코드 품질이 높고 목적에 맞는 구현이라고 할 수 있습니다.

Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
package com.walking.api.service.traffic;

import com.walking.api.repository.traffic.TrafficRepository;
import com.walking.data.entity.traffic.TrafficEntity;
import java.util.List;
import lombok.RequiredArgsConstructor;
import org.springframework.stereotype.Service;

@Service
@RequiredArgsConstructor
public class ReadTrafficService {

private final TrafficRepository trafficRepository;

public TrafficEntity executeById(Long trafficId) {
return trafficRepository.findById(trafficId).orElseThrow();
}

public List<TrafficEntity> executeByIds(List<Long> trafficIds) {
return trafficRepository.findByIds(trafficIds);
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 코드는 대체로 깔끔하며 효율적으로 보입니다. 그러나 개선을 위해 몇 가지 제안 사항이 있습니다:

  1. orElseThrow() 대신에 좀 더 구체적인 예외를 throw하는 것이 좋습니다. 아니면 사용자가 이해할 수 있는 메시지를 첨부해주세요.

  2. executeByIds 메서드의 이름이 복수형으로 되어 있지만, 여러 ID에 해당하는 엔티티를 반환하는 대신 단일 ID에 해당하는 엔티티를 반환하고 있습니다. 메서드 이름을 executeById로 변경하는 것을 고려해보세요.

  3. Null 처리를 고려해 주세요. trafficRepository가 null이 될 가능성이 없다는 확신이 없다면 null 체크하는 코드 추가 추천드립니다.

  4. 각 메서드에 대한 Javadoc 주석을 작성하여, 메서드의 입력과 출력을 명확히 문서화해주세요.

  5. 의존성 주입에 대한 테스트 코드 작성을 고려해주세요.

여기까지가 간단한 코드 리뷰입니다. 부분적인 응용 및 수정은 필요에 따라 적용하시면 좋을 것입니다.

Original file line number Diff line number Diff line change
Expand Up @@ -146,9 +146,9 @@ private static RouteDetailResponse getSampleRouteDetailResponse() {
.viewName("후문")
.point(PointDetail.builder().lat(BACK_DOOR_LAT).lng(BACK_DOOR_LNG).build())
.color("red")
.remainTime(10L)
.redCycle(30L)
.greenCycle(30L)
.timeLeft(10f)
.redCycle(30f)
.greenCycle(30f)
.build()))
.paths(
List.of(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 코드 조각의 변경 내용을 간략히 검토하겠습니다:

버그 위험:

  1. 'remainTime', 'redCycle', 'greenCycle' 속성을 'Long'에서 'Float'으로 변경함으로써, 이전 값과 변수 형식이 변경되었습니다. 기존 사용 사례 및 함수와의 호환성을 확인해야 합니다.

개선 제안:

  1. 주석이나 문서에 실제 시간 단위를 명시하는 것이 좋을 수 있습니다. 예를 들어, 'timeLeft'이 분 또는 초인지 확인해야 합니다.
  2. 상수나 매직 넘버 대신에 이름 지어진 상수를 사용하여 magic number를 없애는 것이 좋습니다.
  3. 코드 이상을 방지하기 위해 단위 테스트가 추가될 수 있습니다.

부가적 의견:
한국어로 작성된 부분들은 한글 코드 리뷰를 제공하기 위해 효과적으로 사용되었습니다. 코드 리뷰 중 언어를 변경할 경우에도 해당 언어의 컨텍스트를 고려하여 평가할 필요가 있습니다.

Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
package com.walking.api.web.controller.traffic;

import com.walking.api.converter.TrafficDetailConverter;
import com.walking.api.security.authentication.token.TokenUserDetails;
import com.walking.api.service.TrafficIntegrationPredictService;
import com.walking.api.service.dto.PredictedData;
import com.walking.api.service.dto.request.IntegrationPredictRequestDto;
import com.walking.api.service.dto.response.IntegrationPredictResponseDto;
import com.walking.api.web.dto.request.point.OptionalTrafficPointParam;
import com.walking.api.web.dto.request.point.OptionalViewPointParam;
import com.walking.api.web.dto.request.traffic.FavoriteTrafficBody;
Expand All @@ -17,13 +22,15 @@
import com.walking.api.web.support.MessageCode;
import java.time.LocalDateTime;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import javax.validation.Valid;
import javax.validation.constraints.Min;
import lombok.RequiredArgsConstructor;
import lombok.extern.slf4j.Slf4j;
import org.springframework.http.HttpStatus;
import org.springframework.security.core.annotation.AuthenticationPrincipal;
import org.springframework.transaction.annotation.Transactional;
import org.springframework.validation.annotation.Validated;
import org.springframework.web.bind.annotation.DeleteMapping;
import org.springframework.web.bind.annotation.GetMapping;
Expand All @@ -41,6 +48,8 @@
@RequiredArgsConstructor
public class TrafficController {

private final TrafficIntegrationPredictService integrationPredictService;

static double TF_BACK_DOOR_LAT = 35.178501;
static double TF_BACK_DOOR_LNG = 126.912083;
static double TF_BACK_THREE_LAT = 35.175841;
Expand Down Expand Up @@ -76,11 +85,20 @@ public ApiResponse<ApiResponse.SuccessBody<SearchTrafficsResponse>> searchTraffi
}

@GetMapping("/{trafficId}")
@Transactional
public ApiResponse<ApiResponse.SuccessBody<BrowseTrafficsResponse>> browseTraffic(
@PathVariable Long trafficId) {
// todo implement
log.info("Traffic browse trafficId: {}", trafficId);
BrowseTrafficsResponse response = getBrowseTrafficsResponse();
IntegrationPredictResponseDto integrationPredictedResponse =
integrationPredictService.execute(
IntegrationPredictRequestDto.builder().trafficIds(List.of(trafficId)).build());

Map<Long, PredictedData> predictedDataMap = integrationPredictedResponse.getPredictedDataMap();
PredictedData predictedData = predictedDataMap.get(trafficId);
TrafficDetail trafficDetail = TrafficDetailConverter.execute(predictedData);
BrowseTrafficsResponse response =
BrowseTrafficsResponse.builder().traffic(trafficDetail).build();
return ApiResponseGenerator.success(response, HttpStatus.OK, MessageCode.SUCCESS);
}

Expand Down Expand Up @@ -151,9 +169,9 @@ private static SearchTrafficsResponse getSearchViewTrafficsResponse() {
.lng(TF_BACK_DOOR_LNG)
.build())
.color("red")
.remainTime(10L)
.redCycle(30L)
.greenCycle(30L)
.timeLeft(10f)
.redCycle(30f)
.greenCycle(30f)
.build(),
TrafficDetail.builder()
.id(2L)
Expand All @@ -171,9 +189,9 @@ private static SearchTrafficsResponse getSearchViewTrafficsResponse() {
.lng(TF_BACK_THREE_LNG)
.build())
.color("green")
.remainTime(20L)
.redCycle(30L)
.greenCycle(30L)
.timeLeft(20f)
.redCycle(30f)
.greenCycle(30f)
.build(),
TrafficDetail.builder()
.id(3L)
Expand All @@ -188,9 +206,9 @@ private static SearchTrafficsResponse getSearchViewTrafficsResponse() {
.point(
PointDetail.builder().lat(TF_CHANPUNG_LAT).lng(TF_CHANPUNG_LNG).build())
.color("red")
.remainTime(10L)
.redCycle(30L)
.greenCycle(30L)
.timeLeft(10f)
.redCycle(30f)
.greenCycle(30f)
.build(),
TrafficDetail.builder()
.id(4L)
Expand All @@ -204,9 +222,9 @@ private static SearchTrafficsResponse getSearchViewTrafficsResponse() {
.viewName("쿠쿠")
.point(PointDetail.builder().lat(TF_CUCU_LAT).lng(TF_CUCU_LNG).build())
.color("green")
.remainTime(20L)
.redCycle(30L)
.greenCycle(30L)
.timeLeft(20f)
.redCycle(30f)
.greenCycle(30f)
.build()))
.build();

Expand Down Expand Up @@ -234,9 +252,9 @@ private static SearchTrafficsResponse getSearchTrafficsResponse() {
.lng(TF_BACK_DOOR_LNG)
.build())
.color("red")
.remainTime(10L)
.redCycle(30L)
.greenCycle(30L)
.timeLeft(10f)
.redCycle(30f)
.greenCycle(30f)
.build()))
.build();

Expand All @@ -260,9 +278,9 @@ private static BrowseTrafficsResponse getBrowseTrafficsResponse() {
.point(
PointDetail.builder().lat(TF_BACK_DOOR_LAT).lng(TF_BACK_DOOR_LNG).build())
.color("red")
.remainTime(10L)
.redCycle(30L)
.greenCycle(30L)
.timeLeft(10f)
.redCycle(30f)
.greenCycle(30f)
.build())
.build();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 코드는 다음과 같은 개선 사항 및 버그 위험에 직면할 수 있습니다:

  1. IntegrationPredictResponseDto에서 예측 데이터를 가져올 때 Null 여부를 확인하는 추가적인 안전장치가 없으므로, Null 포인터 예외의 위험이 있습니다.
  2. 정적 변수들(TF_BACK_DOOR_LAT, TF_BACK_DOOR_LNG 등)을 클래스 수준에서 선언해야 할 수도 있고, 값이 변하지 않을 경우 final 키워드 또는 상수로 처리하는 것이 좋습니다.
  3. 예측된 데이터 매핑 과정에서 오류 발생 시의 예외 처리 로직이 누락되어 있습니다.
  4. 일부 메서드의 이름이 오타일 가능성이 있습니다(searchTraffi -> searchTraffic). 메서드명 일관성이 중요합니다.
  5. 시간 관련 값인데 소수로 표현된 timeLeft, redCycle, greenCycle 등에 대한 유효성 검사 및 단위 통일이 필요합니다.

미리 감사합니다.

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,9 @@
public class TrafficDetail {

private Long id;
private String state;
private Long remainTime;
private Long greenCycle;
private Long redCycle;
private Float timeLeft;
private Float greenCycle;
private Float redCycle;
private PointDetail point;
private String color;
private TrafficDetailInfo detail;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

코드 리뷰:

  1. 'remainTime' 변수를 Float 타입인 'timeLeft'로 변경했습니다. 일관성있게 데이터형을 변경해 좋은 선택입니다.
  2. 'greenCycle' 및 'redCycle' 변수를 Float 형으로 변경했습니다. 이것은 정수형이 아닌 소수점 값을 처리할 때 더 적합합니다.
  3. 주석은 보이지 않습니다. 코드에 주석 추가가 필요할 수 있습니다.
  4. 'TrafficDetailInfo' 클래스에 대한 완전한 이해가 없으므로, 해당 부분의 사용 확인이 필요할 수 있습니다.
  5. 요구 사항에 따라 Null 값 처리가 중요할 수 있습니다. 현재 코드는 Null 값 처리에 대한 로직이 없어 보입니다.

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,7 @@ void searchTrafficsWithTrafficParam() throws Exception {
@DisplayName("GET /{trafficId} 신호등 정보 조회 - 신호등 ID로 조회")
void browseTraffic() throws Exception {
mockMvc
.perform(get(BASE_URL + "/{trafficId}", 1).contentType(MediaType.APPLICATION_JSON))
.perform(get(BASE_URL + "/{trafficId}", 3).contentType(MediaType.APPLICATION_JSON))
.andExpect(status().is2xxSuccessful())
.andDo(
document(
Expand All @@ -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(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

후기:

  1. URL 경로에서 trafficId를 3으로 설정하는 것은 기존의 1에서 변경한 것으로 보입니다. 주석이나 JIRA 티켓 등을 통해 왜 이 변경이 필요한지 명확하게 문서화하는 것이 도움이 될 수 있습니다.
  2. defaultValue()를 3으로 수정함에 따라 해당 API가 어떤 신호등을 조회하는 것인지 팀 내에서 충분히 검토되었는지 확인하는 것이 중요합니다.
  3. 코드 변경 후 테스트 케이스도 업데이트가 필요할 수 있습니다.
  4. 이외에는 주요한 거버넌스 또는 설계적인 문제가 보이지 않습니다.

Expand Down
Loading