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단계 - 자동차 경주 리팩토링] 성하(김성훈) 미션 제출합니다. #624

Merged
merged 3 commits into from
Feb 13, 2023

Conversation

sh111-coder
Copy link

1단계에서 InputView단에 있던 자동차 이름 검증 로직을
도메인 로직에 옮겨 리팩토링하게 되었습니다.

리팩토링 중에 '분리한 자동차 이름이 중복되는 경우 예외 처리' 부분은
RacingCars에서 처리하기가 힘들어서 InputViewValidator에 남겨두게 되었습니다!

리뷰 남겨주시면 감사하겠습니다!

@sh111-coder
Copy link
Author

그리고 RacingCars 멤버 변수를 선언하는 것에 대해서
스레드 키워드로 생각해보라고 하셨는데
동시성때문에 thread-unsafe 해서 그런 것이 맞을까요?

외부 사용자 A, B가 있고
A가 애플리케이션을 사용할 때 RacingCars를 'A'로 변경하고 사용하던 중에,
B 사용자가 애플리케이션을 사용해서 RacingCars를 'B'로 변경했다면
A는 RacingCars가 'A'인줄 알고 사용하는데 실제로는 B의 변경으로 'B'로 사용돼서 그런 걸까요?

제가 이런 부분은 겪어보지 못해서 잘 모르는데 이런 상황 예시가 적절한 건가요?!

Copy link
Member

@begaonnuri begaonnuri left a comment

Choose a reason for hiding this comment

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

안녕하세요 성하!

1단계때 충분히 많은 개선을 해주셔서 2단계땐 큰 피드백 없이 바로 머지하겠습니다. 고생하셨어요 💯

질문 주신 부분은 코멘트로 남기겠습니다

Comment on lines +32 to +34
private boolean isBlankName(String carName) {
return carName.equals("");
}
Copy link
Member

Choose a reason for hiding this comment

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

String.isBlank를 사용할 수도 있을것같아요.

그리고 이런 경우엔 "".equals(carName)과 같이 사용해야 NPE를 피할 수 있답니다!

Copy link
Author

Choose a reason for hiding this comment

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

오 String에 isBlank() 가 따로 존재하는지 몰랐는데
항상 있을 법한 기능은 찾아보는 습관을 들여야겠네요! 감사합니다!

NPE도 예상을 못한 부분인데 짚어주셔서 감사합니다!

Comment on lines +19 to +26
private void validateName(String name) {
if (isWrongNameLength(name)) {
throw new IllegalArgumentException(CAR_NAME_LENGTH_ERROR);
}
if (isBlankName(name)) {
throw new IllegalArgumentException(SPLIT_CAR_NAME_BLANK_ERROR);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

검증하는 부분을 해당 객체로 옮기셨군요 👍🏻 기존과 어떤점이 다르게 느껴지셨나요?

Copy link
Author

Choose a reason for hiding this comment

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

InputView 단이 아닌 도메인 객체로 옮기니 검증하는 의미가 명확해진 느낌이 들었습니다!
'자동차' 이름에 대한 검증인 만큼 '자동차' 객체가 검증 로직을 가지고 있는 것이 더 객체지향 관점에서 맞는 것 같아요!

해당 어플리케이션 코드를 처음 보는 개발자가 봤을 때에도
'자동차 이름에 대한 검증 로직은 어딨지?' 라고 생각할 때,
저라도 InputView보다는 Car 객체를 먼저 들여다 볼 것 같습니다.

또, InputView에 도메인 검증에 관한 로직을 생성하면 지금은 자동차 객체밖에 없지만,
추후에 도메인이 추가되어 여러 객체가 있을 때 여러 도메인에 관한 검증 로직을 InputView에서 처리하는 것이
단일 책임 원칙에 위배된다는 느낌도 받았습니다!

현재 코드에서 아쉬운 점은, 비즈니스 로직에 검증 로직도 포함되지만
실제 핵심 기능(자동차 객체에서 전진 기능 같은)과 검증 로직이 함께 도메인 객체에 존재한다는 것이 조금 신경이 쓰였습니다.
지금은 핵심 기능과 검증 로직이 몇 가지 되지 않아 코드가 길어지지 않았지만
추후에 확장되어 기능이 많아진다면 복잡해질 것으로 생각이 들었습니다!

우연히 조원의 코드를 보았는데, 똑같이 도메인에 검증 로직을 구현했지만
자동차 이름을 원시값 포장하여 새로운 Name 객체로 만들어서
해당 Name 객체에 검증 로직을 옮긴 것을 봤습니다.
이렇게 하니 제가 신경쓰였던 검증 로직도 핵심 기능 로직과 분리할 수 있었고 원시값도 포장되는 효과가 있더라고요!
다음 미션 도메인 검증 시에는 이와 같은 방법으로 구현해보려고 합니다!!

이러한 깊은 생각을 하게 해주셔서 감사합니다! 몰랐으면 넘어갔을 것 같아요! 😄

@begaonnuri
Copy link
Member

#624 (comment) 에 대한 답변입니다.

말씀해주신 경우가 맞습니다! 지금엔 이런게 있다 정도로만 넘어가시고 나중에 웹 미션을 진행하시면서 더 자세히 알아보시면 좋을것같아요 👍🏻

@begaonnuri begaonnuri merged commit a6861ce into woowacourse:sh111-coder Feb 13, 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