[2단계 - 경로 조회 기능] 성하(김성훈) 미션 제출합니다. #142
Conversation
|
|
||
| @Service | ||
| @Transactional | ||
| public class StationSaveService { |
There was a problem hiding this comment.
Q : StationService 네이밍
현재 StationService에서 Line, Section, Station을 전부 사용하고 있습니다!
그래서 네이밍을 모든 개념을 포함하는 SubwayService로 하려고 하니,
LineService나 PathService 등 모든 비즈니스 로직을 SubwayService가 담당해야 할 것 같았습니다.
그래서 그냥 StationService로 놔두게 되었는데, 좋은 방법이 있을까요?
There was a problem hiding this comment.
Station 이 Section과도 연관관계가 있고 Line과도 연관관계가 있기 때문에
세 가지 repository가 모두 필요할 수 밖에 없는데요
네이밍은 이 클래스가 무엇을 사용하는지 보다는 역할이 무엇인지에 초점을 맞추는 게 좋다고 생각해요
현재 이 서비스는 어떤 역할을 하고 있나요?
There was a problem hiding this comment.
역을 등록하는 역할을 하고 있습니다!!
역을 등록하는 역할을 수행하기 위해 Line, Section, Station를 사용한다고 이해하면
StationSaveService도 괜찮은 것 같아요!
제가 처음에 고민했던 부분은 도메인 이해가 없는 다른 개발자가 봤을 때,
StationSaveService라고 하면 Station에 관한 비즈니스 로직만 처리할 것이라고 생각할 것 같아서 고민이었습니다.
이러한 부분은 상관하지 않아도 되는 부분일까요??
There was a problem hiding this comment.
제가 처음에 고민했던 부분은 도메인 이해가 없는 다른 개발자가 봤을 때,
StationSaveService라고 하면 Station에 관한 비즈니스 로직만 처리할 것이라고 생각할 것 같아서 고민이었습니다.
이러한 부분은 상관하지 않아도 되는 부분일까요??
네! 오히려 Station 만 사용하기 위해 복잡한 join, stream 등의 로직을 가져가는 것보다
가독성 좋은 코드가 동료 개발자에게 더 도움이 되지 않을까요?
| import subway.exception.StationNotFoundException; | ||
|
|
||
| @Repository | ||
| public class StationRepository { |
There was a problem hiding this comment.
Q : 현재 구조에서 Repository를 가지는게 효과가 있을까요?
처음에 Repository를 만들 때는 주변에서 Repository로 Service 로직이 엄청 줄었다! 라고 하길래
일단 만들어보게 되었습니다.
만들고 보니 저는 도메인 엔티티를 사용해서 변환 로직이 없었고, 단순히 Dao 로직을 호출하기만 하는
bypass 로직이 거의 대부분이 되었습니다.
사소하지만, 현재 구조에서 Repository를 생성해보고 느낀 장점은 다음과 같았습니다.
-
Dao는 DB에 종속적인 메소드 네이밍을 하기 때문에, Service 단에서 사용 시 조금 이질감이 있었습니다.
ex)stationDao.insert()
이를, Repository를 사용하니 메소드 네이밍을 좀 더 비즈니스 로직을 담는 네이밍을 사용할 수 있었습니다.
ex)stationRepository.saveStation() -
NotFound에 대한 예외 검증을 Repository 계층에서 수행할 수 있었습니다.
기존의 Service -> Dao에서는 Dao에서 데이터를 가져올 때, 없으면 예외를 발생시키는 것이 조금 어색했습니다.
그래서 Service단에 NotFound 검증이 위치했었는데, 비즈니스 로직과 혼재해 있어서 결이 다르다고 생각했는데,
Repository로 분리하니 깔끔해진 것 같습니다!
이러한 장점들을 느꼈는데, 조금 사소하다고 느껴지긴 해서 bypass하는 단점이 더 크게 느껴지는 것 같습니다!
검증을 처리하는 것은 확실히 좋다고 느꼈는데, 현재 구조에서 Repository를 가지는게 효과가 있을까요?
There was a problem hiding this comment.
처음에 Repository를 만들 때는 주변에서 Repository로 Service 로직이 엄청 줄었다! 라고 하길래
일단 만들어보게 되었습니다.
이 고민의 문제는 스스로 필요성을 느끼지 못해서인 것 같은데요
누군가 그게 좋다고 해서, 혹은 다들 그렇게 하니까 보다는
스스로 필요성을 느꼈을 때 해당 기술을 도입하는 것이 학습에 가장 효과적이라고 생각해요
이렇게 하면 적용 전후의 차이와 장단점도 명확히 이해할 수 있고 더 오래 기억하게 되거든요
말씀해주신 예외 검증의 책임이 명확하게 분리된 점은 분명한 장점이 맞지만
크게 장점을 느끼지 못했다면 필요성이 느껴질 때 다시 분리를 고민해봐도 좋을 것 같습니다
There was a problem hiding this comment.
이 고민의 문제는 스스로 필요성을 느끼지 못해서인 것 같은데요
맞습니다!! 스스로 필요성을 느끼지 못하고 적용해서 그랬던 것 같아요!
좋은 답변 감사합니다 ㅎㅎㅎ 😃
비슷한 맥락으로 브라운의 강의에서도 들었었는데, 어떤 기술을 사용하고 싶어서 사용하지 말고
'어떤 기술이 도움이 될까?' 라는 생각을 가지고 기술을 사용하라고 하셨었습니다!!
바다의 답변을 듣고 다시 생각해보게 되었어요! 앞으로는 필요성을 느끼고 사용해보겠습니다!
++Repository의 검증 책임을 서비스로 옮기려고 하다보니 Service 코드가 DB 검증까지 들어가서 맘에 안 드는 것 같아요!
그래서 Repository가 예외 검증의 책임을 가지는 것만으로도 의미를 가진다는 것을 느끼고 놔두게 되었습니다!
| import subway.exception.SectionNotFoundException; | ||
|
|
||
| @Repository | ||
| public class GeneralSectionRepository { |
There was a problem hiding this comment.
Q : 하나의 Repository가 여러 Dao를 가져도 될까?
이전에 Repository를 공부했을 때도 그렇고, 주변 크루들도 대부분 Repository에 여러 Dao를 가진 것을 볼 수 있었습니다.
저도 이번에 Section을 영속화 시킬 때 Station들이 삭제되거나, 추가되는 등의 작업이 필요했는데
해당 작업을 SectionRepository에서 SectionDao, StationDao를 가지고 수행하려고 했습니다.
하지만, SectionRepository가 StationDao를 가지고, Station에 관한 CRUD를 하는 것은
단일 책임 원칙을 위배한다고 생각이 들었습니다.
그래서 하나의 도메인 관련 Repository는 해당 도메인의 Dao만 가지고,
여러 도메인 Repository를 Service가 비즈니스 로직을 처리하기 위해 호출하여
메시지를 뎐져 받아온 값으로 조합하도록 했습니다.
하나의 Repository가 여러 Dao를 가지는 것은 단일 책임 위배가 아닌 걸까요?
바다의 생각이 궁금합니다!!
There was a problem hiding this comment.
Repository와 Dao가 1:1로 매핑되는 것이 아주 이상적이지만
현업에서는 도메인 구조가 아주 복잡하기 때문에 거의 불가능하긴 합니다 😂
이런 점들을 지키는 것도 중요하지만 개발은 협업이기 때문에
코드의 가독성과 정해진 일정, 성능 등을 함께 고려한다면
극단적으로 지키기 위해 고민할 필요는 없을 것 같아요 ㅎㅎ
지금은 학습 단계이니 아주 이상적인 클린코드를 해보겠다 ! 하시는 것도 좋습니다 👍🏻
There was a problem hiding this comment.
아하!! 그렇군요! 친절한 답변 감사합니다!! 😃
그래도 제 생각이 이상적인 구조였다는 것에 만족합니다!!
그렇다면, 할 수만 있다면 1:N 매핑이 되는 것은 단순히 편의성을 위한 것일까? 지양해야 할까?라는 질문이 생겼습니다!!
제가 처음에 생각했던 건 Repository와 Dao가 1:1로 매핑되는 것과 1:N으로 매핑되는 것은
'단일 책임을 어디까지로 보는지에 관한 관점'의 차이라고 봤습니다!
1. Repository와 Dao가 1:1로 매핑되는 것
여기서의 관점은 첫 질문에서 남겼던 것처럼
저는 하나의 Repository는 해당 도메인만 관리하는 것을
책임으로 보는 것으로 생각해서 해당 구조를 사용하게 되었습니다.
2. Repository와 Dao가 1:N으로 매핑되는 것
1:N 매핑을 생각해봤을 때, 해당 관점은 해당 도메인을 영속화하기 위해 다른 도메인의 Dao도 필요하므로
'도메인을 영속화'하는 책임으로 보면 단일 책임을 위배한 것이 아니다!
라는 생각을 했었는데요.
이러한 관점에서 보면 1:N 매핑을 사용하는 것도 충분히 납득이 되는 것 같습니다!
그래서 할 수만 있다면 1:1 매핑을 지향하는 것이 좋을지,
아니면 1:N도 편의성 뿐만 아니라 Repository의 책임을 수행하는 것이기 때문에 지향해도 괜찮을지 궁금합니다!!
요약하면 제가 생각한 관점에서 1:1과 1:N 매핑은 취향차이로 보였었는데,
1:1이 이상적으로 좋은 건지 궁금합니다!!
There was a problem hiding this comment.
제 이상적이라는 표현이 조금 혼란을 드린 것 같네요 😂
이상적이라고 해서 좋다거나 정답이라는 뜻은 아니었구요
극단적으로 모든 객체가 한 개의 책임만 갖는 구조라는 뜻에서 말씀드렸습니다
| @@ -6,35 +6,74 @@ | |||
|
|
|||
| - [x] POST '/stations' uri 맵핑 | |||
There was a problem hiding this comment.
Q: 처음 애플리케이션 코드를 접했을 때, 어떻게 흐름을 파악하는지 궁금합니다!
이번 지하철 미션이 정말 비즈니스 로직이 복잡하다고 생각이 들었습니다!
바다는 현업에서 이미 있는 애플리케이션의 복잡한 코드를 볼 때, 어떤 것들을 보면서 흐름을 파악하시고,
이해하시는지 궁금합니다!
비즈니스 로직이 담겨있는 서비스 프로덕션 코드는 규모가 크다면 코드 자체가 엄청 많을 것 같은데,
그럴 때는 이해하기 좋은 네이밍도 있는 테스트 코드를 보면서 이해하는지, 다른 방법이 있는지 궁금합니다!!
There was a problem hiding this comment.
복잡한 요구사항이 아닌 복잡한 코드를 이해하는 방법에 대한 질문을 주신거죠?
대부분의 경우는 잘 정리된 문서들을 통해 최대한 큰 맥락을 파악하는 편입니다.
여기서 문서란 API 명세, 데이터 간 관계를 파악할 수 있는 ERD,
혹은 전체 구조를 파악할 수 있는 아키텍처 설계 그림 등이 될 수 있어요
그래서 문서화가 중요한 것이죠
한 번에 모든 코드를 이해하는 것은 거의 불가능하기 때문에
현재 진행중인 업무와 관련된 부분을 이해하고 다음 업무를 받았을 때 또 다른 부분을 파악해두고..
이런식으로 진행하다 시간이 지나면 어느 정도 전체 그림을 볼 수 있게 됩니다 👏
There was a problem hiding this comment.
복잡한 요구사항이 아닌 복잡한 코드를 이해하는 방법에 대한 질문을 주신거죠?
맞습니다!! 질문을 더 이해할 수 있게 작성해보도록 하겠습니다!! 😃
정성스러운 답변 주셔서 감사합니다!! ㅎㅎ 😁
그래서 문서화가 중요한 것이죠
☺️
역시 문서화가 중요하군요!!
확실히 코드를 직접 보는 것 보다는 이해하기 쉬운 문서화된 것과 코드를 같이 보는게 이해하기 쉬울 것 같아요!
답변을 보고 생긴 질문이 있습니다!!
Q: 바다가 경험했던 API 명세, ERD, 아키텍처 설계 그림 등을 나타내는 좋았던 툴을 소개해주실 수 있을까요?!
경험해보고 싶습니다!
There was a problem hiding this comment.
바다가 사용했던 툴들을 적어주셔서 감사합니다!!
ERD Cloud는 이번에도 사용했었는데, draw.io는 들어만 보고 사용을 못해봤던 것 같아요!
꼭 다음에 사용해보도록 하겠습니다!!
| private final Logger log = LoggerFactory.getLogger(getClass()); | ||
|
|
||
| @ExceptionHandler(IllegalArgumentException.class) | ||
| public ResponseEntity<String> handleIllegalArgumentException(IllegalArgumentException exception) { |
There was a problem hiding this comment.
Q : IlleagalArgumentException, NotFoundException 같은 사용자 예외 로그 질문
사소하지만 사용자 예외 (400번대)들은 로그를 남겨야 할지에 대한 고민이 들었습니다!
사용자 예외를 로그로 남겨서 추적하는 일이 많아서 로그를 남겨야 하는지 궁금합니다!!
There was a problem hiding this comment.
벌써 로깅에 대해 고민해보셨군요 !
이 고민은 로그 레벨에 대해 학습해보시면 조금 실마리가 잡힐 것 같아요
참고할 수 있는 글을 첨부해드릴게요
There was a problem hiding this comment.
헉 도움되는 링크 감사합니다!! 😃
일단 에러 확인 뿐만 아니라 통계적인 부분에서도 로그를 남기는 것이 좋다는 것을 알게 되었어요!
그런데 1가지 질문이 생겼습니다!!
해당 링크에서는 사용자 예외들을 서비스, 도메인의 시나리오 상태 (동작 확인 용도)로 INFO 레벨로 사용하는 것 같습니다!
얼마전에 효율적으로 로그 모니터링하기 글을 본적이 있었는데요!!
해당 글에서는 사용자 입력 에외 등을 WARN 레벨로 사용하는 것 같아요!
제가 정리해본 두 글의 관점은 다음과 같습니다!
✅ 1. INFO 레벨로 사용
- 명확한 목적을 가지는 로그 레벨은 ERROR, INFO이다.
- 로그 레벨을 잘 활용하기 어렵다면 ERROR, INFO만 사용하자.
- 둘을 나누는 기준은 '개발자의 의도'이다.
- 사용자 입력 오류 부분은 개발자가 의도적으로 예외 처리를 해주는 것이므로 예상치 못한 예외가 아닌, 의도적인 예외이므로 ERROR가 아닌 INFO가 적절하다.
✅ 2. �WARN 레벨로 사용
- 모든 로그 레벨을 세분화해서 설정한다.
- INFO는 정상 작동의 경우만 설정하는 것이 좋다.
- 그래서 예외 상황은 ERROR, WARN을 사용한다.
- 둘을 나누는 기준은 '개발자가 제어가 가능한가?' 이다.
- 개발자가 제어할 수 없는 사용자 입력 예외 같은 경우에는 WARN을 사용하고, 개발자가 조취를 취할 수 있는, 취해야 하는 오류, 예외는 ERROR를 사용한다.
이처럼 로그 레벨 부분도 뭐가 맞다, 틀리다가 아닌 취향 차이인 것 같은데,
저는 INFO로 사용하면 정상 작동으로 느껴져서, WARN을 사용하는 것이 더 적절하다고 생각합니다!
바다는 사용자 입력 예외에서 어떤 로그 레벨을 사용하시는지 궁금합니다!!
There was a problem hiding this comment.
저는 INFO로 사용하면 정상 작동으로 느껴져서, WARN을 사용하는 것이 더 적절하다고 생각합니다!
바다는 사용자 입력 예외에서 어떤 로그 레벨을 사용하시는지 궁금합니다!!
로그 레벨은 회사마다, 팀마다 전부 다르게 적용하고 있기 때문에 정답이 없습니다.
일반적으로 프로그램을 단계별로 테스트하기 위해 sandbox, cbt, prod 와 같은 phase 들을 두는데요
페이즈 별 로깅 레벨을 다르게 설정해서 불필요한 로그가 수집되지 않도록 관리해줄 수 있습니다.
(로그가 쌓이면 비용이 되기 때문에 적정수준의 로깅을 하는 것도 중요하거든요)
지금은 비용걱정 없이 학습을 하는 단계이니 많은 로그를 남겨보시는 것도 좋은 경험이 될 것 같아요
딱 Logback 에 대해서 학습하기 좋은 타이밍인 것 같습니다 ㅎㅎ
There was a problem hiding this comment.
오!! phase 부분은 몰랐던 부분이었는데 바다 덕분에 알아갑니다!!
Logback에 대해서도 학습해볼게요! 감사합니다 ㅎㅎ 👍
| import subway.domain.section.general.NearbyStations; | ||
| import subway.domain.station.Station; | ||
|
|
||
| public class GeneralSectionFixtures { |
There was a problem hiding this comment.
Q: TestFixture 사용 질문
TestFixture를 이번에 사용하게 되었습니다!
사용하게 된 계기는, 테스트를 짜다가 불편해서 사용한 것이 아니라
테스트를 짜기 전에 이건 Fixture를 쓰면 편해지겠다! 라고 생각해서 사용하게 되었는데요.
다 짜고 보니, 뭔가 클래스도 많아지고 테스트를 짜다가 불편해서 생성한 것이 아니라서
코드 중복이 얼마나 줄었는지 체감도 되지 않아서 '이게 맞는건가?' 싶었습니다!
현재 Fixture 구조가 괜찮은지, 어느 정도로 사용해야 하는지 궁금합니다!!
There was a problem hiding this comment.
TestFixture를 잘 사용하면 테스트를 아주 빠르고 효율적이게 작성할 수 있게 되지만
잘못 사용하게 되면 운영코드의 변경사항이 발생할 때마다 테스트 코드를 수정하는 시간이 더 걸리게 되죠 😂
너무 많은 데이터를 미리 만들어두면 테스트간 결합도가 너무 높아지고
또 안 만들자니 중복코드가 많이 발생하게 되어 처음엔 이 적정선을 찾기가 어려울텐데요
위에서도 말씀드렸지만 직접 경험해보면서 찾아가는 것이 최고입니다 ㅎㅎ
픽스쳐야말로 정말 정답이 없고 본인의 취향껏 정의하면 되는 부분이기 때문에
어느 정도로 사용해야 한다 ! 라는 것은 없어요
이번 미션에서 Fixture를 사용해보신 것만으로도 일단 훌륭합니다 💯
There was a problem hiding this comment.
잘못 사용하게 되면 운영코드의 변경사항이 발생할 때마다 테스트 코드를 수정하는 시간이 더 걸리게 되죠 😂
이 부분도 이번에 경험해본 것 같아요 😭
구현하면서 구현-리팩토링 하는 부분이 많았는데 그 때마다 커밋 로그에 Fixture가 있게 되었습니다 😂
제대로 Fixture를 사용한 적은 이번이 처음이어서 바다가 말해주신대로 사용해보면서 적정선을 찾아가야 할 것 같아요!
칭찬해주셔서 감사합니다 ㅎㅎㅎ 😁
|
|
||
| ### 5. 최단 경로 조회 API | ||
|
|
||
| - [x] GET '/sections/short-path?startLine=&startStation=&endLine&endStation= |
There was a problem hiding this comment.
Q: Query String 개수 질문
경로 조회 API에서 Query String으로 4가지 정보를 주고 있는데, URL이 길어져서 조금 찝찝한 느낌인데, 괜찮은건지 궁금합니다!
극단적으로 GET 메소드에 Query String 파라미터가 한 10개가 되어도 Request Body에 담지 않고,
Query String을 사용해도 문제가 없는지 궁금합니다!!
There was a problem hiding this comment.
query string 개수에 대해서 정해진 것은 없지만 길이가 길어지면 휴먼에러가 발생할 수 있기 때문에
Body로 담아 보내주는 것이 좋아요 이후 협업미션을 통해 학습하시겠지만
어떤 uri가 좋을지 잘 모르겠다면 프론트 개발자와 협의해서 진행하시면 됩니다
이 경우는 query string의 개수보다는 타입에 대한 피드백을 조금 드리고 싶은데요
한글로 노선의 이름을 받는 것보다 노선의 PK 인 id 값을 받아주면 더 명확한 의사 전달이 될 것 같습니다 :)
There was a problem hiding this comment.
제가 이번 경로 조회 API에서 Query String과 노선의 이름을 받은 이유는 다음과 같았습니다!
-
HTTP 스펙에서 GET 메소드는 일반적으로 Request Body를 가지지 않는 것으로 배웠습니다!
그래서 정보가 많아도, Request Body 대신 Query String이나 PathVariable을 사용하도록 했습니다. -
PathVariable 대신 Query String을 사용한 이유
PathVariable은 리소스가 계층 구조일 때 사용하는 것이라고 배웠습니다!
/members/{memberId}처럼 상위 계층에 하위 리소스가 속해있을 때 해당 리소스를 '/' 를 통해 가리킨다고 배웠습니다!
경로 조회 시에는 출발 노선 이름, 역 이름과 도착 노선 이름, 역 이름을 리소스로 요청하는데,
해당 관계가 계층 관계는 아니라고 생각하여 Query String을 사용했습니다. -
PK인 id 대신 이름으로 설계한 이유
최단 경로 조회 API가 호출되는 시나리오를 생각해봤을 때,
사용자가 출발 노선과 역, 도착 노선과 역을 입력하는 상황이 자연스럽다고 생각했습니다.
사용자 입장에서 출발 노선과 역, 도착 노선과 역을 입력할 때 역과 노선의 id는 모르기 때문에
이름을 입력하는 것이 자연스럽다고 생각해서 이름을 입력하도록 했습니다.
그래서 바다의 답변을 듣고 질문이 생겼습니다!!
query string 개수에 대해서 정해진 것은 없지만 길이가 길어지면 휴먼에러가 발생할 수 있기 때문에
Body로 담아 보내주는 것이 좋아요
이 부분에서 GET 메소드인데도 Request Body로 값을 담아서 보내도 되는지 궁금합니다!
한글로 노선의 이름을 받는 것보다 노선의 PK 인 id 값을 받아주면 더 명확한 의사 전달이 될 것 같습니다 :)
또, 이 부분에서 사용자 입력 시에는 사용자가 역과 노선의 'id'는 모르기 때문에
'이름'을 입력 받는 것이 맞다고 생각하는데요!
URI에 PK인 id로 요청한다는 것은 사용자가 'id'를 알고 있어야 한다고 생각합니다!
그래서 어떻게 요청이 되는건지 흐름이 잘 떠오르지가 않아서 id로 바뀌었을 때
요청 흐름이 어떻게 되는지 간단한 흐름을 설명해주실 수 있을까요?
There was a problem hiding this comment.
이 부분에서 GET 메소드인데도 Request Body로 값을 담아서 보내도 되는지 궁금합니다!
기존에는 GET method에 body가 허용되지 않았는데 지금은 허용이 되는 것으로 알고 있어요
이 부분은 같이 협업하는 분들과 협의를 통해 결정하면 될 것 같습니다 이 글이 도움이 될 것 같아요
이 부분에서 사용자 입력 시에는 사용자가 역과 노선의 'id'는 모르기 때문에
'이름'을 입력 받는 것이 맞다고 생각하는데요!
URI에 PK인 id로 요청한다는 것은 사용자가 'id'를 알고 있어야 한다고 생각합니다!
그래서 어떻게 요청이 되는건지 흐름이 잘 떠오르지가 않아서 id로 바뀌었을 때
요청 흐름이 어떻게 되는지 간단한 흐름을 설명해주실 수 있을까요?
이후 협업 미션을 통해 자연스레 학습하시겠지만 사용자가 직접 url을 입력해서 호출하는 것은 아닙니다.
사용자는 화면에 구성된 어떤 버튼을 통해 자신이 원하는 역을 선택할 수 있구요
이를 프론트에서 해당 역의 id를 넣어 서버에 요청을 보내게 됩니다.
아직은 잘 와닿지 않아도 다음 미션에서 학습하게 될 내용이니 크게 걱정하지 않으셔도 됩니다
There was a problem hiding this comment.
감사합니다!! 프론트랑 소통해서 결정하면 되는 것 같아요!!
id로 보냈을 때 어떻게 하는지도 바다 덕분에 조금은 감을 잡은 것 같습니다!! 감사합니다! 👍
| ## 역 등록, 제거, 조회 테스트 시 DB Situation | ||
| ```java | ||
| `test-schema.sql` & `test-data.sql`을 적용하면 '2호선 & 7호선 / 2호선 - A역 & 2호선 - C역 & 7호선 - A역 / A-C 구간'이 등록된다. | ||
| - 이후 Case들은 TestFixtures를 이용하여 테스트한다. |
|
|
||
| public void removeStationById(Long stationId) { | ||
| Station findStation = stationRepository.findStationById(stationId); | ||
| System.out.println("findStation = " + findStation); |
There was a problem hiding this comment.
앗 이번에 구현이 어려워서 많은 디버깅이 있었는데, 지운다고 지웠는데 놓친 것 같습니다!! 죄송합니다!!
리팩토링 진행했습니다!!
| public void removeLineById(Long lineId) { | ||
| if (lineDao.isNotExistById(lineId)) { | ||
| throw new LineNotFoundException("노선 ID에 해당하는 노선이 존재하지 않습니다."); | ||
| } | ||
| lineDao.deleteById(lineId); | ||
| } |
| List<Line> findLines = lineRepository.findAllLine(); | ||
| Lines lines = new Lines(findLines); | ||
| Optional<Line> findNullableLine = lines.findByLineName(stationRequest.getLineName()); |
There was a problem hiding this comment.
lineRepository 에서 바로 findByName으로 가져와도 될 것 같은데
전부 가져온 뒤 걸러내도록 구성하신 이유는 무엇인가요?
There was a problem hiding this comment.
노선 이름의 중복을 검증하기 위해 Lines를 만들고, 해당 도메인을 거치도록 했습니다!
그런데 생각해보니, LineRepository에서 saveLine을 할 때 한 번에 중복 검증도 수행하면 될 것 같아서
Lines는 삭제하고 해당 방향으로 리팩토링했습니다!!
| List<Station> findStations = stationRepository.findAllStation(); | ||
| StationsByLine stationsByLine = StationsByLine.of(findLines, findStations); | ||
| Line line = findNullableLine.get(); |
There was a problem hiding this comment.
모든 stations 과 lines 들을 순회돌며 Map 으로 매핑시켜주는 연산을 역을 저장할 때마다 수행하는 건가요?
초기에 한 번만 생성하고 이후에는 추가되는 부분만 변경하도록 해줘도 될 것 같습니다
+) stationsByLine 이라는 객체의 역할에 대해서도 생각해보시면 좋을 것 같아요
line이 역들을 가지고 있거나 역이 line에 대한 정보를 갖고 있다면
그때그때 필요한 만큼만 효율적으로 사용할 수 있을 것 같은데요
이 중간 매핑 객체 없이 도메인들끼리 로직을 수행하도록 개선해보는 것은 어떤가요?
There was a problem hiding this comment.
바다의 리뷰를 보고 생각해보니, Station에 Line이 있었는데 불필요한 객체를 생성했던 것 같아요!
그래서 역 저장 시에 stationRepository.findAllStationByLineId()로 노선에 해당하는 역들을 가져오고,
해당 역들로 Stations를 구성하여 이후 로직에서 Stations에서 노선을 가져오도록 수정하고,
StationsByLine의 로직들을 Stations가 수행하도록 리팩토링했습니다!
| import subway.exception.SectionNotFoundException; | ||
|
|
||
| @Repository | ||
| public class GeneralSectionRepository { |
There was a problem hiding this comment.
Repository와 Dao가 1:1로 매핑되는 것이 아주 이상적이지만
현업에서는 도메인 구조가 아주 복잡하기 때문에 거의 불가능하긴 합니다 😂
이런 점들을 지키는 것도 중요하지만 개발은 협업이기 때문에
코드의 가독성과 정해진 일정, 성능 등을 함께 고려한다면
극단적으로 지키기 위해 고민할 필요는 없을 것 같아요 ㅎㅎ
지금은 학습 단계이니 아주 이상적인 클린코드를 해보겠다 ! 하시는 것도 좋습니다 👍🏻
| @@ -6,35 +6,74 @@ | |||
|
|
|||
| - [x] POST '/stations' uri 맵핑 | |||
There was a problem hiding this comment.
복잡한 요구사항이 아닌 복잡한 코드를 이해하는 방법에 대한 질문을 주신거죠?
대부분의 경우는 잘 정리된 문서들을 통해 최대한 큰 맥락을 파악하는 편입니다.
여기서 문서란 API 명세, 데이터 간 관계를 파악할 수 있는 ERD,
혹은 전체 구조를 파악할 수 있는 아키텍처 설계 그림 등이 될 수 있어요
그래서 문서화가 중요한 것이죠
한 번에 모든 코드를 이해하는 것은 거의 불가능하기 때문에
현재 진행중인 업무와 관련된 부분을 이해하고 다음 업무를 받았을 때 또 다른 부분을 파악해두고..
이런식으로 진행하다 시간이 지나면 어느 정도 전체 그림을 볼 수 있게 됩니다 👏
| private final Logger log = LoggerFactory.getLogger(getClass()); | ||
|
|
||
| @ExceptionHandler(IllegalArgumentException.class) | ||
| public ResponseEntity<String> handleIllegalArgumentException(IllegalArgumentException exception) { |
There was a problem hiding this comment.
벌써 로깅에 대해 고민해보셨군요 !
이 고민은 로그 레벨에 대해 학습해보시면 조금 실마리가 잡힐 것 같아요
참고할 수 있는 글을 첨부해드릴게요
| import subway.domain.section.general.NearbyStations; | ||
| import subway.domain.station.Station; | ||
|
|
||
| public class GeneralSectionFixtures { |
There was a problem hiding this comment.
TestFixture를 잘 사용하면 테스트를 아주 빠르고 효율적이게 작성할 수 있게 되지만
잘못 사용하게 되면 운영코드의 변경사항이 발생할 때마다 테스트 코드를 수정하는 시간이 더 걸리게 되죠 😂
너무 많은 데이터를 미리 만들어두면 테스트간 결합도가 너무 높아지고
또 안 만들자니 중복코드가 많이 발생하게 되어 처음엔 이 적정선을 찾기가 어려울텐데요
위에서도 말씀드렸지만 직접 경험해보면서 찾아가는 것이 최고입니다 ㅎㅎ
픽스쳐야말로 정말 정답이 없고 본인의 취향껏 정의하면 되는 부분이기 때문에
어느 정도로 사용해야 한다 ! 라는 것은 없어요
이번 미션에서 Fixture를 사용해보신 것만으로도 일단 훌륭합니다 💯
|
|
||
| ### 5. 최단 경로 조회 API | ||
|
|
||
| - [x] GET '/sections/short-path?startLine=&startStation=&endLine&endStation= |
There was a problem hiding this comment.
query string 개수에 대해서 정해진 것은 없지만 길이가 길어지면 휴먼에러가 발생할 수 있기 때문에
Body로 담아 보내주는 것이 좋아요 이후 협업미션을 통해 학습하시겠지만
어떤 uri가 좋을지 잘 모르겠다면 프론트 개발자와 협의해서 진행하시면 됩니다
이 경우는 query string의 개수보다는 타입에 대한 피드백을 조금 드리고 싶은데요
한글로 노선의 이름을 받는 것보다 노선의 PK 인 id 값을 받아주면 더 명확한 의사 전달이 될 것 같습니다 :)
|
안녕하세요! 바다!! 🌊 1단계 머지되고 2단계 PR까지 리뷰를 못받고 거의 모든걸 고쳐서 PR을 보냈었는데요! 이번 바다의 리뷰와 코멘트를 보고 많은 칭찬과 좋은 피드백을 해주셔서 기분이 너무 좋았습니다! 리뷰 주신 코멘트들을 바탕으로 리팩토링 진행해보았습니다! 리팩토링을 진행하다가 바다가 주신 좋은 답변들 덕분에 질문할 부분이 생겨 코멘트 단에 질문을 남겨놓았습니다! 1. 네이밍 관련 재질문2. Repository와 Dao 매핑 관련 재질문3. API 명세, ERD, 아키텍처 설계 그림 등을 나타내는 좋았던 툴 질문4. 로깅 레벨 관련 재질문5. Query String 관련 재질문1차 PR 시 기록한 선택 기준추가적으로, 1차 PR 부분에 제가 구현 시에 선택한 기준들을 적어놨었는데요! 궁금한 점이 많아서 질문들이 너무 많아진 것 같네요 ㅠㅠ 😭 |
| private StationSaveResponse saveSectionWhenUpStationMiddle(Station savedUpStation, GeneralSection currentSection, | ||
| GeneralSection sectionToSave) { |
| Optional<Line> findLine = lineDao.selectByName(lineToSave.getName()); | ||
| if (findLine.isPresent()) { | ||
| throw new IllegalArgumentException("노선 이름은 중복될 수 없습니다."); | ||
| } |
| assertThat(findLine.get()).usingRecursiveComparison() | ||
| .ignoringFields("id").isEqualTo(INITIAL_Line2.FIND_LINE); |
There was a problem hiding this comment.
usingRecursiveComparison 을 적절하게 잘 활용하시네요 💯
| Line line = findStation.getLine(); | ||
| GeneralSection sectionToSave = NewGeneralSectionMaker.makeSectionToSave(line, newUpStation, newDownStation, newDistance); | ||
| GeneralSection sectionToSave = NewGeneralSectionMaker.makeSectionToSave(newUpStation, newDownStation, newDistance); |
| @@ -6,35 +6,74 @@ | |||
|
|
|||
| - [x] POST '/stations' uri 맵핑 | |||
|
|
||
| ### 5. 최단 경로 조회 API | ||
|
|
||
| - [x] GET '/sections/short-path?startLine=&startStation=&endLine&endStation= |
There was a problem hiding this comment.
이 부분에서 GET 메소드인데도 Request Body로 값을 담아서 보내도 되는지 궁금합니다!
기존에는 GET method에 body가 허용되지 않았는데 지금은 허용이 되는 것으로 알고 있어요
이 부분은 같이 협업하는 분들과 협의를 통해 결정하면 될 것 같습니다 이 글이 도움이 될 것 같아요
이 부분에서 사용자 입력 시에는 사용자가 역과 노선의 'id'는 모르기 때문에
'이름'을 입력 받는 것이 맞다고 생각하는데요!
URI에 PK인 id로 요청한다는 것은 사용자가 'id'를 알고 있어야 한다고 생각합니다!
그래서 어떻게 요청이 되는건지 흐름이 잘 떠오르지가 않아서 id로 바뀌었을 때
요청 흐름이 어떻게 되는지 간단한 흐름을 설명해주실 수 있을까요?
이후 협업 미션을 통해 자연스레 학습하시겠지만 사용자가 직접 url을 입력해서 호출하는 것은 아닙니다.
사용자는 화면에 구성된 어떤 버튼을 통해 자신이 원하는 역을 선택할 수 있구요
이를 프론트에서 해당 역의 id를 넣어 서버에 요청을 보내게 됩니다.
아직은 잘 와닿지 않아도 다음 미션에서 학습하게 될 내용이니 크게 걱정하지 않으셔도 됩니다
|
|
||
| @Service | ||
| @Transactional | ||
| public class StationSaveService { |
There was a problem hiding this comment.
제가 처음에 고민했던 부분은 도메인 이해가 없는 다른 개발자가 봤을 때,
StationSaveService라고 하면 Station에 관한 비즈니스 로직만 처리할 것이라고 생각할 것 같아서 고민이었습니다.
이러한 부분은 상관하지 않아도 되는 부분일까요??
네! 오히려 Station 만 사용하기 위해 복잡한 join, stream 등의 로직을 가져가는 것보다
가독성 좋은 코드가 동료 개발자에게 더 도움이 되지 않을까요?
| private final Logger log = LoggerFactory.getLogger(getClass()); | ||
|
|
||
| @ExceptionHandler(IllegalArgumentException.class) | ||
| public ResponseEntity<String> handleIllegalArgumentException(IllegalArgumentException exception) { |
There was a problem hiding this comment.
저는 INFO로 사용하면 정상 작동으로 느껴져서, WARN을 사용하는 것이 더 적절하다고 생각합니다!
바다는 사용자 입력 예외에서 어떤 로그 레벨을 사용하시는지 궁금합니다!!
로그 레벨은 회사마다, 팀마다 전부 다르게 적용하고 있기 때문에 정답이 없습니다.
일반적으로 프로그램을 단계별로 테스트하기 위해 sandbox, cbt, prod 와 같은 phase 들을 두는데요
페이즈 별 로깅 레벨을 다르게 설정해서 불필요한 로그가 수집되지 않도록 관리해줄 수 있습니다.
(로그가 쌓이면 비용이 되기 때문에 적정수준의 로깅을 하는 것도 중요하거든요)
지금은 비용걱정 없이 학습을 하는 단계이니 많은 로그를 남겨보시는 것도 좋은 경험이 될 것 같아요
딱 Logback 에 대해서 학습하기 좋은 타이밍인 것 같습니다 ㅎㅎ
| import subway.exception.SectionNotFoundException; | ||
|
|
||
| @Repository | ||
| public class GeneralSectionRepository { |
There was a problem hiding this comment.
제 이상적이라는 표현이 조금 혼란을 드린 것 같네요 😂
이상적이라고 해서 좋다거나 정답이라는 뜻은 아니었구요
극단적으로 모든 객체가 한 개의 책임만 갖는 구조라는 뜻에서 말씀드렸습니다
| import subway.domain.section.general.NearbyStations; | ||
| import subway.domain.station.Station; | ||
|
|
||
| public class GeneralSectionFixtures { |
안녕하세요 바다!! 🌊 성하입니다! 🙇🏻♂️
1단계에서는 구현을 많이 못했어서 금방 리팩토링 및 구현하고 2단계 구현하려고 했는데 예상만큼 빨리하지 못했네요 ㅠㅠ 😭
1, 2단계에서 고민했던 부분들을 말씀드리고, 질문들은 코드 단에 코멘트로 달아놓도록 하겠습니다!!
리뷰 잘부탁드립니다 ㅎㅎㅎ 😃
🎯 1. 도메인과 엔티티 분리 or 병합
원래 1차 PR 날렸을 때는 도메인과 엔티티를 분리했었습니다!
그 이유는 도메인과 엔티티라는 개념이 다르다고 느껴서 분리를 한채로 진행을 했었는데요!
결과적으로 지금 코드에서는 도메인과 엔티티를 병합해서 하나의 객체로 사용했습니다!
(따로 용어가 있는지 아직 모르겠어서 일단 도메인 엔티티라고 하겠습니다!)
1-1. 도메인 엔티티 사용 이유
도메인 엔티티를 사용한 이유는, 분리했을 때의 불편함 때문이었습니다.
서비스 계층이 담당하는 책임은 '비즈니스 로직'인데, 비즈니스 로직과 도메인, 엔티티 변환 로직이 섞여 있어서
비즈니스 로직이 한눈에 들어오지 않는 느낌이었습니다.
이러한 문제는 Repository를 만들어서 해결할 수 있다고 생각합니다!
하지만 Repository 자체의 책임을 객체를 CRUD하는 책임으로 생각하는데, 관점에 따라 다르겠지만
변환 로직이 들어가게 되면 마찬가지로 책임이 한 눈에 들어오지 않을 것 같았습니다!
1-2. 도메인 엔티티에 DB 필드를 1:1로 맵핑하지 않고, 객체로 맵핑한 이유
현재 코드에서 GeneralSection을 예로 들면,
처음 합쳤을 때 GeneralSection에는 Line이 아닌 Long 타입 PK인 line_id가 존재했습니다.
Section을 DB에서 가져올 때 join해서 가져오는데
find한 Section(Dao단)은 join 해서 모든 line 정보가 있지만, 도메인 엔티티로 맵핑한 것은 line_id 밖에 없었습니다!
그래서 Service 단에서 find한 Section을 통해 line_name 등 line의 정보를 찾으려고 하면
다시 DB에 쿼리를 날려서 line_name을 가져와야 했습니다.
이를 도메인 엔티티에 Long PK가 아닌 객체를 가지면서 다음과 같은 장점을 느꼈습니다!
SectionDao에서 한 번의 쿼리로
Service에게 Section을 반환해서, Section.getLine().getName()으로 수행을 할 수 있다!
서비스 단에서 한번 Section을 생성하고 난 후에는, Line을 사용하지 않고 생성된 Section의 Line을 생성해서
여러 메소드를 분리할 때 파라미터로 넘겨주던 Line을 제거할 수 있었다!
하지만 다음과 같은 단점도 느끼게 되었습니다!
개인적으로 단점보다는 장점이 성능상으로나 편의상으로나 좋게 느껴져서 사용하게 되었습니다!
🎯 2. 서비스 테스트 슬라이스 테스트 & 통합테스트 사용 기준
보통 이전까지는 서비스 테스트를 Dao와의 의존을 끊어서 단순 비즈니스 로직만 테스트하기 위해서
Dao를 Mocking하여 서비스 계층만 테스트하는 슬라이스 테스트만 진행했습니다.
하지만 이번에 통합 테스트도 진행한 이유는,
서비스 로직에서 Repository를 Mocking해야 했는데,
Mocking할 로직이 많아서 Mocking을 하는 비용이
통합테스트에서 느린 속도 비용보다 더 클 것 같아서 통합테스트를 진행했습니다!
LineService는 Mocking할 Repository 로직이 적었기 때문에 Mocking을 하여 진행하게 되었습니다.
🎯 3. 컨트롤러 단위 테스트(슬라이스 테스트)를 하지 않은 이유
결론적으로 e2e 테스트가 있으므로, 컨트롤러의 단위 테스트는 e2e 테스트에서 검증이 되므로
하지 않아도 괜찮다고 생각했습니다!
서비스나 Dao는 해당 로직을 슬라이스해서 테스트하는 것이 의미가 있었지만
컨트롤러 슬라이스 테스트는 단순히 요청을 해서 응답이 제대로 나오는지를 보기 떄문에
e2e 테스트로 대체가 가능할 것 같아서 하지 않았습니다!
🎯 4. TransferSection 도메인을 추가한 이유
결론적으로는 최단 경로 조회 시 “환승” 정보를 사용자에게 주고 싶어서 추가했습니다.
기존 구조에서는 Station에서 노선이 다른 역은 다른 역으로 취급해서
Station에서 line_id를 FK로 가지고 2호선 잠실역과 8호선 잠실역이 2개의 레코드로 DB에 저장되게 했습니다!
그래서 그래프를 사용할 때, Station을 Key로 하면 2호선 잠실역과 8호선 잠실역을 그래프에서 이어줄 수 없었습니다.
이를 해결하기 위해 Key를 String으로 바꿔서 역 이름으로 이으면 단순하게 해결이 됐었지만,
응답이 '2호선 A - 2호선 C' -> '3호선 C -> 3호선 E' 이런 식으로 환승 구간이 없게 되었습니다.
'2호선 A - 2호선 C' -> '2호선 C - 3호선 C' -> '3호선 C -> 3호선 E' 이렇게 환승 정보를 주고 싶었기 때문에
환승 구간 도메인인 TransferSection을 추가해서 환승 구간과 일반 구간으로 나누고,
추상 클래스인 Section을 생성해서 상속하도록 했습니다.
그래서 그래프의 Value로 추상 클래스인 Section을 넣어서, 경로에 환승 구간도 포함되도록 했습니다!
그런데 하고 보니 응답을 환승 없이 내려주고,
역 이름이 같다면 환승 처리해주는 작업을 프론트에서 하는게 훨씬 비용이 적다는 생각이 드네요..
너무 오버한 생각이었을까요? 😭