[터틀] 지하철 정보 관리 - 인수테스트 미션 제출합니다. #33

begaonnuri · 2020-05-09T09:01:15Z

감사합니다.

Hue9010

전체적으로 복잡하지 않게 잘 구현해주셨네요! 그래서 어느부분에서 고민이 되셨는지 궁금하네요.
피드백 확인해 주세요! 궁금한 사항은 코멘트나 DM으로 문의주세요!
피드백이 늦었는데 오늘은 빠른 피드백 드릴 수 있으니 빠르게 문의주셔도 됩니다!

Hue9010 · 2020-05-11T04:17:40Z

src/main/java/wooteco/subway/admin/controller/LineController.java

+	}
+
+	@GetMapping("/admin-line")
+	public ModelAndView adminLine() {


페이지에 대한 요청은 IndexController 쪽으로 옮기고 RestController에서는 페이지에 대한 처리를 빼주세요!
그후 @RequestMapping도 활용해보세요. 😉

ViewController에서 페이지에 대한 요청을 처리하고, 각 RestController에서 해당하는 데이터를 반환해주도록 변경했습니다! @RequestMapping을 통해 중복 Path를 제거했습니다!

Hue9010 · 2020-05-11T04:22:54Z

src/main/java/wooteco/subway/admin/controller/LineController.java

+		String name = request.getName();
+		String color = request.getColor();
+		LocalTime startTime = request.getStartTime();
+		LocalTime endTime = request.getEndTime();
+		int intervalTime = request.getIntervalTime();
+
+		Line line = new Line(name, color, startTime, endTime, intervalTime);
+		Line created = lineRepository.save(line);


service에서 해야할 작업들이 아닐까요? 그래서 지금 service에서 사용하지 않는 메서드들이 많네요.
new Line도 request에서 하나씩 값을 받아와서 만들지 말고 lineRequest.toLine()으로 변환해주면 좋을 것 같네요.

참고로 이름이 중복 될 수 있는데 중복된 이름은 사용되면 안되지 않을까요? 그에 대한 검증이 들어가야 할 것 같네요.

오류 시에 메시지도 모두 클라이언트에서 검증 후 클라이언트에서만 알려주는데 api에서도 요청 실패시 잘못됐다는걸 클라이언트에 알려줄 수 있게 변경을 해보죠. 모든 곳을 완벽하게 하는걸 목표로 하지 말고 이곳에 한해서만 적용해보도록 해요.
이름이 중복됐을 때만 고려하고 중복 시 ResponseEntity.created가 아니라 badRequest를 발생시켜보세요.

다음 사항을 반영했습니다!

Controller의 로직들을 각각 해당하는 Service로 이동했습니다.

RequestDto는 to~, ResponseDto는 of~ 메소드로 변환하는 코드를 해당 클래스로 이동했습니다.

이름에 대한 중복 검증 로직을 추가했습니다.

RestControllerAdvice를 만들고, ExceptionDto를 통해 메시지를 클라이언트에 전달하도록 변경했습니다. 상태코드 또한 badRequest로 변경했습니다.

RestControllerAdvice를 처음 사용해봤는데 적절하지 않은 부분 있으면 지적 부탁드립니다!

Hue9010 · 2020-05-11T04:24:45Z

src/main/java/wooteco/subway/admin/controller/LineController.java

+	}
+
+	@PostMapping("/lines")
+	public ResponseEntity<?> create(


?보다는 제대로된 타입을 적어주세요! 없는 경우에는 Void를 사용하시면 됩니다.

리턴 타입을 명시적으로 변경했습니다!

Hue9010 · 2020-05-11T04:27:44Z

src/main/resources/static/js/views/AdminStation.js

+    $stationNameInput.value = "";
+  };
+
+  const isValidStationName = stationName => {


검증 로직이 클라이언트에만 존재하는데 서버에도 존재해야하지 않을까요?
지금의 경우 동일한 api를 쓰는 다른 클라이언트가 생기게 된다면 모든 클라이언트에서 검증 로직을 추가해줘야 합니다.
사실 클라, 서버 둘 다 하는게 좋다고 생각하지만 클라쪽에서 누락하는 일이 발생하게 됐을 때도 있기 때문에 서버쪽이 더 우선적으로 존재해야 한다고 생각하는 편입니다. 더군다나 클라마다 동일한 검증 로직을 넣는 다는 보장도 없기 때문에 더더욱 api에서 챙겨야 한다고 생각합니다.

서버쪽이 더 우선적으로 존재해야 한다고 생각하는 편입니다.

감사합니다! 서버엔 반드시 검증로직을 추가해야한다는 점을 인지하겠습니다 :)
검증 로직을 추가했습니다!

Hue9010 · 2020-05-11T04:32:23Z

src/main/java/wooteco/subway/admin/service/LineStationService.java

+	public List<LineResponse> findAll() {
+		List<LineResponse> lineResponses = new ArrayList<>();
+		List<Line> lines = lineRepository.findAll();
+		for (Line line : lines) {
+			Set<Station> stations = stationRepository.findAllById(line.getLineStationsId());
+			lineResponses.add(LineResponse.of(line, stations));
+		}
+		return lineResponses;


딱 복잡하지 않는 선에서 스트림을 연습해보기 좋은 상황이네요.
LineResponse에서 Line과 Station을 받아 LineResponse를 생성하는 메서드를 만들어 주시고 Lines의 스트림을 통해서 LineResponse List를 만들어보세요.

지금 보니 LineResponse.of(line, stations)은 존재하는군요! 그럼 더 수월하겠네요. 😉

해당 로직을 스트림만으로 처리하도록 변경했습니다!

Hue9010 · 2020-05-11T04:40:40Z

src/main/java/wooteco/subway/admin/service/LineStationService.java

+	public Set<LineStation> findLineStation(long lineId) {
+		Line line = lineRepository.findById(lineId)
+				.orElseThrow(NoSuchElementException::new);
+		return line.getStations();
+	}


테스트 코드에서만 사용하는 것 같습니다!

해당 로직과 해당 로직을 테스트하는 코드를 제거했습니다!

Hue9010 · 2020-05-11T04:45:38Z

src/main/java/wooteco/subway/admin/controller/LineStationController.java

+		return ResponseEntity.created(url).body(lineStation);
+	}
+
+	@DeleteMapping("/lineStations/{lineId}/{stationId}")


지금처럼 아이디가 연속으로 있으면 Line, Station, LineStation 중 어떤 Id인지 구분이 안됩니다. 일반적인 상황에서 생각해보면 사용자는 lineId는 lineStations의 아이디라고 판단하기 쉽습니다.
/lines/{lineId}/stations/{stationId} 로 표현해주시는게 좋을 것 같습니다.

사용 예)

/lineStations/1/1
/lines/1/stations/1

그리고 REST API 제대로 알고 사용하기의 4-2. URI 설계 시 주의할 점 > 5) URI 경로에는 소문자가 적합하다. 참고해주세요!

지금까지 URI 설계에 대해 생각해본적이 없었는데, 링크를 보면서 흉내내봤습니다! 감사합니다🙂

Hue9010 · 2020-05-11T04:49:26Z

src/main/java/wooteco/subway/admin/controller/LineStationController.java

+		int distance = request.getDistance();
+		int duration = request.getDuration();


진도 부분이 확실치 않아 문의 드리면 이 부분은 화면 포함해서 다음에 구현하는 걸까요?

넵 이번주차에 진행되는것같습니다 :)

Hue9010 · 2020-05-11T04:51:06Z

src/main/resources/schema.sql

+    duration       integer
+);
+
+-- insert into LINE values(1, '1호선', 'bg-blue-700', '05:30', '23:30', 10, CURRENT_TIMESTAMP(), CURRENT_TIMESTAMP());


편하게 초기 데이터들 세팅했네요. 👍
참고로 schema.sql은 스키마에 대한 정의만 작성하고 data.sql를 만들어서 거기로 옮겨주시는게 좋답니다! 😉

data.sql을 생성하고 DML을 해당 파일로 이동했습니다!

Hue9010 · 2020-05-11T04:57:56Z

src/main/resources/application.properties

+spring.h2.console.enabled=true
+spring.h2.console.path=/h2-console

+handlebars.suffix=.html
+
+logging.level.org.springframework.jdbc.core.JdbcTemplate=debug


- LineRequest에서 Line으로 변환하는 로직을 dto 내부로 이동

- StationRequest에서 Station으로 변경하는 로직을 dto 내부로 이동

- 컨트롤러에서 뷰로 LineStationResponse로 반환해주도록 변경 - default 데이터 제거 - 받아온 데이터로 보여주도록 변경

- 에러 발생시 alert로 메시지를 보여주도록 변경

begaonnuri · 2020-05-12T14:59:41Z

이번에도 리뷰 감사합니다. :)

코멘트 달아주신 부분을 반영했습니다!
한가지 반영 못한 부분이 있는데, 너무 지연되는것 같아 먼저 리뷰 요청드렸습니다.

이전 역이 없는 출발역은 추가하지 못하는 것 같네요.

위 내용은 구현할때 생각 못했던 이슈가 있어 해결하고 마저 반영하겠습니다.
또한 테스트코드를 신경 못썼는데 이 부분도 위 문제를 해결하고 마저 반영하겠습니다.

리뷰 해주신 URI 부분과 에러를 처리하는 부분(ControllerAdvice)을 처음 듣고 적용해봐서 미숙한데 부족한 부분 지적해주시면 감사하겠습니다 :)

Hue9010

몇가지 피드백 남겼습니다. 확인 부탁드려요!!

Hue9010 · 2020-05-13T18:46:33Z

src/main/java/wooteco/subway/admin/service/StationService.java

+		if (name.isEmpty()) {
+			throw new IllegalArgumentException("값을 입력해주세요.");
+		}
+		if (name.contains(" ")) {
+			throw new IllegalArgumentException("공백 없이 입력해주세요.");
+		}
+		for (char c : name.toCharArray()) {
+			if (Character.isDigit(c)) {
+				throw new IllegalArgumentException("숫자 없이 입력해주세요.");
+			}
+		}


중복 확인 외에는 StationService가 아니라 Station에서 생성시에 확인하는게 좋지 않을까요? 😉

StationName을 VO로 만들고 검증 로직을 한곳에서 관리하도록 변경했습니다 :)

Hue9010 · 2020-05-13T19:01:36Z

src/main/java/wooteco/subway/admin/controller/advice/ExceptionAdvice.java

+
+import wooteco.subway.admin.controller.advice.dto.ExceptionDto;
+
+@RestControllerAdvice


try-catch, ExceptionHandler, ControllerAdvice의 차이가 무엇일까요?
스프링부트 : REST어플리케이션에서 예외처리하기
구현부분은 크게 참고할 부분은 없고 첫 문단과 전체적인 내용만 보시면 될 것 같아요. 😉

사실 ControllerAdvice을 주로 쓰는 편이지만 먼저 언급 드리지 않았는데 이유는 이런 스프링의 기능은 설정이나 애노테이션 정도로 손쉽게 사용이 가능해지는 부분이 있어 편하다면 편하지만 조금은 더 눈에 보이는 단계에서 처리를 해보고자 했답니다. 하지만 먼저 알게 되는 것도 좋은 방향이라고 생각합니다. 😉

링크까지 주시고 감사합니다 :) @Responsetatus는 처음 접하네요!

작성 시기도 어느정도 되어서 키워드 정도만 챙기시면 될 것 같아요.
대표적으로 RestControllerAdvice가 아닌 ControllerAdvice, RestController 두가지를 사용하고 있죠. 😉

Hue9010 · 2020-05-13T19:07:10Z

src/main/java/wooteco/subway/admin/dto/LineResponse.java

+	private LocalDateTime createdAt;
+	private LocalDateTime updatedAt;


지금 제거하실 필요는 없지만 사실 도메인 모델과 Reponse간에 1:1로 맞춰야 할 필요는 없답니다. 즉 응답에 불필요하다 생각된다면 포함시키지 않아도 무방하답니다.

도메인 모델과 Reponse간에 1:1로 맞춰야 할 필요는 없답니다.

몰랐던 사실 알려주셔서 감사합니다 :) 두 필드 모두 제거했습니다.

Hue9010 · 2020-05-13T19:11:50Z

src/main/java/wooteco/subway/admin/controller/advice/dto/ExceptionDto.java

@@ -0,0 +1,16 @@
+package wooteco.subway.admin.controller.advice.dto;
+
+public class ExceptionDto {


ControllerAdvice는 현재 package 구조로는 지금 위치가 적당해보이지만 ExceptionDto는 엄염히 Dto인데 Dto 패키지로 가야하지 않을까요?

DTO이긴 한데 Controller에서 생성되고 Advice에서 에러 처리용도로만 사용되어서 그렇게 배치했는데, 적절하지 않았나보네요. dto 패키지로 이동했습니다.

Hue9010 · 2020-05-13T19:13:41Z

src/main/java/wooteco/subway/admin/service/EdgeService.java

+		return line.getStations();
+	}
+
+	@Transactional


@transactional 👍
Transactional을 붙였을 때와 안 붙였을 때의 차이가 무엇일까요?

해당 메소드에서 문제가 생겨 처리가 중간에 끊겼을때 해당 트랜잭션을 롤백시켜 의도하지 않은 데이터 조작을 막을 수 있습니다. 그렇기 때문에 CUD 메소드를 Transactional로 만들면 적절하다고 생각합니다!

Transactional을 물어보진 않겠지만 나중에 면접때 학습한/사용한 기술을 물어 볼 수 도 있어서 어느 정도 대답 해 둘 수 있으면 좋답니다. 😉

Hue9010 · 2020-05-13T19:14:51Z

src/main/resources/static/api/index.js

+    }
+  };
+
+  const lineStation = {


lineStation과 edge 중 하나로 통일하는게 좋을 것 같아요.

모두 변경했다고 생각했는데 놓친 부분이 있었네요! 수정했습니다 :)

Hue9010 · 2020-05-13T19:17:44Z

src/main/java/wooteco/subway/admin/domain/Line.java

+		Edge target = stations.stream()
+				.filter(st -> st.isEqualsWithStationId(stationId))
+				.findFirst()
+				.orElseThrow(NoSuchElementException::new);


NoSuchElementException이 발생하는 곳은 메시지가 없어서인지 화면에서 null이라는 문구를 보여주는 것 같네요.
화면에서 요청 실패 시 실패 메시지를 그대로 사용자한테 보여주면 발생할 수 있는 상황이랍니다. 정말 그대로 보여주던지 보여주는 상황과 아닌 경우를 클라이언트에서 정해줄 필요가 있답니다.

id를 직접적으로 사용하지 않는 사용자에게 NoSuchElement에 대한 커스텀 익셉션을 만들어 메시지를 전할 필요까진 없다고 생각이 드네요! ControllerAdvice에서 IllegalArgumentException만을 처리하도록 변경했습니다!

Hue9010 · 2020-05-13T19:18:56Z

src/main/java/wooteco/subway/admin/service/EdgeService.java

+		Station preStation = stationRepository.findByName(request.getPreStationName())
+				.orElseThrow(NoSuchElementException::new);


빈 구간에 역을 추가해보는게 볼가하네요. 기능 추가 부탁드립니다. 😢

아래 사항을 변경해서 기능 추가했습니다!

출발역 다음역을 select box로 선택할 수 있도록 변경

front에서 dataset으로 Id를 갖고있도록 변경

id를 통해 Line을 조회하도록 변경

Hue9010 · 2020-05-13T19:21:32Z

src/test/java/wooteco/subway/admin/acceptance/EdgeAcceptanceTest.java

+	@Mock
+	private StationRepository stationRepository;
+
+	private LineStationService lineStationService;


말씀 주신것 처럼 테스트 코드 수정 부탁드려요!
테스트 코드가 잘 짜두면 많은 도움이 되지만 신경을 못 쓰다보면 도리어 더 큰 기술 부채가 되어 버릴수도 있답니다. 😢

테스트 코드 수정했습니다! 매번 반영 우선순위에서 밀리네요 😢
변경사항이 있을때마다 신경쓰겠습니다!

커밋하기 직전에 전체 테스트를 돌려보면서 이상이 없는지 확인 해보는게 좋답니다. 😉
그래서 빠른 테스트가 중요한데 테스트가 오래 걸린다면 자주 돌리는데 부담이 되고 전체 테스트를 해보는 주기가 길어지게 됩니다. 사실 이 부분에선 저도 개선해야 할 부분이 있네요. 😏

- lineStation의 이름을edge로 변경 - ExceptionAdvice에서 NoSuchElementException을 처리하지 않도록 변경 - LineResponse의 createdAt, updatedAt 필드제거

Hue9010

고생하셨습니다! 👍 이번 단계는 여기서 머지하도록 할게요! 😉

yeonnseok and others added 29 commits May 6, 2020 16:43

docs: 1단계 기능 목록 작성

3dfbcb8

feat: 지하철 노선 추가 기능

b1085a7

feat: 지하철 노선 목록 조회 기능

5e7213f

feat: LineStation 테이블 생성

1d71762

feat: 노선 조회 기능

a8a310d

feat: 노선 수정 기능

d70ae2c

feat: 노선 삭제 기능

c0da499

docs: 2단계 기능 목록 추가

c547297

feat: 프론트 리소스 추가

fd74dee

feat: 인덱스 컨트롤러 추가

fa3c9e5

feat: 페이지에서 노선 목록 조회

155200e

docs: 프론트 유효성 검증 구현 목록 추가

c2425cd

feat: 페이지에서 노선 추가

032e9aa

feat: 페이지에서 노선 상세 조회

c4c338b

feat: 페이지에서 노선 수정

78696e9

feat: 페이지에서 노선 삭제

a2b0e60

test: 테스트에 color추가

b3f4889

refactor: 설정 변경

164a4f4

docs: 3단계 구현 목록 추가

4647608

refactor: 프론트 리소스 위치 설정 변경

f2836f6

feat: 지하철 노선에 역 추가 기능, 테스트 작성

5bb6ff6

refactor: 지하철 노선에 역 추가 테스트, Mock 서버 활용

fdadfdc

feat: 지하철 노선의 지하철역 목록 조회

55762f5

feat: 지하철 노선에 역 제거

97219c0

style: 코드 포맷 변경

2f92e2a

docs: 4단계 기능 목록 추가

5d06978

feat: 지하철 노선에 역 추가 기능 구현

a69c0ab

docs: 노선에 추가된 역들로 노선을 조회하는 기능 구현

57960d2

feat: 지하철 노선에 역 제거하는 기능 구현

293ce8b

begaonnuri changed the title ~~[터틀] 미션제출~~ [터틀] 지하철 정보 관리 - 인수테스트 미션 제출합니다. May 9, 2020

begaonnuri added 6 commits May 10, 2020 20:11

feat: 구간 추가 기능 구현

41bfe7d

refactor: 노선의 id를 dataset으로 갖고있도록 변경

3371810

feat: 구간 제거 기능 구현

2587745

refactor: 임시 데이터 제거

3a1f2d5

feat: 역 추가하는 기능 구현

c2f735d

feat: 역 제거하는 기능 구현

3dfac58

Hue9010 suggested changes May 11, 2020

View reviewed changes

begaonnuri added 13 commits May 11, 2020 14:53

refactor: 페이지 요청과 데이터 요청 분리

923a8b6

refactor: ResponseEntity의 타입을 명확하게 변경

5ae0cca

refactor: URI변경 및 RequestMapping 설정

c635570

refactor: 정적 페이지를 ViewController에서 반환하도록 변경

8ce25d3

refactor: api 요청 메소드를 파일로 분리

4066cdf

refactor: LineController의 로직을 임시Service로 이동

fa7d962

- LineRequest에서 Line으로 변환하는 로직을 dto 내부로 이동

refactor: StationController의 로직을 StationService로 이동

b430739

- StationRequest에서 Station으로 변경하는 로직을 dto 내부로 이동

refactor: LineStationController의 로직을 LineStationService로 이동

d0e5f43

- 컨트롤러에서 뷰로 LineStationResponse로 반환해주도록 변경 - default 데이터 제거 - 받아온 데이터로 보여주도록 변경

refactor: 템플릿엔진으로 보여주던 방식에서 DOM을 생성해 보여주는 방식으로 변경

4804e3a

style: NewLineService의 이름을 LineService로 변경

63fef2d

style: LineStation의 이름을 Edge로 변경

452afac

refactor: data.sql 추가

0f158ae

feat: 중복 노선 이름 검증 기능 추가

5c47f5c

- 에러 발생시 alert로 메시지를 보여주도록 변경

feat: 역 이름 검증 로직 추가

5535340

Hue9010 suggested changes May 13, 2020

View reviewed changes

begaonnuri added 5 commits May 14, 2020 23:07

refactor: 역 이름의 검증로직 일부를 StationName클래스로 이동

14a9c91

refactor: 리뷰 반영

b498270

- lineStation의 이름을edge로 변경 - ExceptionAdvice에서 NoSuchElementException을 처리하지 않도록 변경 - LineResponse의 createdAt, updatedAt 필드제거

test: 인수테스트 수정

324a424

fix: 출발 역도 추가 가능하도록 변경

8d6a69f

test: 테스트코드 수정

575fd39

Hue9010 approved these changes May 17, 2020

View reviewed changes

Hue9010 merged commit d8fc759 into woowacourse:begaonnuri May 17, 2020

		int distance = request.getDistance();
		int duration = request.getDuration();


		import wooteco.subway.admin.controller.advice.dto.ExceptionDto;

		@RestControllerAdvice

		private LocalDateTime createdAt;
		private LocalDateTime updatedAt;

		@@ -0,0 +1,16 @@
		package wooteco.subway.admin.controller.advice.dto;

		public class ExceptionDto {

		Station preStation = stationRepository.findByName(request.getPreStationName())
		.orElseThrow(NoSuchElementException::new);

Uh oh!

[터틀] 지하철 정보 관리 - 인수테스트 미션 제출합니다. #33

[터틀] 지하철 정보 관리 - 인수테스트 미션 제출합니다. #33

Uh oh!

Conversation

begaonnuri commented May 9, 2020

Uh oh!

Hue9010 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

begaonnuri May 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

begaonnuri May 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

begaonnuri commented May 12, 2020

Uh oh!

Hue9010 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

begaonnuri May 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

begaonnuri May 12, 2020 •

edited

Loading

begaonnuri May 12, 2020 •

edited

Loading

begaonnuri May 15, 2020 •

edited

Loading