Skip to content
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

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

Merged
merged 47 commits into from
May 16, 2020

Conversation

giantim
Copy link

@giantim giantim commented May 9, 2020

구구 안녕하세요! 오랜만에 다시 리뷰 요청 드립니다!

구간을 추가할 때 지하철 역 이름을 직접 입력해야 하고 시작 역을 등록할때는 이전역을 출발역 이라고 직접 기재해야 하는 불편함이 있습니다....

많은 지적, 피드백 부탁드립니다 감사합니다 😊

giantim and others added 30 commits May 6, 2020 13:41
LineController post lines 라우터 구현
LineController get lines 라우터 구현
LineController get lines 시 lineStationsId 컬럼까지 추가 반환하는 버그 수정
LineController line id 를 받는 라우터 구현
style: 컨벤션 수정

LineController 업데이트 라우터 구현
style: 컨벤션 수정

LineController 업데이트 라우터 구현
feat: 라인 삭제 라우팅 구현

LineController 삭제 라우터 구현
LineController 모든 라우터에 LineService 적용
LineRepository 노선 이름 중복 검사를 위한 메서드 구현
LineService 노선을 추가할 때 이름 중복 검사 로직 추가 및 테스트 코드 작성
LineService 업데이트 중복 체크 구현
프론트 리소스 추가
AdminController IndexController -> AdminController 로 이름 변경 및 노선 관리 페이지 이동 라우터 구현
AdminLine.js 에서 저장된 노선 목록 불러오도록 변경
index.html 노선 관리 클릭 시 페이지가 아닌 라우터로 연결
schema 기본 노선 데이터 추가
Line bgColor 컬럼 추가
LineRequest bgColor 컬럼 추가
뷰 수정
style: 미사용 import문 정리

LineController 노선 저장 api request 타입 변경
LineResponse bgColor property 추가
Request 요청에 대한 제네릭 클래스 생성
schema 테스트 위한 데이터 초기화
뷰 수정
AdminLine.js 노선 상세 정보 표시 기능 구현
LineController PutMapping 요청에 대한 타입 변경
AdminLine.js 모달 페이지 기능 별 이벤트 수정
index.js PUT 메서드에 값 전달
AdminLine.js 노선 삭제 함수 구현
index.js 노선 삭제 요청 api 연결
admin-line.html 뷰의 기본 값 삭제
refactor: StationController 의 공통 라우터 추출

LineStationController 구현
LineStationResponse dto 생성
LineStationService 테스트를 위한 기능 구현
LineStationAcceptanceTest 지하철 노선에 역을 추가하는 기능 테스트
feat: 3단계 완료
refactor: LineStationController 라우팅 uri 변경

LineStationController 라우팅 uri 변경
LineStationService 테스트 위한 삭제 기능 구현
LineStationAcceptanceTest 삭제 테스트 추가 구현
LineStationController LineStationService -> LineService 사용하도록 변경
LineStationResponse of 메서드 오버라이딩
LineStationCreateRequest 메서드 인자 변경
LineService LineStation 관련 기능 구현
Line LineStation 제거 기능 구현
schema line_station 테이블에 line 인덱스 컬럼 추가
LineStation 이전역 정보 수정 기능 구현
LineStationResponse of 오버로딩 메서드 추가
LineService line id로 역 정보 불러오기 구현
Line 역 추가 삭제 기능 구현
StationRepository station id 목록으로 해당 역들 불러오기 구현
LineAcceptanceTest @Sql 추가
StationAcceptanceTest @Sql 추가
truncate.sql auto_incremenet 값을 1로 초기화 하는 구문 추가
refactor: 공통 코드 메서드 추출

Line 공통 부분 메서드로 추출
LineStation LineId 를 모르도록 프로퍼티 변경
LineStationResponse LineId 를 서비스에서 참조하도록 변경
LineService LineId를 LineStationResponse 를 생성할 때 전달하도록 변경
schema.sql 컨벤션, 데이터 타입 변경
AdminController 역 관리, 구간 관리 페이지로 연결하는 라우터 추가
StationCreateRequest 기본 생성자를 추가해서 응답을 받을 수 있도록 변경
index.js request 함수를 body 가 있을때와 없을때를 구분해서 반환값을 다르게 변경
AdminStation.js 역 관리 기능 구현
constants.js 역 관리에서 사용하는 상수 추가
giantim added 5 commits May 9, 2020 16:51
AdminEdge.js 저장된 데이터를 불러와서 보여주기 / 삭제 기능 구현
admin-edge.html div id 를 목적에 맞게 수정
schema.sql 임시 데이터 저장 쿼리 추가
fix: 노선 이름을 제외한 정보 업데이트 안되는 문제 수정

LineService 노선 이름 중복 검사 로직 업데이트 시에는 수행하지 않도록 변경
index.js 구간 추가 요청 라우터 구현
AdminEdge.js 구간 추가 기능 구현
AdminLine.js 노선 이름을 같게 업데이트 요청 보내면 예외를 던지는 문제 수정
LineController 제네릭에 자료형 추가
LineStationController 제네릭에 자료형 추가
StationController 제네릭에 자료형 추가
LineResponse 생성자에 bgColor 를 이용하도록 변경
LineStation 사용하지 않는 메서드 삭제
LineController 반환값을 LineResponse 로 변경
LineResponse 사용하지 않는 메서드 제거
LineService LineResponse 를 생성하여 역 정보를 담아서 모든 노선 정보를 보여주도록 변경
AdminEdge.js 구간 정보 불러오는 로직 수정
fix: 역을 추가할 때 중복 검사를 실패하는 문제 수정

admin-edge.html input -> select 으로 변경
AdminEdge.js 노선에 포함된 이전역을 불러오는 함수 구현
AdminStation.js 중복 검사 로직 변경
constants.js 이벤트 상수 추가
schema.sql 테스트 데이터 추가
Copy link

@kang-hyungu kang-hyungu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 라빈 다시 만나서 반갑네요 :)
미션 구현 잘 하셨어요 👏
피드백 남겼으니 확인해보시고 궁금한 점 있으시면 코멘트 남기거나 dm 주세요

new IllegalArgumentException("잘못된 라인 아이디를 입력하였습니다."));
List<LineStation> lineStations = line.getStations();

return Collections.unmodifiableList(lineStations.stream()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return Collections.unmodifiableList(lineStations.stream()
return lineStations.stream()
.map(lineStation -> LineStationResponse.of(id, lineStation))
.collect(Collectors.collectingAndThen(Collectors.toList(), Collections::unmodifiableList));

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Collections 에 변수로 넘겨주던 값을 모두 람다식으로 변경했습니다 :)

@RestController
@RequestMapping("/lines")
public class LineController {
private LineService lineService;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private LineService lineService;
private final LineService lineService;

final을 붙여주는게 좋겠네요
다른 컨트롤러, 서비스도 멤버 변수들에 final을 붙여주세요

}

@PostMapping
public ResponseEntity createLines(@RequestBody Request<LineRequest> view) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public ResponseEntity createLines(@RequestBody Request<LineRequest> view) {
public ResponseEntity<LineResponse> createLines(@RequestBody Request<LineRequest> view) {

반환하는 ResponseEntity에 타입을 명시해주는게 좋습니다
다른 메서드도 같이 변경해주세요

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ResponseEntity 에 메서드의 반환형을 명시적으로 작성했습니다 :)


@PostMapping
public ResponseEntity createLines(@RequestBody Request<LineRequest> view) {
Line line = view.getContent().toLine();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Request 클래스로 감싸면 이런 부분이 불편하지 않을까요?
Request로 감싸면 어떤 장점이 있나요?

Copy link
Author

@giantim giantim May 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저가 페어와 Request 클래스를 설계한 이유는 제공받은 자바스크립 코드에서 값을 넘겨줄 때 다음과 같은 형식으로 넘겨주어서 설계했습니다.
PUT(data) { return { method: "PUT", headers: { "Content-Type": "application/json" }, body: JSON.stringify({ content: data }) };
이 api 를 공통으로 이용하기 위해 content 에서 값을 자동으로 꺼내기 용이하도록 Request 클래스를 설계했습니다.

Request 클래스를 설계한 이유는 자바스크립트의 api 를 모든 컨트롤러에서 사용할 수 있게 변경할 방법을 모르겠어서 제공받은 api 에 맞춰서 설계했습니다 :) Request 를 설계함으로써 생긴 장점은 제공받은 프론트 코드를 변경하지 않고서 작성한 모든 컨트롤러에서 api 를 사용할 수 있다는 점이라고 생각합니다!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

프론트 개발자와 협의해서 정하면 문제 없는 코드죠 👍

@@ -18,13 +20,15 @@
private int intervalTime;
private LocalDateTime createdAt;
private LocalDateTime updatedAt;
private String bgColor;

private Set<Station> stations;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dto에 도메인 클래스가 노출되면 안됩니다.
도메인 클래스 변경 사항이 있을 때마다 dto에 그대로 반영되는 문제가 생깁니다.
StationDto를 추가해주세요

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dto 가 도메인에 의존하지 않도록 기존에 작성해두었던 StationResponse 클래스를 이용하여 값을 전달하도록 변경하였습니다.


@Service
public class LineService {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

트랜잭션 추가도 필요해보이네요
cud 작업할 때는 꼭 Transactional 어노테이션을 추가해주세요

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

설계한 서비스에서 cud 작업이 발생하는 부분에 트랜잭션 어노테이션을 모두 추가해 주었습니다!

public void deleteLineById(Long id) {
lineRepository.deleteById(id);
}

public void addLineStation(Long id, LineStationCreateRequest request) {
// TODO: 구현
Line line = lineRepository.findById(id).orElseThrow(() ->

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lineRepository.findById(id).orElseThrow
이 패턴이 반복적으로 보이니 메서드 하나로 묶어주면 중복을 줄일 수 있겠네요


@Service
public class LineService {
private LineRepository lineRepository;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private LineRepository lineRepository;
private final LineRepository lineRepository;

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

빈으로 사용하는 레포지토리 객체에 모두 final 키워드를 붙여주었습니다 😃

);

insert into station values(1, '수원', CURRENT_TIMESTAMP());

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

schema를 정의하는 쿼리만 남기고 데이터 추가하는 쿼리는 따로 분리해주세요

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

data 를 추가하는 구문은 data.sql 로 sql 파일을 하나 더 만들어서 추가하도록 변경하였습니다.

List<StationResponse> stations = getStations();
LineResponse line = getLine(1L);

createLineStation(null, stations.get(0).getId(), line.getId());

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

계속 stations.get을 사용하지 말고 get(0), get(1), get(2)를 변수로 추출해주면 좀더 이해하기 쉽지 않을까요?
StationResponse 강변 = stations.get(0); 이런식으로요

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

테스트 코드의 이해를 돕기 위해 계속 사용하는 값을 변수로 추출해서 재사용 하도록 변경하였습니다. 그러면서 궁금한점이 생겼는데 저는 여기서 사용하는 값이 지하철역이어서 해당 지하철역의 한글 발음을 그대로 영문으로 옮겨 적었습니다. 저는 테스트 코드에서는 이런 식으로 변수명을 지어도 괜찮다고 생각하는데 어떻게 생각하시나요? 😃

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

네 영문으로 써도 문제 없습니다.
협업 할 때 어떻게 할지 잘 정해서 진행하시면 됩니다

giantim and others added 9 commits May 11, 2020 14:06
AdminEdge.js 도착역을 선택하는 옵션 추가
LineResponse 사용하지 않는 임포트 제거
LineService Collections 로 감싸지 않고 unmodifiableList 를 생성해서 반환하도록 변경
refactor: cud 작업에 @transactional 추가

LineService cud 작업에 트랜잭셔널 어노테이션 추가
StationController StationRepository -> StationService 를 사용하도록 변경
StationRepository List 컬렉션으로 Station 을 담는 메서드 오버라이드
StationResponse 기본 생성자 접근자 private 로 변경, 코드를 읽기 좋게 메서드 위치 변경
StationService 추가
Line 메서드 위치 조정, 데이터베이스와 칼럼명 매핑
LineAcceptanceTest backgroundColor 값 전달
LineRequest 메서드 위치 조정, 생성자 private 으로 변경, 프로퍼티명 축약하지 않도록 변경
LineStation 데이터베이스와 칼럼명 매핑, 생성자 private 으로 변경, 사용하지 않는 생성자 삭제
LineStationAcceptanceTest 공통으로 사용하는 값 추출
schema.sql Line_Station 테이블 칼럼명 변경
LineStationController LineStationResponse 를 생성할 때 PathVariable 을 이용하도록 변경
LineStationCreateRequest lineId 프로퍼티 제거
LineStationResponse lineId 를 직접 받아서 LineStationResponse 생성하도록 변경
LineResponse Set<Station> -> Set<StationResponse> 로 변경
LineService LineResponse 를 생성할 때 dto 를 이용하도록 변경
@giantim
Copy link
Author

giantim commented May 11, 2020

구구 안녕하세요! 리뷰 주신 내용 감사합니다!
피드백 주신 내용 반영했습니다 😃

Request 클래스를 설계한 부분에 대해 질문 주신 내용에 답 남겼습니다 :)
그리고 피드백을 반영하던 중 질문이 두 가지 생겨서 댓글로 남겼습니다.
질문이 있는 부분은 LineStation 을 추가하는 로직Line 의 backgroundColor 를 enum 으로 관리 하는 부분입니다.
감사합니다!

lineRepository.saveAll(lines);

Assertions.assertThat(lineRepository.existsByName("신분당선")).isTrue();
Assertions.assertThat(lineRepository.existsByName("3호선")).isFalse();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 테스트가 깨지네요
쿼리를 보니 아예 값이 없으면 true, false 판단 할 수 없어서 생긴 문제로 보이네요
count를 활용해보면 어떨까요?

List<StationResponse> stations = getStations();
LineResponse line = getLine(1L);

createLineStation(null, stations.get(0).getId(), line.getId());

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

네 영문으로 써도 문제 없습니다.
협업 할 때 어떻게 할지 잘 정해서 진행하시면 됩니다

Copy link

@kang-hyungu kang-hyungu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

라빈 피드백 반영 잘 하셨어요 👍
몇 가지 간단한 추가 피드백 남겼어요
확인해보시고 리뷰 재요청 주시면 빠르게 반영할게요
추가로 궁금한 사항은 코멘트 남기거나 dm 주세요!

}

@DeleteMapping("/{id}")
public ResponseEntity<LineResponse> deleteLine(@PathVariable Long id) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

api 반환하는 타입이 없으니 ?나 Void로 표시하는게 좋겠네요

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

모든 컨트롤러에서 반환 타입이 없는 부분을 Void 로 표기를 변경하였습니다


@PostMapping
public ResponseEntity createLines(@RequestBody Request<LineRequest> view) {
Line line = view.getContent().toLine();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

프론트 개발자와 협의해서 정하면 문제 없는 코드죠 👍

this.name = name;
this.startTime = startTime;
this.endTime = endTime;
this.intervalTime = intervalTime;
this.backgroundColor = "bg-blue-500";

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

하드코딩 대신 상수로 빼서 기본 색상이라는 표시를 해주는게 좋겠네요

import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.ValueSource;

public class LineTest {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line 객체에 대한 테스트를 좀더 보강해보면 좋겠네요
update, findLineStationsId 도 의도대로 동작하는지 테스트 추가해보아요
예외 케이스에 대한 테스트도 추가하면 좋겠어요

giantim and others added 3 commits May 15, 2020 01:10
LineRepositoryTest 실패하는 테스트 케이스 수정
Controller 반환 타입이 없는 경우 Void 를 ResponseEntity 에 담도록 변경
Line 기본 색상을 상수로 추출
LineTest 퍼블릭 메서드에 대한 검증 테스트 코드 추가
refactor: 2차 피드백 반영
Copy link

@kang-hyungu kang-hyungu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

라빈 피드백 반영한 내용 확인했어요
코드 반영 잘 하셨어요 :)
이번 미션 pr은 머지 처리할게요
코드 구현하느라 수고하셨어요!

@kang-hyungu kang-hyungu merged commit e409780 into woowacourse:giantim May 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants