-
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 #1119
feat: 학교버스 시간표 관련 API #1119
Conversation
/courses/shuttle 학교버스 노선 조회 API 구현 /timetable/shuttle 학교버스 시간표 조회 API 구현
모델 객체와 DTO 분리 컨트롤러 메서드 이름 변경
정규학기, 계절학기, 방학에 따라 다른 노선 반환
전체 노선 조회 response 구조 변경
f43cb64
to
c835f5f
Compare
지역 순서대로 반환 (천안, 서울, 청주) 노선 종류 순서대로 반환 (순환, 주중, 주말)
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.
늦어서 죄송합니다!!
@@ -29,6 +34,8 @@ | |||
public class BusController implements BusApi { | |||
|
|||
private final BusService busService; | |||
private final ShuttleBusRepository shuttleBusRepository; |
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.
db로부터 받은 데이터를 단순히 response dto에 매핑시키는 간단한 로직이라 service 단 거치지 않는 구조로 만드신 것 같습니다. 이 과정에서 contoller에 repository 의존성이 추가되었는데 확장성이나 유지보수 고려하면 간단하더라도 service단 한번 거치는게 좋지 않을까요?
@Operation(summary = "학교버스 시간표 조회") | ||
@GetMapping("/timetable/shuttle") | ||
ResponseEntity<ShuttleBusRouteResponse> getShuttleBusTimetable(@RequestParam String id); |
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.
id에 무슨 내용이 들어와야 하는지 @Opreation(description = "")
로 적어주시면 api 문서 안봐도 되서 편할 것 같아요
public static List<RouteRegion> mapCategories(List<ShuttleBusRoute> shuttleBusRoutes) { | ||
return shuttleBusRoutes.stream() | ||
.collect(Collectors.groupingBy(ShuttleBusRoute::getRegion)) | ||
.entrySet().stream() | ||
.map(entry -> new RouteRegion(entry.getKey(), RouteName.mapRouteNames(entry.getValue()))) | ||
.sorted() | ||
.toList(); | ||
} | ||
} |
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.
데이터 전송 객체가 너무 많은 책임을 가지고 있는 것 같아요. service 사용하지 않은 이유가 궁금해요.
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.
Service
에서 특정 DTO
에 의존하게 되면 여러 종류의 Controller
에서 해당 Service
를 이용할 수 없어 코드 재사용성이 떨어지게 된다라고 생각해서 변환의 책임을 DTO
에 두었습니다.
그런데 다시 생각해보니 말씀하신대로 데이터 전송 객체에 무거운 책임을 두는 것은 좋지 않은 생각인 것 같아요. service
사용하도록 수정하겠습니다. 👍
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.
굉장히 늦은 리뷰 죄송합니다 😭
고생하셨습니다!
@GetMapping("/courses/shuttle") | ||
public ResponseEntity<ShuttleBusRoutesResponse> getShuttleBusRoutes() { | ||
VersionMessageResponse version = versionService.getVersionWithMessage("shuttle_bus_timetable"); | ||
return ResponseEntity.ok() | ||
.body(ShuttleBusRoutesResponse.of(shuttleBusRepository.findBySemesterType(version.title()), version)); | ||
} | ||
|
||
@GetMapping("/timetable/shuttle") | ||
public ResponseEntity<ShuttleBusRouteResponse> getShuttleBusTimetable(@RequestParam String id) { | ||
return ResponseEntity.ok().body(ShuttleBusRouteResponse.from(shuttleBusRepository.getById(id))); | ||
} |
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
controller에서 repository에 직접 접근하고 있군요..!
service를 반드시 껴야하는 건 아니긴 한데, 괜찮은 구조라고 생각하시는지 궁금해요
저도 잘 확신이 안서네요..
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.
controller 입장에서 계층적으로 service는 가까운 친구이지만, repository는 멀리있는 친구이기 때문에 service만 알고 있는 것도 좋은 방법 같습니다. (repository에 직접 참조한다면 당장은 편하겠지만..)
우선 service에 의존하도록 하고 bus 리팩토링 기간 조금 더 고민해보도록 하겠습니다!
@Schema(description = "지역 이름", example = "천안") | ||
String region, |
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을 적용하는 건 어떻게 생각하시나요??
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.
enum 사용 시, 정렬할 때 우선순위 값으로 활용할 수 있고 확장성이 향상되는 등 객체지향 기법에 적절하다고 생각해서 말씀하신대로 수정해보겠습니다👍
import in.koreatech.koin.domain.bus.exception.BusNotFoundException; | ||
import in.koreatech.koin.domain.bus.model.mongo.ShuttleBusRoute; | ||
|
||
public interface ShuttleBusRepository extends Repository<ShuttleBusRoute, ObjectId> { |
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
ObjectId는 무슨 자료형인가요??
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.
ObjectId
: mongodb 방식에 특화되어 기본적으로 제공하는 id
타입으로 알고있습니다.
spring data mongodb에서 자바와 맵핑 가능하도록 제공하는 것으로 보입니다.
jpa에서 string
이 아닌, 해당 타입으로 설정해야 id
로 검색이 가능해서 설정해두었습니다.
uri 수정: /bus/timetable/shuttle/{id} ShuttleBusService 구현 dto 책임 분리 dto 이름 변경
지역, 노선종류 속성 enum 대체 정렬 로직 개선
노선 정보에 detail 속성 추가 노선 지역 response 이름 수정
# Conflicts: # src/main/java/in/koreatech/koin/domain/bus/controller/BusApi.java # src/main/java/in/koreatech/koin/domain/bus/controller/BusController.java
🔥 연관 이슈
새롭게 개선되는 학교버스 시간표 기능에서 다음과 같은 변경점이 존재합니다.
위와 같은 변경점들로 인해 기존의 mongodb 컬렉션(
bus_timetables
)을 그대로 사용하기에 많은 어려움이 있습니다.따라서 변경사항에 적합한 데이터 구조를 가진 컬렉션(
shuttlebus_timetables
)을 추가하려합니다.예상되는 문제점들과 솔루션은 다음과 같습니다.
shuttlebus_timetables
컬렉션에 "정규학기, 계절학기, 방학" 데이터를 미리 삽입bus_timetables
,shuttlebus_timetables
) -> 이상현상 발생 가능성 존재🚀 작업 내용
shuttlebus_timetables
스키마 예시 천안셔틀💬 리뷰 중점사항