-
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
[라빈] 지하철 정보 관리 - 인수 테스트 미션 제출합니다. #25
Conversation
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 지하철 노선에 역을 추가하는 기능 테스트
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 목록으로 해당 역들 불러오기 구현
Atdd mission
refactor: 공통 코드 메서드 추출 Line 공통 부분 메서드로 추출 LineStation LineId 를 모르도록 프로퍼티 변경 LineStationResponse LineId 를 서비스에서 참조하도록 변경 LineService LineId를 LineStationResponse 를 생성할 때 전달하도록 변경 schema.sql 컨벤션, 데이터 타입 변경
AdminController 역 관리, 구간 관리 페이지로 연결하는 라우터 추가
StationCreateRequest 기본 생성자를 추가해서 응답을 받을 수 있도록 변경 index.js request 함수를 body 가 있을때와 없을때를 구분해서 반환값을 다르게 변경 AdminStation.js 역 관리 기능 구현 constants.js 역 관리에서 사용하는 상수 추가
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 테스트 데이터 추가
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.
안녕하세요 라빈 다시 만나서 반갑네요 :)
미션 구현 잘 하셨어요 👏
피드백 남겼으니 확인해보시고 궁금한 점 있으시면 코멘트 남기거나 dm 주세요
new IllegalArgumentException("잘못된 라인 아이디를 입력하였습니다.")); | ||
List<LineStation> lineStations = line.getStations(); | ||
|
||
return Collections.unmodifiableList(lineStations.stream() |
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.
return Collections.unmodifiableList(lineStations.stream() | |
return lineStations.stream() | |
.map(lineStation -> LineStationResponse.of(id, lineStation)) | |
.collect(Collectors.collectingAndThen(Collectors.toList(), Collections::unmodifiableList)); |
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.
Collections 에 변수로 넘겨주던 값을 모두 람다식으로 변경했습니다 :)
@RestController | ||
@RequestMapping("/lines") | ||
public class LineController { | ||
private LineService lineService; |
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.
private LineService lineService; | |
private final LineService lineService; |
final을 붙여주는게 좋겠네요
다른 컨트롤러, 서비스도 멤버 변수들에 final을 붙여주세요
} | ||
|
||
@PostMapping | ||
public ResponseEntity createLines(@RequestBody Request<LineRequest> view) { |
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.
public ResponseEntity createLines(@RequestBody Request<LineRequest> view) { | |
public ResponseEntity<LineResponse> createLines(@RequestBody Request<LineRequest> view) { |
반환하는 ResponseEntity에 타입을 명시해주는게 좋습니다
다른 메서드도 같이 변경해주세요
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.
ResponseEntity 에 메서드의 반환형을 명시적으로 작성했습니다 :)
|
||
@PostMapping | ||
public ResponseEntity createLines(@RequestBody Request<LineRequest> view) { | ||
Line line = view.getContent().toLine(); |
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.
Request 클래스로 감싸면 이런 부분이 불편하지 않을까요?
Request로 감싸면 어떤 장점이 있나요?
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.
저가 페어와 Request 클래스를 설계한 이유는 제공받은 자바스크립 코드에서 값을 넘겨줄 때 다음과 같은 형식으로 넘겨주어서 설계했습니다.
PUT(data) { return { method: "PUT", headers: { "Content-Type": "application/json" }, body: JSON.stringify({ content: data }) };
이 api 를 공통으로 이용하기 위해 content 에서 값을 자동으로 꺼내기 용이하도록 Request 클래스를 설계했습니다.
Request 클래스를 설계한 이유는 자바스크립트의 api 를 모든 컨트롤러에서 사용할 수 있게 변경할 방법을 모르겠어서 제공받은 api 에 맞춰서 설계했습니다 :) Request 를 설계함으로써 생긴 장점은 제공받은 프론트 코드를 변경하지 않고서 작성한 모든 컨트롤러에서 api 를 사용할 수 있다는 점이라고 생각합니다!
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.
프론트 개발자와 협의해서 정하면 문제 없는 코드죠 👍
@@ -18,13 +20,15 @@ | |||
private int intervalTime; | |||
private LocalDateTime createdAt; | |||
private LocalDateTime updatedAt; | |||
private String bgColor; | |||
|
|||
private Set<Station> stations; |
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.
dto에 도메인 클래스가 노출되면 안됩니다.
도메인 클래스 변경 사항이 있을 때마다 dto에 그대로 반영되는 문제가 생깁니다.
StationDto를 추가해주세요
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.
dto 가 도메인에 의존하지 않도록 기존에 작성해두었던 StationResponse
클래스를 이용하여 값을 전달하도록 변경하였습니다.
|
||
@Service | ||
public class LineService { |
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.
트랜잭션 추가도 필요해보이네요
cud 작업할 때는 꼭 Transactional 어노테이션을 추가해주세요
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.
설계한 서비스에서 cud 작업이 발생하는 부분에 트랜잭션 어노테이션을 모두 추가해 주었습니다!
public void deleteLineById(Long id) { | ||
lineRepository.deleteById(id); | ||
} | ||
|
||
public void addLineStation(Long id, LineStationCreateRequest request) { | ||
// TODO: 구현 | ||
Line line = lineRepository.findById(id).orElseThrow(() -> |
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.
lineRepository.findById(id).orElseThrow
이 패턴이 반복적으로 보이니 메서드 하나로 묶어주면 중복을 줄일 수 있겠네요
|
||
@Service | ||
public class LineService { | ||
private LineRepository lineRepository; |
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.
private LineRepository lineRepository; | |
private final LineRepository lineRepository; |
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.
빈으로 사용하는 레포지토리 객체에 모두 final
키워드를 붙여주었습니다 😃
src/main/resources/schema.sql
Outdated
); | ||
|
||
insert into station values(1, '수원', CURRENT_TIMESTAMP()); |
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.
schema를 정의하는 쿼리만 남기고 데이터 추가하는 쿼리는 따로 분리해주세요
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.
data 를 추가하는 구문은 data.sql 로 sql 파일을 하나 더 만들어서 추가하도록 변경하였습니다.
List<StationResponse> stations = getStations(); | ||
LineResponse line = getLine(1L); | ||
|
||
createLineStation(null, stations.get(0).getId(), line.getId()); |
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.
계속 stations.get을 사용하지 말고 get(0), get(1), get(2)를 변수로 추출해주면 좀더 이해하기 쉽지 않을까요?
StationResponse 강변 = stations.get(0); 이런식으로요
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.
테스트 코드의 이해를 돕기 위해 계속 사용하는 값을 변수로 추출해서 재사용 하도록 변경하였습니다. 그러면서 궁금한점이 생겼는데 저는 여기서 사용하는 값이 지하철역이어서 해당 지하철역의 한글 발음을 그대로 영문으로 옮겨 적었습니다. 저는 테스트 코드에서는 이런 식으로 변수명을 지어도 괜찮다고 생각하는데 어떻게 생각하시나요? 😃
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.
네 영문으로 써도 문제 없습니다.
협업 할 때 어떻게 할지 잘 정해서 진행하시면 됩니다
AdminEdge.js 도착역을 선택하는 옵션 추가
Atdd mission modify
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 를 이용하도록 변경
구구 안녕하세요! 리뷰 주신 내용 감사합니다!
|
lineRepository.saveAll(lines); | ||
|
||
Assertions.assertThat(lineRepository.existsByName("신분당선")).isTrue(); | ||
Assertions.assertThat(lineRepository.existsByName("3호선")).isFalse(); |
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.
이 테스트가 깨지네요
쿼리를 보니 아예 값이 없으면 true, false 판단 할 수 없어서 생긴 문제로 보이네요
count를 활용해보면 어떨까요?
List<StationResponse> stations = getStations(); | ||
LineResponse line = getLine(1L); | ||
|
||
createLineStation(null, stations.get(0).getId(), line.getId()); |
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.
네 영문으로 써도 문제 없습니다.
협업 할 때 어떻게 할지 잘 정해서 진행하시면 됩니다
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.
라빈 피드백 반영 잘 하셨어요 👍
몇 가지 간단한 추가 피드백 남겼어요
확인해보시고 리뷰 재요청 주시면 빠르게 반영할게요
추가로 궁금한 사항은 코멘트 남기거나 dm 주세요!
} | ||
|
||
@DeleteMapping("/{id}") | ||
public ResponseEntity<LineResponse> deleteLine(@PathVariable Long 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.
api 반환하는 타입이 없으니 ?나 Void로 표시하는게 좋겠네요
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.
모든 컨트롤러에서 반환 타입이 없는 부분을 Void
로 표기를 변경하였습니다
|
||
@PostMapping | ||
public ResponseEntity createLines(@RequestBody Request<LineRequest> view) { | ||
Line line = view.getContent().toLine(); |
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.
프론트 개발자와 협의해서 정하면 문제 없는 코드죠 👍
this.name = name; | ||
this.startTime = startTime; | ||
this.endTime = endTime; | ||
this.intervalTime = intervalTime; | ||
this.backgroundColor = "bg-blue-500"; |
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.
하드코딩 대신 상수로 빼서 기본 색상이라는 표시를 해주는게 좋겠네요
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 { |
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.
Line 객체에 대한 테스트를 좀더 보강해보면 좋겠네요
update, findLineStationsId 도 의도대로 동작하는지 테스트 추가해보아요
예외 케이스에 대한 테스트도 추가하면 좋겠어요
LineRepositoryTest 실패하는 테스트 케이스 수정 Controller 반환 타입이 없는 경우 Void 를 ResponseEntity 에 담도록 변경 Line 기본 색상을 상수로 추출 LineTest 퍼블릭 메서드에 대한 검증 테스트 코드 추가
refactor: 2차 피드백 반영
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.
라빈 피드백 반영한 내용 확인했어요
코드 반영 잘 하셨어요 :)
이번 미션 pr은 머지 처리할게요
코드 구현하느라 수고하셨어요!
구구 안녕하세요! 오랜만에 다시 리뷰 요청 드립니다!
구간을 추가할 때 지하철 역 이름을 직접 입력해야 하고 시작 역을 등록할때는 이전역을
출발역
이라고 직접 기재해야 하는 불편함이 있습니다....많은 지적, 피드백 부탁드립니다 감사합니다 😊