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

[2단계 - 경로 조회 기능] 깃짱(조은기) 미션 제출합니다! #134

Merged
merged 31 commits into from
May 24, 2023

Conversation

gitchannn
Copy link

@gitchannn gitchannn commented May 18, 2023

안녕하세요 서브웨이!

이번 미션 통해서 처음부터 도메인 코드가 라이브러리와 강하게 결합하면 어떻게 되는지 잘 배운 것 같습니다.....ㅠ
ㅋㅋㅋ이제 앞으로는 이렇게 만들지 않겠습니다!!!!!

질문은 댓글로 달아놨으니 확인 부탁드려요!
디엠 대답도 잘 해주시고, 항상 피드백도 열심히 달아주셔서 감사합니다!
덕분에 많이 배웠어요

Copy link

@joseph415 joseph415 left a comment

Choose a reason for hiding this comment

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

안녕하세요 깃짱!

구현 잘해주셨네요ㅎㅎ💯

이전단계보다 많이 좋아진것 같아서 좋았습니다! 리뷰 남겼는데 확인 부탁드립니다! 1단계와 마찬가지로 확인이 완료된 리뷰는 resolve conversation 눌러주시면 감사하겠습니다!

README.md Outdated Show resolved Hide resolved
.idea/httpRequests/http-requests-log.http Show resolved Hide resolved
http-request.http Show resolved Hide resolved
src/main/java/subway/dao/LineDao.java Outdated Show resolved Hide resolved
src/main/java/subway/repository/DbLineRepository.java Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved

@Transactional
LineResponse addStation(Long id, SectionCreateRequest request);
public LineResponse addStation(final Long lineId, final SectionCreateRequest request) {

Choose a reason for hiding this comment

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

겹치는 부분이 있어서 메서드로 빼도 좋을 것 같아요.

        final Subway subway = subwayService.findSubway();
        final Line line = lineRepository.findById(lineId);

        final Station upStation = stationRepository.findById(request.getUpStationId());
        final Station downStation = stationRepository.findById(request.getDownStationId());

        if (upStation == null || downStation == null) {
            throw new StationNotFoundException("존재하는 역의 id를 입력해 주세요.");
        }

        final int distance = request.getDistance();

Copy link
Author

Choose a reason for hiding this comment

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

@joseph415

메서드로 빼고 싶어도, 내부에서 사용되는 변수가 한개가 아닌 여러개여서 빼기가 여럽네요...!

Copy link
Author

Choose a reason for hiding this comment

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

스크린샷 2023-05-22 오후 3 17 53

src/main/java/subway/service/PathService.java Show resolved Hide resolved
Copy link
Author

@gitchannn gitchannn left a comment

Choose a reason for hiding this comment

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

질문 남겼습니다!

import javax.validation.constraints.Min;
import javax.validation.constraints.NotNull;

public class PathRequest {
Copy link
Author

Choose a reason for hiding this comment

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

질문!

현재 필드 중 age에 대해서, URL이 /source=1&target=2처럼 오게 될 경우에, 자동으로 성인 요금을 적용하고 싶어서 기본값으로 19를 지정하고 싶었습니다.

여러 가지 방법을 생각해 봤는데,

  1. 생성자 파라미터에 @DefaultValue("19")를 추가
    => 잘 적용이 안되고, NPE가 발생

  2. 현재 방식대로 this.age = age != null ? age : DEFAULT_AGE_VALUE;
    => URL이 들어왔을 때 age 파라미터가 없으면 잘 동작하지 않는 것 같음

  3. age 파라미터를 가지지 않는 생성자를 별도로 추가
    => 마찬가지로 잘 동작하지 않음

어떻게 하면 좋을까요..?

참고: 이 클래스는 현재 사용되고 있지 않지만, 질문을 위해 함께 Push 했습니다.

Choose a reason for hiding this comment

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

제가 해당 api를 만든다면, age를 필수로 받도록 했을 것 같아요ㅎㅎ 아니면, 1번정도로 생각할 것 같습니다. 2,3번의 방식은 상대적으로 나중에 디버깅하기 쉽지 않을 수 있겠다 싶어서요.

dto를 필드를 통해서 request값을 바로 파악하면 좋은데, 뭔가 그런 디폴트값 로직이 숨겨져 있는 느낌이 들어서요.

Copy link
Author

Choose a reason for hiding this comment

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

@joseph415

아하...! 헷갈리지 않게 하려면 명확하게 하는게 저도 좋을 것 같네요..!
그러면 기존에 나이에 관한 요구사항이 없다가, 추가된 경우(현재 2단계에서는 없다가 3단계에서 생긴 것처럼)에는
기존 코드를 크게 변경하면서 기존에 사용하던 내용들이 다 에러가 날텐데 어떻게 하실건가요??

Age를 아예 받지 않는 API, Age를 필수로 받는 API로 나눌 건가요??

Choose a reason for hiding this comment

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

그런 경우라면, age를 nullable하게 해서 하던가, 신규로 api 만들어야할것 같네요!.. 근데, 이렇게 까지 생각하면 현재 깃짱이 만들어주신 것처럼 하는게 저는 베스트라고 생각합니다!

Copy link
Author

Choose a reason for hiding this comment

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

네 감사합니당!!!

src/main/java/subway/service/PathService.java Show resolved Hide resolved
Copy link

@joseph415 joseph415 left a comment

Choose a reason for hiding this comment

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

안녕하세요. 깃짱!

리뷰 잘적용해주셨네요ㅎㅎ.
Path와 PathFinder로 기술 구현부와 명세가 나뉘게 되어서 쫌 더 깔끔해진 것 같습니다!
질문주신 부분은 답변은 남겨두었고, 한번 확인 부탁드려요~ 요구사항은 충분히 만족하신 것 같아서 지하철 미션은 이만 merge하겠습니다.
갑자기 어려워진 지하철 미션하느라 고생 정말 많으셨어요! 레벨2 마지막 미션도 화이팅입니다!


import java.util.List;

public interface Path {

Choose a reason for hiding this comment

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

👍

@joseph415 joseph415 merged commit 85e35c1 into woowacourse:eunkeeee May 24, 2023
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.

None yet

2 participants