-
Notifications
You must be signed in to change notification settings - Fork 177
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
[학성] 지하철 정보 관리 - 인수 테스트 미션 제출합니다. #2
Changes from 27 commits
47db1b3
39c8edf
de22f86
26e2c48
20cb2f2
94998ed
d177ba0
ab5323f
c958b4a
e32749c
5c29b66
986a35c
04f4b56
28e0b35
6086bcc
054b237
5c017ff
b7330f7
2776b45
505db8e
caf2bbb
4cf5d4a
e4b8c64
c20cb95
c01a32b
f256963
5a0f382
0a62468
18da087
3c39990
c0f1664
9f48090
d4be836
e1cbe31
6320b00
0ea08bc
9b14281
d1f7cd5
18d9f0f
3f03bfb
c8d8503
c162c16
049690e
61945e8
5670163
fd4f285
f1b5254
8d0d32b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,3 +30,4 @@ out/ | |
|
||
### VS Code ### | ||
.vscode/ | ||
.DS_Store |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,28 +1,29 @@ | ||
plugins { | ||
id 'org.springframework.boot' version '2.2.5.RELEASE' | ||
id 'io.spring.dependency-management' version '1.0.9.RELEASE' | ||
id 'java' | ||
id 'org.springframework.boot' version '2.2.5.RELEASE' | ||
id 'io.spring.dependency-management' version '1.0.9.RELEASE' | ||
id 'java' | ||
} | ||
|
||
group = 'com.example' | ||
version = '0.0.1-SNAPSHOT' | ||
sourceCompatibility = '1.8' | ||
|
||
repositories { | ||
mavenCentral() | ||
mavenCentral() | ||
} | ||
|
||
dependencies { | ||
implementation 'org.springframework.boot:spring-boot-starter-web' | ||
implementation 'org.springframework.boot:spring-boot-starter-data-jdbc' | ||
implementation 'net.rakugakibox.spring.boot:logback-access-spring-boot-starter:2.7.1' | ||
testImplementation 'io.rest-assured:rest-assured:3.3.0' | ||
testImplementation('org.springframework.boot:spring-boot-starter-test') { | ||
exclude group: 'org.junit.vintage', module: 'junit-vintage-engine' | ||
} | ||
runtimeOnly 'com.h2database:h2' | ||
implementation 'org.springframework.boot:spring-boot-starter-web' | ||
implementation 'org.springframework.boot:spring-boot-starter-data-jdbc' | ||
implementation 'net.rakugakibox.spring.boot:logback-access-spring-boot-starter:2.7.1' | ||
implementation 'pl.allegro.tech.boot:handlebars-spring-boot-starter:0.3.1' | ||
testImplementation 'io.rest-assured:rest-assured:3.3.0' | ||
testImplementation('org.springframework.boot:spring-boot-starter-test') { | ||
exclude group: 'org.junit.vintage', module: 'junit-vintage-engine' | ||
} | ||
runtimeOnly 'com.h2database:h2' | ||
} | ||
|
||
test { | ||
useJUnitPlatform() | ||
useJUnitPlatform() | ||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,105 @@ | ||||||
package wooteco.subway.admin.controller; | ||||||
|
||||||
import org.springframework.http.ResponseEntity; | ||||||
import org.springframework.web.bind.annotation.DeleteMapping; | ||||||
import org.springframework.web.bind.annotation.GetMapping; | ||||||
import org.springframework.web.bind.annotation.PathVariable; | ||||||
import org.springframework.web.bind.annotation.PostMapping; | ||||||
import org.springframework.web.bind.annotation.PutMapping; | ||||||
import org.springframework.web.bind.annotation.RequestBody; | ||||||
import org.springframework.web.bind.annotation.RequestMapping; | ||||||
import org.springframework.web.bind.annotation.RestController; | ||||||
import wooteco.subway.admin.domain.Line; | ||||||
import wooteco.subway.admin.domain.Station; | ||||||
import wooteco.subway.admin.dto.LineRequest; | ||||||
import wooteco.subway.admin.dto.LineResponse; | ||||||
import wooteco.subway.admin.dto.LineStationCreateRequest; | ||||||
import wooteco.subway.admin.service.LineService; | ||||||
import wooteco.subway.admin.service.StationService; | ||||||
|
||||||
import java.net.URI; | ||||||
import java.util.HashMap; | ||||||
import java.util.List; | ||||||
import java.util.stream.Collectors; | ||||||
|
||||||
@RestController | ||||||
@RequestMapping("/lines") | ||||||
public class LineController { | ||||||
private final LineService lineService; | ||||||
private final StationService stationService; | ||||||
|
||||||
public LineController(final LineService lineService, final StationService stationService) { | ||||||
this.lineService = lineService; | ||||||
this.stationService = stationService; | ||||||
} | ||||||
|
||||||
@PostMapping | ||||||
public ResponseEntity createLine(@RequestBody LineRequest lineRequest) { | ||||||
lineService.validateTitle(lineRequest); | ||||||
Line line = new Line( | ||||||
lineRequest.getTitle(), | ||||||
lineRequest.getStartTime(), | ||||||
lineRequest.getEndTime(), | ||||||
lineRequest.getIntervalTime(), | ||||||
lineRequest.getBgColor() | ||||||
); | ||||||
Line savedLine = lineService.save(line); | ||||||
return ResponseEntity.created(URI.create("/lines/" + savedLine.getId())).body(savedLine); | ||||||
} | ||||||
|
||||||
@GetMapping | ||||||
public ResponseEntity findAllLine() { | ||||||
List<LineResponse> lineResponses = lineService.findAll().stream() | ||||||
.map(line -> lineService.findLineWithStationsById(line.getId())) | ||||||
.collect(Collectors.toList()); | ||||||
return ResponseEntity.ok(lineResponses); | ||||||
} | ||||||
|
||||||
@GetMapping("/{id}") | ||||||
public ResponseEntity findLine(@PathVariable Long id) { | ||||||
return ResponseEntity.ok(lineService.findLineWithStationsById(id)); | ||||||
} | ||||||
|
||||||
@PutMapping("/{id}") | ||||||
public ResponseEntity updateLine(@PathVariable Long id, @RequestBody LineRequest lineRequest) { | ||||||
lineService.validateTitleWhenUpdateInfo(id, lineRequest); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 컨트롤러에서 validateTitleWhenUpdateInfo 후 update하지 말고 lineService.updateLine에서 검증하는건 어떨까요? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. 감사합니다. 말씀하신부분은 제가 미처 생각을 하지 못했네요. 저는 controller 와 service 의 역할 구분을 "작업 흐름,전처리 vs 실질적인 작업 수행" 로 생각을 하였고, 그래서 controller 에서 유효성 검사를 하였습니다. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 눈으로만 그냥 가볍게 볼만한 자료는 안보이네요 😿 https://jojoldu.tistory.com/129 에서 보면 유효성 검사는 service에서 주로 한다기보다는 앞서 말씀 드린 애노테이션으로 request에서 요청 데이터들에 대해서 검증 할 테고, 도메인에서도 생성 시 문제가 없는지 검증, 노선 이름이 중복되는지 여부에 대해서는 db를 통해야 하기 때문에 service에서 이뤄지겠네요. |
||||||
Line line = new Line( | ||||||
lineRequest.getTitle(), | ||||||
lineRequest.getStartTime(), | ||||||
lineRequest.getEndTime(), | ||||||
lineRequest.getIntervalTime(), | ||||||
lineRequest.getBgColor() | ||||||
); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. lineRequest.toLine()을 통해서 line으로 변환해 줄 수 있을 것 같네요. 😉 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 앗! 리뷰중 수정해주신 거 확인 했는데 LineStation을 보니 도메인 모델에서 of로 변환을 하시는데 도메인 모델에서 변환을 하시면 나중에 dto의 변경이 생길 때마다 도메인 모델도 변경을 해줘야 합니다. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 아 그렇겠네요! 감사합니다 |
||||||
lineService.updateLine(id, line); | ||||||
return ResponseEntity.ok().build(); | ||||||
} | ||||||
|
||||||
@DeleteMapping("/{id:\\d+}") | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
public ResponseEntity deleteLine(@PathVariable Long id) { | ||||||
lineService.deleteLineById(id); | ||||||
return ResponseEntity.ok().build(); | ||||||
} | ||||||
|
||||||
@PostMapping("/addStation/{id}") | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Line, Station, LineStation 중 어떤 Id인지 구분이 안됩니다. 특정 노선에 지하철 역을 추가한다는 걸 알 수 있게 그리고 REST API 제대로 알고 사용하기의 |
||||||
public ResponseEntity addStation(@PathVariable Long id, | ||||||
@RequestBody HashMap<String, String> map) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 이 부분은 코멘트에 남긴 것 처럼 map이 아니라 LineStationCreateRequest로 받아보세요. |
||||||
Long preStationId = stationService.findStationId(map.get("preStationName")); | ||||||
Station inputStation = stationService.save(map.get("stationName")); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. + 기존에는 request 모델을 사용한 걸로 보이는데 map으로 바꾸신 이유가 있을까요? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. 그리고 위에 말씀하신부분이 이 부분이 맞는지는 모르겠지만 처음에는 @PostMapping("/{id}/stations")
public ResponseEntity addStation(@PathVariable Long id,
@RequestBody String preStationName,
@RequestBody String stationName,
@RequestBody String distance,
@RequestBody String duration) { 이렇게 했었는데 인식이 안되길래 map으로 바꾸었습니다. map은 잘 쓰지 않는 방법인가요? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. map 많이 쓴다면 많이 쓰고 아니라면 아닙니다. Request에 대한 관리가 안된다는 측면에서 저는 map의 사용은 비추합니다. 그리고 차후에 배우겠지만(아마도 빠르면 즐겨찾기에서 문서자동화 부분) request모델을 사용하면 좀더 쉽게 추가 기능들을 붙일 수 있답니다. 올려주신 코드가 안되신 이유는 http를 다시 한번 생각해보면 하나의 Request에는 하나의 body밖에는 존재하지 않습니다! 그렇기 때문에 여러개의 ReqeustBody는 사용이 불가합니다. |
||||||
|
||||||
LineStationCreateRequest lineStationCreateRequest = | ||||||
new LineStationCreateRequest( | ||||||
preStationId, | ||||||
inputStation.getId(), | ||||||
Integer.parseInt(map.get("distance")), | ||||||
Integer.parseInt(map.get("duration"))); | ||||||
|
||||||
lineService.addLineStation(id, lineStationCreateRequest); | ||||||
return ResponseEntity.ok().build(); | ||||||
} | ||||||
|
||||||
@DeleteMapping("/station/{lineId}/{stationId}") | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||
public ResponseEntity deleteStation(@PathVariable Long lineId, @PathVariable Long stationId) { | ||||||
lineService.removeLineStation(lineId, stationId); | ||||||
return ResponseEntity.ok().build(); | ||||||
} | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
package wooteco.subway.admin.controller; | ||
|
||
import org.springframework.http.ResponseEntity; | ||
import org.springframework.web.bind.annotation.GetMapping; | ||
import org.springframework.web.bind.annotation.PathVariable; | ||
import org.springframework.web.bind.annotation.RestController; | ||
import wooteco.subway.admin.service.LineService; | ||
|
||
@RestController | ||
public class LineStationController { | ||
private final LineService lineService; | ||
|
||
public LineStationController(final LineService lineService) { | ||
this.lineService = lineService; | ||
} | ||
|
||
@GetMapping("/lineStations/{lineId}") | ||
public ResponseEntity findAllLineStations(@PathVariable Long lineId) { | ||
return ResponseEntity.ok(lineService.findLineStationByLineId(lineId)); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,12 @@ | ||
package wooteco.subway.admin.controller; | ||
|
||
import org.springframework.http.ResponseEntity; | ||
import org.springframework.web.bind.annotation.*; | ||
import org.springframework.web.bind.annotation.DeleteMapping; | ||
import org.springframework.web.bind.annotation.GetMapping; | ||
import org.springframework.web.bind.annotation.PathVariable; | ||
import org.springframework.web.bind.annotation.PostMapping; | ||
import org.springframework.web.bind.annotation.RequestBody; | ||
import org.springframework.web.bind.annotation.RestController; | ||
import wooteco.subway.admin.domain.Station; | ||
import wooteco.subway.admin.dto.StationCreateRequest; | ||
import wooteco.subway.admin.dto.StationResponse; | ||
|
@@ -20,11 +25,19 @@ public StationController(StationRepository stationRepository) { | |
@PostMapping("/stations") | ||
public ResponseEntity createStation(@RequestBody StationCreateRequest view) { | ||
Station station = view.toStation(); | ||
Station persistStation = stationRepository.save(station); | ||
validateName(station.getName()); | ||
|
||
Station persistStation = stationRepository.save(station); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Service가 아닌 Controller에서 repository를 직접 사용하고 계시네요. |
||
return ResponseEntity | ||
.created(URI.create("/stations/" + persistStation.getId())) | ||
.body(StationResponse.of(persistStation)); | ||
.created(URI.create("/stations/" + persistStation.getId())) | ||
.body(StationResponse.of(persistStation)); | ||
} | ||
|
||
private void validateName(final String stationName) { | ||
stationRepository.findByName(stationName) | ||
.ifPresent(station -> { | ||
throw new IllegalArgumentException("존재하는 역 이름입니다"); | ||
}); | ||
} | ||
|
||
@GetMapping("/stations") | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
package wooteco.subway.admin.controller; | ||
|
||
import org.springframework.stereotype.Controller; | ||
import org.springframework.web.bind.annotation.GetMapping; | ||
|
||
@Controller | ||
public class WebController { | ||
@GetMapping("/") | ||
public String index() { | ||
return "index"; | ||
} | ||
|
||
@GetMapping("/admin-line") | ||
public String adminLine() { | ||
return "admin-line"; | ||
} | ||
|
||
@GetMapping("/admin-edge") | ||
public String adminEdge() { | ||
return "admin-edge"; | ||
} | ||
|
||
@GetMapping("/admin-station") | ||
public String adminStation() { | ||
return "admin-station"; | ||
} | ||
} |
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.
한번에 많이 모든 곳을 처리하려 하지 말고 한번 여기에 한해서 예외 처리를 시도해보죠.
이름이 중복되면 IllegalArgumentException이 발생하는데 try-catch로 아래 형태로 응답해보세요.
사실 위의 방법도 시도해보셨을 수도 있을 것 같은데 조금 더 구체적으로 시도해보신 방법을 아는게 좋을 것 같아요. 여차하면 실패한 방법을 적용해서 push 해주시는 것도 좋을 것 같습니다.