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

[1단계 - 로또(자동)] 유콩(김유빈) 미션 제출합니다. #397

Merged
merged 47 commits into from
Mar 2, 2022

Conversation

kyukong
Copy link

@kyukong kyukong commented Feb 24, 2022

안녕하세요 소니! 유콩입니다 😁
로또 미션 첫 PR 날립니다.

지난 미션에서 처음 접해보는 개념들이 많아
이번 미션에는 지난번에 알게 되었던 것들을 익숙해질 때까지 적용해보는 것이 목표입니다.
TDD, 일급 컬렉션, Stream, 인스턴스 변수 중 List 의 불변 등을 적용해보았습니다.
(우테코 페이지에 있는 1단계 피드백을 지금 단계에서 봐도 되는지 몰라서 적용을 못했습니다..!
리팩토링 시에 읽고 반영하겠습니다!!)

나름 열심히 해보았으나 코드에 찝찝함이 있어도 어떤 부분이 찝찝한지 찾기가 어렵네요. 🥲
피드백 기다리고 있겠습니다. ㅎㅎ

시간을 내주셔서 감사합니다! 🙇‍♀️

kyukong and others added 22 commits February 22, 2022 15:19
- 사용하지 않는 import 삭제
- 사용하지 않는 Ball 생성자 삭제
- Lotto 매직넘버 상수화
- 테스트코드 스네이크 케이스로 수정
- Rank 열거형 생성
- 간결한 네이밍 수정
- 매직넘버 상수화
- 테스트코드 간결화
- Output 에서 에러 메시지 출력
- 사용하지 않는 util 클래스 삭제
- Input, Output 메소드명 수정 및 호출 방식 수정
- 검증하는 메소드의 의도를 더 정확하게 나타내기 위해 validate 으로 수정
- 구입 가능한 로또 개수는 구입 금액에 속해있는 값이므로 이동
- 역할이 없는 LottoMachine 객체 삭제
@kyukong
Copy link
Author

kyukong commented Feb 25, 2022

소니! 궁금한 것이 하나 더 있습니다. 😁

저는 이펙티브 자바에서 toString 에 대해 디버깅 시에 편리하고 로그를 남길 때 사용한다 라고 이해했습니다. (아이템 12) 그러므로 매번 정의하라고 하더군요. 하지만 이전 미션의 피드백과 다른 크루들의 피드백들을 보면 toString 을 사용하지 않는 분이 더 많아 보이더라구요. 접근 제어자가 public 이기도해서 당장 기능에 사용하지 않아도 view 의 역할을 대신하기 때문인 것 같습니다.

소니는 평소 toString 을 사용하는지, 사용하지 않는다면 그 이유가 궁금합니다...! 저에겐 아직 경험이 많이 부족해 실무자의 의견이 궁금합니다. 👀

Copy link

@sonypark sonypark left a comment

Choose a reason for hiding this comment

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

유콩 안녕하세요~~
이번 리뷰를 맡은 소니입니다! 😃
로또 미션하느라 고생 많으셨어요.
지난 미션에 배운 내용을 적용하려고 노력하신 것 같아요. 잘 하셨습니다!
코드 중 개선해볼 수 있는 부분에 대해 의견남겨 보았는데요. 한 번 읽어보시고 생각해보시면 좋을 것 같습니다.

p.s. 제 피드백이 틀릴 수 있으니 무조건 적으로 반영하지 마시고 공부를 통해 납득할 수 있는 경우에 반영해주세요. 또한 더 좋은 방법이 있는 경우에도 알려주시면 좋을 것 같아요. 더 나은 구조를 위한 토론은 언제나 환영입니다.🙌🏻

src/main/java/lotto/domain/Payment.java Outdated Show resolved Hide resolved
src/main/java/lotto/domain/Payment.java Outdated Show resolved Hide resolved
src/main/java/lotto/domain/Lottos.java Outdated Show resolved Hide resolved
src/main/java/lotto/domain/Lotto.java Outdated Show resolved Hide resolved
src/main/java/lotto/domain/Lotto.java Outdated Show resolved Hide resolved
src/main/java/lotto/domain/Ball.java Outdated Show resolved Hide resolved
src/main/java/lotto/domain/Ball.java Outdated Show resolved Hide resolved
src/main/java/lotto/domain/Lottos.java Outdated Show resolved Hide resolved
src/main/java/lotto/controller/LottoController.java Outdated Show resolved Hide resolved
src/main/java/lotto/domain/WinningLotto.java Outdated Show resolved Hide resolved
@sonypark
Copy link

소니! 궁금한 것이 하나 더 있습니다. 😁

저는 이펙티브 자바에서 toString 에 대해 디버깅 시에 편리하고 로그를 남길 때 사용한다 라고 이해했습니다. (아이템 12) 그러므로 매번 정의하라고 하더군요. 하지만 이전 미션의 피드백과 다른 크루들의 피드백들을 보면 toString 을 사용하지 않는 분이 더 많아 보이더라구요. 접근 제어자가 public 이기도해서 당장 기능에 사용하지 않아도 view 의 역할을 대신하기 때문인 것 같습니다.

소니는 평소 toString 을 사용하는지, 사용하지 않는다면 그 이유가 궁금합니다...! 저에겐 아직 경험이 많이 부족해 실무자의 의견이 궁금합니다. 👀

네 toString을 재정의하는 이유는 유콩말처럼 로깅과 디버깅을 할 때 가독성을 높이기 위해서 입니다. 재정의 하지 않으면 클래스 이름과 메모리주소만 나오기 때문에 객체 안에 들어있는 값이 무엇인지 알 수 없기 때문인데요. [Oracle문서: toString에 대한 정의](https://docs.oracle.com/javase/10/docs/api/java/lang/Object.html#toString())에도 보면 모든 서브클래스에 toString()을 재정의 할 것을 권장하고 있네요. 단, 재정의 할 때는 객체가 가진 정보를 잘 표시해주는게 중요한데 보통 수동으로 하지 않고 IDE의 자동생성 기능을 이용하는걸 권장합니다. 유콩이 Ball에 toString()을 재정의 해주셨는데, IDE로 재정의한것과 어떤 차이가 있는지 한 번 확인해보시면 좋을 것 같아요. 실무에서도 사용하는지 질문하셨는데, 저희 팀 같은 경우는 로깅이 필요한 부분(DTO, 변경이력을 남기는 클래스 등)에 toString() 메서드를 재정의해서 사용하는 경우가 있습니다. 그런데 로깅을 할 때 객체를 통째로 로그에 넣어주기 보다는 필요한 값만 추출해서 넣어주는 경우가 많아서 모든 클래스에 toString을 재정의하지는 않고 있습니다.

- 메소드 이동에 따른 테스트 위치 수정
- 파라미터 타입을 String 에서 int 로 변경하여 생성자 내부에 String 관련 검증 로직을 Input 으로 이동
- PaymentTest 에서 String 관련 검증 테스트 하던 것을 InputTest 로 이동
- 입력 값을 임의로 지정하기 위해 전략 패턴 사용하여 Entering 인터페이스로 분리
- 타입 변경에 따른 테스트 코드 수정
@kyukong
Copy link
Author

kyukong commented Feb 26, 2022

toString 관련

toString 을 재정의 하는 목적이 디버깅로깅 인 만큼 해당 필요성이 있을 때에만 정의를 해주는 것이 좋겠군요. 이번 미션에서는 로그를 남기지 않기 때문에 로깅을 제외하더라도 디버깅하기 어려운 값도 없을 뿐더러 테스트에서 잘못된 값이 입력될 경우를 확인하므로 굳이 필요하지는 않아 보입니다! Ball 에서 재정의한 toString 도 삭제하였습니다. 😁
(Ball 객체에서 toString 을 재정의한 것은 초반에 모든 객체에 재정의 하는 것이 맞은가를 고민하다가 실수로 객체 하나에만 추가하고 삭제를 안했습니다...!! 🤣)

@kyukong
Copy link
Author

kyukong commented Feb 26, 2022

Ball 객체의 equals 관련

equals를 오버라이드 하셨네요. 아마 숫자 비교를 위해 하셨을 것 같은데. 비교할 수 있는 다른 방법은 없을까요?

equals 를 사용해본 적이 없어 연습겸 추가해봤습니다. 🤣 다른 방법을 생각해 본다면 같은 로직을 가지고 있는 별도의 메소드를 생성하는 것밖에는 생각이 안나네요....😂😂 equals 에 대해 다시 찾아본 결과, equals 는 보통 구현한 프로그램 내에서 실제로는 다른 주소값을 갖는 객체지만 동일한 객체(인스턴스)로 판단해도 되는 값 에 한해 구현한다고 이해했습니다. 로또 미션에서 본다면 로또의 숫자인 Ball 은 Lotto 를 생성할 때마다 각각 생성되는 별도의 객체지만 실질적 객체 비교는 무의미하고 객체 내 숫자의 동일 여부가 더 중요하다는 생각이 들어 equals 를 삭제하지 않았습니다. 로직에서 타입에 관한 검증이 부족해 추가하였습니다!

추가로 equals와 hashcode를 언제 오버라이드 하는지, 둘 중 하나만 해도 되는지 등에 대해 공부해보시면 좋을 것 같아요🧐

이펙티브 자바에 의하면, equals 를 재정의하여 프로그램 내에서 동등하다고 판단하였으나 인스턴스를 HashMap 또는 HashSet 같은 컬렉션의 원소로 사용할 때 hashCode 의 값을 기준으로 정의하므로 사용 시 문제가 발생할 수 있다고 합니다. equals 와 동일한 맥락으로 값이 동일한 것은 동등한 객체로 판단하였기 때문에 해시값도 일치해야 하여 필수로 재정의 해야 한다고 이해했습니다. 😁

@kyukong
Copy link
Author

kyukong commented Feb 27, 2022

생성자를 호출하는 클래스(Creator) 관련

Controller 가 수행하는 역할이 많아 보여 로직을 줄이고 싶어도 어떤 것을 줄일지 고민이 많았는데 피드백을 보고 생성자 호출 관련 로직만을 분리해보았습니다...! 통일성을 위해 생성자 호출 외에 다른 로직이 없는 것들도 별도의 메소드를 만들어 호출하였습니다. 입력값에 대해 생성자를 호출하여 반환하므로 view 로 분류해보았는데 괜찮을까요?
(Lotto 와 Lottos 의 자기 자신을 생성하여 생성자를 호출 하던 것은 다른 방식으로 구현할 수 있을듯해 생성자를 삭제하였습니다!)

그리고 피드백에서 주신 자기 자신의 객체 정보를 생성하여 생성자를 호출하는 형태가 왜 좋지 않은지 모르겠습니다. 피드백에서 언급해주신 테스트를 진행하기 어려워서일까요?🥺 어차피 또 다른 생성자를 호출하는 형태므로 마지막으로 호출하는 생성자만을 테스트하면 간접적으로 테스트 할 수 있지 않을까요? 아니면 자기 자신의 정보를 자기가 생성한 다는 것이 의미가 맞지 않아서 일까요?

Copy link

@sonypark sonypark left a comment

Choose a reason for hiding this comment

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

유콩 안녕하세요 :)
작성해주신 코드 잘 봤습니다😃
2단계로 넘어가기 전에 객체의 역할/책임을 분리하는 것에 초점을 두고 리팩토링을 해보면 좋을 것 같습니다!
(아 그리고 테스트가 깨지는게 몇개 보이던데 확인 부탁드려요🙏🏻)

우선 전반적인 코멘트를 남겨봅니다.

  1. 생성자를 호출하는 클래스(Creator)를 만들어 주셨는데요. 이 클래스의 역할은 View에서 입력값을 받아 도메인 객체를 생성하고 있는데 view 패키지에 들어있네요. view 패키지에 있는게 맞을까요? 역할은 컨트롤러의 역할을 하고 있는 것 같아서요.

  2. 자기 자신의 객체 정보를 생성하는 생성자가 좋지 않은 이유는 생성자의 역할을 생각해보시면 될 것 같습니다. 정적 팩토리 메서드를 사용해서 객체를 생성하거나 복수의 생성자가 있어 내부에서 생성자를 호출하는 경우가 아니라면 클래스 내부에서 생성자를 호출할 일이 있을까요? 로또 하나는 6개의 로또 번호를 갖고 있습니다. 그렇다면 로또가 생성될 때 6개의 로또 번호를 받아야 할거에요. (내부에서 스스로 생성하는게 아니라) 따라서 객체의 역할과 책임의 분리가 필요한 것 같아 피드백을 드렸습니다. 관련 코멘트는 아래에 남겼으니 한 번 고민해보시면 좋을 것 같아요~ 이해 안되시거나 궁금한 점 있으면 언제든 코멘트 남겨주세요~

src/main/java/lotto/domain/Lotto.java Outdated Show resolved Hide resolved
src/main/java/lotto/domain/Lotto.java Outdated Show resolved Hide resolved
src/main/java/lotto/domain/Lotto.java Outdated Show resolved Hide resolved
src/main/java/lotto/domain/Ball.java Show resolved Hide resolved
src/main/java/lotto/domain/Lotto.java Outdated Show resolved Hide resolved
src/main/java/lotto/domain/Payment.java Outdated Show resolved Hide resolved
src/main/java/lotto/domain/LottoResult.java Outdated Show resolved Hide resolved
src/main/java/lotto/domain/Lottos.java Outdated Show resolved Hide resolved
src/main/java/lotto/view/Creator.java Outdated Show resolved Hide resolved
src/main/java/lotto/domain/Profit.java Outdated Show resolved Hide resolved
Copy link

@sonypark sonypark left a comment

Choose a reason for hiding this comment

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

피드백은 2단계에서 이어서 진행하겠습니다 :)

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

3 participants