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

[로또 - TDD] 3단계 - 로또(2등) #2405

Merged
merged 13 commits into from
May 10, 2022
Merged

[로또 - TDD] 3단계 - 로또(2등) #2405

merged 13 commits into from
May 10, 2022

Conversation

Wu22e
Copy link

@Wu22e Wu22e commented May 7, 2022

안녕하세요 이민님!
로또 미션 3단계 구현후 PR 요청 드립니다!

step3 시작 전에 코멘트 주신 것처럼 stringNumber의 parse를 정적 메서드를 이용하여 커스텀 예외 처리를 진행할 수 있었습니다!
근데 LottoNumber 뿐만 아니라 LottoTotalPrice에서도 똑같은 검증과정을 거치고 있어서 코드의 중복이 발생하였습니다.
그래서 StringNumberUtils 클래스로 분리하여 문자를 파싱하는 메서드를 공통으로 사용하도록 하였습니다.


step3의 경우 2등에 대한 등수가 추가되면서, 기존의 LottoRank 의 일치 숫자 갯수가 5일 경우, 미리 Enum 캐싱을 할때 key 중복이 발생하여 등수에 대한 값을 찾을 때 Enum 값을 탐색하면서 해당 조건에 맞는 등수를 반환하도록 리팩토링 하였습니다.

또, 보너스 볼의 경우 당첨 번호와 중복을 허용하지 않기 때문에 WinningLotto 객체를 통해 당첨 로또에 대한 값들을 생성할때 중복에 대한 유효성 처리를 할 수 있도록 하였습니다. (Controller에서 단순히 중복 비교를 하여 예외처리를 하려고 했지만, 필요한 도메인 객체가 있다면 해당 객체로 넘겨서 책임을 분리하고자 했습니다.)

추가적으로 불필요해 보이는 getter는 최대한 해당 객체에 메시지를 보낼 수 있도록 하였습니다. (수익률을 구하는 부분)

이번 리뷰도 잘 부탁드립니다!

Copy link

@yiminan yiminan left a comment

Choose a reason for hiding this comment

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

안녕하세요. 형우님!
3단계도 잘 구현해주셨네요. 👍
꼼꼼하게 코드를 작성하시다보니, 견고한 코드가 만들어졌고,
수정사항도 별로 없네요.
몇가지 코멘트 남겼는데요. 한 번 생각해보시고, 다시 요청 부탁드릴게요.

src/main/java/lotto/domain/LottoGroup.java Show resolved Hide resolved
src/main/java/lotto/domain/LottoNumber.java Show resolved Hide resolved
src/main/java/lotto/domain/LottoRank.java Show resolved Hide resolved
src/main/java/lotto/domain/LottoRank.java Outdated Show resolved Hide resolved
src/main/java/lotto/domain/WinningLotto.java Outdated Show resolved Hide resolved
@Wu22e
Copy link
Author

Wu22e commented May 9, 2022

이민님 리뷰 감사합니다!

equals, hashCode의 경우, 인텔리제이의 자동생성 기능을 사용하다보니.. 통일되지 않은 코드가 된 것같습니다.. ㅠㅠ
primitive와 reference에 대한 규칙들을 지키면서 non - null 필드에 대한 처리를 클래스별로 공통처리할 수 있도록 해보았습니다.


[엘레강트오브젝트 1.3 생성자에 코드를 넣지 마세요.] 에 대한 부분을 깊게 고민해 보았습니다.

우선 블로그를 보고 제가 생각한 내용을 정리하면 다음과 같습니다.

생성자를 사용한다 -> 넘겨진 파라미터를 바로 초기화 하여 사용 되는게 좋다. 만약 넘겨진 파라미터를 가공하여 넘긴다면 예기치 못한 가공비용이 발생한다. 이를 해결하기 위한 방법으로 초기화 하기전 캡슐화를 한 뒤, 클래스 사용자가 파싱 시점을 결정한다.

우선 제가 작성한 코드에서 해당 내용을 적용시키기 위해 Number 인터페이스와 그 구현체를 만들어서 적용하는 것은 좀 과도한 분리라는 생각이 들었습니다 .. (순전히 저의 생각이기 때문에 무엇이 정답인지는 모르겠습니다..)
또한, 캡슐화를 하는 과정에서 검증되지 않은 잘못된 값을 가지고 있다가 필요한 시점에 검증과 동시에 파싱을 하게된다면 정작 필요한 값을 얻는 과정에서 문제가 발생할 수 있겠다는 생각도 들었습니다. (물론 여기도 정답은 없겠지만 차라리 객체를 생성할때 바로 문제를 아는게 낫지 않나? 라는 생각이었습니다.)

해당 규칙을 지키고자 한다면 2가지 정도의 방법이 있을것 같다는 생각이 들었습니다.

  1. 생성자를 통해 받는 파라미터를 외부에서 검증하여 넘겨준다. (지금의 형태라면 InputView에서 해당 부분을 처리할 수 있을 것 같습니다.)
  2. 생성자가 아닌 정적 팩터리 메서드 패턴을 이용하여 파라미터의 가공 로직을 위임한다.

해당 내용에 대해서 어떻게 생각하시는지 궁금합니다!

@yiminan
Copy link

yiminan commented May 10, 2022

이민님 리뷰 감사합니다!

equals, hashCode의 경우, 인텔리제이의 자동생성 기능을 사용하다보니.. 통일되지 않은 코드가 된 것같습니다.. ㅠㅠ primitive와 reference에 대한 규칙들을 지키면서 non - null 필드에 대한 처리를 클래스별로 공통처리할 수 있도록 해보았습니다.

[엘레강트오브젝트 1.3 생성자에 코드를 넣지 마세요.] 에 대한 부분을 깊게 고민해 보았습니다.

우선 블로그를 보고 제가 생각한 내용을 정리하면 다음과 같습니다.

생성자를 사용한다 -> 넘겨진 파라미터를 바로 초기화 하여 사용 되는게 좋다. 만약 넘겨진 파라미터를 가공하여 넘긴다면 예기치 못한 가공비용이 발생한다. 이를 해결하기 위한 방법으로 초기화 하기전 캡슐화를 한 뒤, 클래스 사용자가 파싱 시점을 결정한다.

우선 제가 작성한 코드에서 해당 내용을 적용시키기 위해 Number 인터페이스와 그 구현체를 만들어서 적용하는 것은 좀 과도한 분리라는 생각이 들었습니다 .. (순전히 저의 생각이기 때문에 무엇이 정답인지는 모르겠습니다..) 또한, 캡슐화를 하는 과정에서 검증되지 않은 잘못된 값을 가지고 있다가 필요한 시점에 검증과 동시에 파싱을 하게된다면 정작 필요한 값을 얻는 과정에서 문제가 발생할 수 있겠다는 생각도 들었습니다. (물론 여기도 정답은 없겠지만 차라리 객체를 생성할때 바로 문제를 아는게 낫지 않나? 라는 생각이었습니다.)

해당 규칙을 지키고자 한다면 2가지 정도의 방법이 있을것 같다는 생각이 들었습니다.

  1. 생성자를 통해 받는 파라미터를 외부에서 검증하여 넘겨준다. (지금의 형태라면 InputView에서 해당 부분을 처리할 수 있을 것 같습니다.)
  2. 생성자가 아닌 정적 팩터리 메서드 패턴을 이용하여 파라미터의 가공 로직을 위임한다.

해당 내용에 대해서 어떻게 생각하시는지 궁금합니다!

공유차원에서 드린 링크였는데, 많은 고찰이 필요했던 링크였나보네요.

정확히 솔루션들을 찾으신것으로 생각합니다. 👍

다만, 1번 형태는 캡슐화 관점에서 비즈니스의 책임이 외부로 넘어가서 적절하지 않은 형태가 될 수 있다고 생각합니다.
물론, InputView의 추상화 정도가 현재는 로또 입력에 최적화 되어있기 때문에 비즈니스의 위임이 가능할 순 있겠네요.

저는 2번 형태가 적절하다고 생각합니다.
일단 앞서, 검증을 말씀해주셨는데요.
객체 생성을 위한 검증은 꼭 주생성자에서 하지 않아도 생각합니다. 정적 팩토리 메서드에서도 가능하다고 생각하구요.
주 생성자는 객체의 생성에만 책임을 다하게 할 수 있구요.
말씀하신대로, 파라미터를 가공해서 생성에 필요한 값을 만드는 일은 정적 팩토리 메서드가 위임받는 게 좋은 방향이라고 봅니다.

Copy link

@yiminan yiminan left a comment

Choose a reason for hiding this comment

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

안녕하세요. 형우님!
이번 단계 미션도 고생하셨습니다. 👍
주신 코멘트에 답변 드렸구요. 확인부탁드릴게요.
PR은 머지하도록 하겠습니다!

@yiminan yiminan merged commit ac714cb into next-step:wu22e May 10, 2022
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