-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat: 버스 교통편 조회 API #1099
base: develop
Are you sure you want to change the base?
feat: 버스 교통편 조회 API #1099
Conversation
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 상세히 작성해주셔서 리뷰하기 편했습니다!
고생많이하셨어요~
src/main/java/in/koreatech/koin/domain/bus/model/express/ExpressBusSchedule.java
Outdated
Show resolved
Hide resolved
private static final Integer ADDITIONAL_TIME_DEPART_TO_KOREATECH_400 = 6; | ||
private static final Integer ADDITIONAL_TIME_DEPART_TO_KOREATECH_402 = 13; | ||
private static final Integer ADDITIONAL_TIME_DEPART_TO_KOREATECH_405 = 7; | ||
private static final Integer ADDITIONAL_TIME_DEPART_TO_STATION = 7; |
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.
A
기점에서 정류장까지 걸리는 시간(분)인가요?
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.
맞습니다! 시내버스 API 에서 받아오는 출발 시간 데이터는 기점 출발 기준입니다. 예를 들어 400번의 경우 한기대 정류장이 아니라 출발지인 병천3리 정류장을 기준으로 스케줄 정보를 제공합니다. 따라서 사용자가 한기대 -> 터미널의 스케줄을 요청한 경우 시간 데이터에 병천3리 -> 한기대 사이의 이동 시간을 추가하여 시간을 보정합니다. 해당 작업을 위해 정의한 상수입니다.
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.
A
해당 정보의 출처는 어디인지, 이 상수들의 용도가 무엇인지 주석으로 명시하면 좋을 것 같아요!
src/main/java/in/koreatech/koin/domain/bus/service/route/ShuttleBusRouteStrategy.java
Outdated
Show resolved
Hide resolved
src/main/java/in/koreatech/koin/domain/bus/service/BusService.java
Outdated
Show resolved
Hide resolved
src/main/java/in/koreatech/koin/domain/bus/service/route/BusRouteStrategy.java
Outdated
Show resolved
Hide resolved
if (!foundDepart && node.getNodeName().contains(departNode.getQueryName())) { | ||
foundDepart = true; | ||
} | ||
|
||
else if (foundDepart && node.getNodeName().contains(arriveNode.getQueryName())) { | ||
return true; | ||
} |
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.
C
출발지를 찾는 로직과 도착지를 찾는 로직을 else로 엮지 않고 분리하면 조금 더 잘 읽힐 것 같습니다!
if (!foundDepart && node.getNodeName().contains(departNode.getQueryName())) {
foundDepart = true;
}
if (foundDepart && node.getNodeName().contains(arriveNode.getQueryName())) {
return true;
}
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.
else-if 블록이 실행되는 경우를 살펴보려면, 상위 if문의 조건과, else-if문의 조건을 모두 만족해야하기 때문에 조건을 두번 확인해야하는 수요가 생깁니다.
if 문을 한번만 사용한다면 분기를 한번만 확인하기 때문에 가독성이 높아진다고 생각합니다. 어떻게 생각하시나요?
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.
리뷰 늦게남겨서 죄송합니다 😭
고생하셨습니다! 코멘트 확인 부탁드려요~
### 버스 교통편 조회 | ||
- **시간** : 00:00 인 경우 해당 날짜의 모든 스케줄을 조회합니다. | ||
- **날짜** : 요일을 기준으로 스케줄을 출력합니다. 공휴일 처리는 구현되어 있지 않습니다. | ||
- **출발지 & 도착지** : 출발지와 도착지가 일치하는 경우 빈 리스트를 반환합니다. (천안역 -> 터미널) & (터미널 -> 천안역) 역시 빈 리스트를 반환합니다. |
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.
R
터미널, 천안역 간 교통편 조회는 빈 결과를 반환한다는 요구사항이 있었나요??
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.
저희 데이터 중 터미널-천안역 간 정확한 스케줄 정보를 제공할 수 있는 데이터가 없어서 빈 결과를 반환하게 했습니다. 따로 제시된 요구사항은 없습니다. 천안역 -> 천안역이 빈 리스트를 반환하는 것과 같은 맥락으로 생각했습니다. PM님과 상의해야하는 부분일까요?
@Parameter(description = "yyyy-MM-dd") @RequestParam @DateTimeFormat(pattern = "yyyy-MM-dd") LocalDate date, | ||
@Parameter(description = "HH:mm") @RequestParam String time, |
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.
C
request와 response에서 모두 date와 time을 분리하여 전달하고 있는데, 분리해야 했던 이유가 있을까요?? 다른 곳에서는 대부분은 합쳐서 사용하고 있지 않나요??
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.
디자인에 '오늘 오후 10:30 출발' 이런식으로 출력하도록 되어 있길래 분리 하는 게 클라이언트 입장에서 더 편할 것 같다고 생각했습니다. 버스 도메인 다른 API(버스 검색)도 저런 형태로 정보를 받고 있어서 그대로 진행했습니다.
@Parameter( | ||
description = "CITY, EXPRESS, SHUTTLE, ALL" | ||
) @RequestParam BusRouteType busRouteType, |
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.
A
파라미터 타입으로 enum을 주면 swagger에서 조회 시 드롭다운으로 나타날텐데 description에 예시를 제공해야 할까요??
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.
C
request dto 용도의 클래스를 만들어야 한다면 차라리 GET요청이지만 request body를 사용하는 건 어떻게 생각하시나요?? 기능에 비해 복잡도가 커지는 것 같아서 우려되네요
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.
BusRouteCommand
를 request dto처럼 만드는게 어떻냐는 말씀이신가요? command 객체가 BusRouteStrategy
추상화 메소드의 매개변수 타입으로 사용되고 있는데 이 객체를 request dto처럼 만들면 더 복잡해 질 것 같습니다.
@JsonNaming(SnakeCaseStrategy.class) | ||
public record ScheduleInfo( | ||
String busType, | ||
String busName, | ||
LocalTime departTime | ||
) { |
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.
A
여기도 @Schema
로 부연설명을 붙일 수 있을 것 같아요. 그렇게 한다면 schedule 변수의 Schema 예시 설명을 제거할 수 있을 것 같습니다.
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.
A
주석으로 부연설명을 조금 달아두는 것도 좋을 것 같아요!
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 static final Integer ADDITIONAL_TIME_DEPART_TO_KOREATECH_400 = 6; | ||
private static final Integer ADDITIONAL_TIME_DEPART_TO_KOREATECH_402 = 13; | ||
private static final Integer ADDITIONAL_TIME_DEPART_TO_KOREATECH_405 = 7; | ||
private static final Integer ADDITIONAL_TIME_DEPART_TO_STATION = 7; |
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.
A
해당 정보의 출처는 어디인지, 이 상수들의 용도가 무엇인지 주석으로 명시하면 좋을 것 같아요!
} | ||
|
||
public ScheduleInfo getCommutingShuttleBusScheduleInfo(BusStation depart) { | ||
String busType = "한기대".equals(depart.getQueryName()) ? "하교셔틀" : "등교셔틀"; |
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.
A
출발지가 한기대가 아닌데 하교셔틀이면 어떡하나요??
ex) 터미널 -> 천안역
+) 크게 치명적인 내용은 아닌 것 같습니다
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.
위 터미널 <-> 천안역 사이 빈 결과를 반환 부분과 연결되는 내용 같아요.. 셔틀버스를 터미널에서 타서 천안역에서 내리는 경우는 없다고 생각하고 배제해서 로직을 작성했습니다...
🔥 연관 이슈
🚀 작업 내용
설계
DataSource
셔틀버스 스케줄 조회 상세 구현 로직
RequestBody
"2024-11-05"
)."00:00"
).ALL, CITY, EXPRESS, SHUTTLE
)KOREATECH, TERMINAL, STATION
).KOREATECH, TERMINAL, STATION
).ResponseBody
💬 리뷰 중점사항