[1단계 - 지하철 정보 관리 기능] 무민(박무현) 미션 제출합니다. #94
Conversation
| private List<StationResponseDto> getStationResponseDtosByLineId(final Long lineId) { | ||
| final List<StationEntity> stationEntities = sectionService.findSortedStationEntityByLineId(lineId); | ||
| return StationEntityConverter.convertToStationResponseDto(stationEntities); |
There was a problem hiding this comment.
컨트롤러의 역할이 너무 큰 것 같아요. 무민이 생각하는 컨트롤러와 서비스의 역할은 무엇인가요?
There was a problem hiding this comment.
급하게 짜다 보니 아무렇게나 배치했던 것 같습니다ㅠ..
컨트롤러는 단순히 view와 model 사이의 중개만 해주는 역활이고,
서비스는 비즈니스 로직들을 다루며 데이터들을 가공해 줄 수 있을 것 같습니다.
그런데 여기서 하나 궁금한 것이 있는데
응답으로 dto를 넘겨줄 때 그 dto를 service, controller 중 어디서 만드는게
좋다고 생각하시나요? 터틀의 생각이 궁금합니다!
There was a problem hiding this comment.
서비스의 역할에 대해선 제가 설명 드릴수도 있지만 좀더 찾아보고 고민해보시는게 좋을 것 같아요.
그리고 DTO의 생성은 위치와 연관이 있는데요, DTO의 위치는 애플리케이션의 복잡도에 따라 다양한 전략이 있습니다. (아직 학습을 추천드리지 않는 내용도 상당수라서 너무 깊게는 고민 안하셔도 좋습니다): https://jaime-note.tistory.com/422
DTO를 만드는 위치엔 좋고 나쁜것이 없는 부분입니다. 하지만 저라면 지금 정도의 규모에서 Service 레이어에 위치시킬 것 같고, DTO 생성 역시 Service에서 할것같네요.
|
안녕하세요 터틀~ 피드백 반영하였고 각 기능들 조금씩들(?) 리팩터링하였습니다! 완전 삭제하고 다시 하려했는데 그렇게 되면 변경사항이 너무 많을 것 같아 일단 원래 포맷에서 조금 더 다듬었습니다 궁금한 점이번에 역을 삭제할 때 이름상으로는 역을 삭제하라는거지만 실제로는 노선안의 구간을 끊어내었는데요. 그래서 delete api를 SectionController안에 두었습니다. 근데 api endPoint는 현재 pathVariable을 사용하기 위해 /sections/lines/{lineId}/stations/{stationId} 와 같습니다. 뭔가 너무 길어져 가독성이 떨어지는게 아닌가도 생각이 조금 들기도 하는데 터틀생각은 어떤지 궁금합니다! |
| @PostMapping | ||
| public ResponseEntity<Void> create(@RequestBody LineCreateDto lineCreateDto) { |
There was a problem hiding this comment.
POST 요청 후 반환이 없으면 클라이언트에서 추가적인 작업이 어렵지 않을까요?
예를들어 게시글 생성 이후 해당 게시글의 상세 페이지를 보여준다 와 같은 요구사항이 있을 수도 있을 것 같아요. 이를 위해 응답 Body나 Location 헤더를 통해 자원의 위치를 표현해주면 어떨까요?
| if (lineEntity.isPresent()) { | ||
| throw new IllegalArgumentException("이미 존재하는 노선 이름입니다."); | ||
| } | ||
| final Line line = new Line(lineCreateDto.getName(), Sections.empty()); |
There was a problem hiding this comment.
Line이 생성될때 Sections는 empty로 초기화된다는 도메인 로직이 아닐까요? 서비스까지 노출될 필요가 없어보여요.
name만 인자로 받는 정적 팩터리 메서드를 활용해보면 어떨까요?
| } | ||
|
|
||
| public Line(String lineName, Sections sections) { | ||
| this(null, lineName, sections); |
There was a problem hiding this comment.
생성자 체이닝을 이용할땐 일반적으로 모든 인자가 있는 생성자를 맨 마지막에 위치시킵니다!
| @@ -28,12 +27,11 @@ public LineDao(final JdbcTemplate jdbcTemplate) { | |||
| this.namedParameterJdbcTemplate = new NamedParameterJdbcTemplate(jdbcTemplate); | |||
| this.simpleJdbcInsert = new SimpleJdbcInsert(jdbcTemplate) | |||
| .withTableName("LINE") | |||
| return ResponseEntity.ok().build(); | ||
| @DeleteMapping("/lines/{lineId}/stations/{stationId}") | ||
| public ResponseEntity<Void> delete(@PathVariable(name = "lineId") final Long lineId, | ||
| @PathVariable(name = "stationId") final Long stationId) { |
There was a problem hiding this comment.
@PathVariable의 name이 인자의 이름과 같을 경우 생략이 가능합니다
| sections.getSections().forEach((section) -> sectionDao.insert( | ||
| private void updateLine(Line line) { | ||
| sectionDao.deleteAllByLineId(line.getId()); | ||
| line.getSections().forEach((section) -> sectionDao.insert( |
There was a problem hiding this comment.
라인이 변경될때마다 라인이 갖고 있는 섹션 수 만큼 N번 쿼리가 발생할 것 같아요.
이런 경우 bulk insert를 활용할 수도 있습니다. 이 부분은 반영하지 않으셔도 됩니다.
| @@ -3,14 +3,14 @@ DROP TABLE IF EXISTS SECTION; | |||
| create table if not exists STATION | |||
| ( | |||
| id bigint auto_increment not null, | |||
| name varchar(255) not null unique, | |||
| stationName varchar(255) not null, | |||
There was a problem hiding this comment.
지금은 H2를 사용하지만 MySQL의 경우 대소문자를 구분하지 않기 때문에 단어를 구분하기 위해 snake_case를 사용합니다.
| import org.junit.jupiter.api.Test; | ||
| import subway.section.domain.Sections; | ||
|
|
||
| class LineTest { |
There was a problem hiding this comment.
리팩토링을 대규모로 하셨으면 검증을 위해 테스트는 반드시 추가해주세요!
| this.namedParameterJdbcTemplate = new NamedParameterJdbcTemplate(jdbcTemplate); | ||
| this.simpleJdbcInsert = new SimpleJdbcInsert(jdbcTemplate) | ||
| .withTableName("LINE") | ||
| .usingGeneratedKeyColumns("id"); |
There was a problem hiding this comment.
LineDao에서 수행되는 부분을 반복하는 이유가 있을까요?
There was a problem hiding this comment.
억.. 그러게요 왜 또 적어놨을까요..ㅜㅜ
| () -> assertThat(잠실_잠실새내.getDistance()).isEqualTo(4) | ||
| ); | ||
| } | ||
| // @DisplayName("해당 라인 id를 가진 모든 구간을 조죄한다.") |
There was a problem hiding this comment.
git과 같은 VCS를 통해 파악이 가능하니 불필요한 주석은 제거하는것이 좋습니다!
|
#94 (comment) 에 대한 답변 API도 결국 사용자가 사용하는것이기 때문에 구체적인 자원의 관계를 표현하기보단 행위에 집중하는것이 좋다고 생각해요. 변경사항이 많아질까봐 이렇게 리뷰 드리긴 했는데 저라면 |
No description provided.