코드리뷰 Lotto mission

안녕하세요 동호님 :) 미션 수행하느라 수고 많으셨어요 👍

코드를 천천히 읽어보았는데 실제 고객이 로또를 사는 것처럼 미션을 구현하신 게 인상 깊었어요. <로또 기계가 로또를 발행하고, 고객이 로또에 동그라미를 치듯이, 로또의 collect값을 변경하는 부분>

코드 전체적으로 객체가 해야 하는 일과 가져야 할 정보를 통제하려고 노력하신 것 같고, 예외처리나 테스트를 보면서 많은 시간을 들여서 꼼꼼하게 구현하셨다고 느꼈습니다.

동호님이 요청하신 피드백 사항

원시값 포장: 중요하고 안전하게 다뤄져야 하는 값은 객체로 포장하는 게 좋다고 생각합니다. [객체가 많아져도 좋아요 👍] 다만, BonusBall 객체 혹은 Lotto를 나타내는 객체는 구성 요소인 Integer가 모두 1에서 45 사이의 범위인지 확인하기 때문에 유효성 검사의 코드가 중복되고 있었습니다. Integer가 아닌 LottoNumber라는 객체를 생성해서 포장했다면 로또 번호의 유효성 검사의 코드 중복을 막을 수 있고, 원시값을 더 안전하게 보관할 수 있다고 생각합니다. (물론 Integer도 객체이지만, 더 의미를 잘 나타낼 수 있는 객체를 사용하라는 미션 취지였던 것 같습니다.) 또한 이러한 원시값을 의미 있는 객체로 포장하다 보면 2번 사항도 충족하게 됩니다.

일급 컬렉션: 동호님의 코드는 Consumer와 Lotto의 관계만 따진다면 Consumer가 Lotto의 비즈니스를 관리하므로, Lotto가 일급 컬렉션으로 만들어졌다고 표현할 수 있겠네요. 그러나 Lotto에서 바로 주소값을 통해 리스트를 외부로 전달하는 것은 위험해 보입니다. 저도 일급 컬렉션이라는 단어를 이번 미션하면서 처음 들었는데, 일급 컬렉션을 만들기 위한 코드가 아닌 "객체의 역할과 책임"을 생각하면서 객체를 분리하면 알아서 따라오는 것 같더라구요! (저도 아직 잘..🥲) 관련 블로그 링크 남겨봅니다: 일급 컬렉션 블로그

---

개별 피드백

LottoApplication.java

• mangsuyo: LottoStatistics 객체와 LottoMachine 객체의 어떠한 차이점 때문에 공백으로 구분하셨나요? 공백도 의미를 담을 수 있어요. 동호님은 어떠한 기준으로 공백으로 구분하셨는지 궁금해요.
• PlusUltraCode: 아무런 의미 없이 공백을 구분했네요. 아직 공백 및 컨벤션에 대한 습관이 미숙한 듯합니다. 피드백 감사합니다. 개인적으로 private 및 연관된 객체들이라 공백 없는 게 더 좋을 것 같습니다.

Consumer.java

• mangsuyo: Lotto를 구매하고 가지고 있는 건 "Consumer"이다. 실제 로또를 사는 상황을 생각하면서 미션을 구현하신 것 같아요 :)
• PlusUltraCode: 네, 맞아요. 그렇게 생각하며 구현했습니다!

ConsumerMoney.java

• mangsuyo: ConsumerMoney는 exception 패키지 안의 객체들처럼 정보를 저장하고, validate를 진행하네요. model에 있는 객체들과 exception에 있는 객체들의 차이점이 궁금합니다. 동호님은 어떤 기준으로 구분하셨나요?
• PlusUltraCode: 저의 부주의입니다. ConsumerMoney.java 파일도 exception 패키지로 이동했어야 했는데, 주의하겠습니다!

ConsumerMoney.java (투입금액 유효성 검사)

• mangsuyo: Lotto 클래스에서 static 상수인 PRICE를 public으로 공개해서, 숫자로 표시된 부분을 상수로 변경해보는 것도 좋을 것 같아요 :)
• PlusUltraCode: 항상 멤버 변수는 private로 선언해서 꽁꽁 숨긴다는 느낌으로 구현했었는데, 그런 방식으로 해도 되는군요! 확실히 public으로 공개하면 다른 객체들 사이에서 유용하게 쓰일 것 같습니다.

Lotto.java (getLottoNumber 메서드)

• mangsuyo: Lotto 클래스에서 저장하는 필드 lottoNumber는 너무너무 중요한 정보인 것 같아요. 동호님도 그 중요성을 알기 때문에 final로 선언하신 것 같습니다. 그러나 lottoNumber라는 리스트의 값이 아닌 주소를 반환하기 때문에 외부에서 lottoNumber를 수정하면, 주소를 공유하는 lottoNumber도 자동으로 수정될 거예요.
• PlusUltraCode: 맞습니다. 값을 동일하게 유지하면서 새로운 주소를 반환하도록 변경해야겠네요.

CollectNumber.java (validate 로직 중복)

• mangsuyo: CollectNumber와 BuyDirectNumber의 validate 로직이 중복되고 있네요. 두 클래스가 결국은 Lotto를 나타내고 있기 때문이라 생각합니다. 어떤 방법으로 코드의 중복을 막을 수 있을까요?
• PlusUltraCode: 각각의 입력마다 exception 패키지에 추가하는 형식이 아닌, 로또 자체의 로또번호를 record 이용하여 구현하면 해결될 것 같습니다. 저도 이 부분에서 중복된 코드를 복붙해도 되나 고민 많이 했었는데 감사합니다.

OutView.java (반올림 확인)

• mangsuyo: 반올림이 되는지 확인해보셔도 좋을 것 같아요 :)
• PlusUltraCode: 이를 방지하기 위해서는 DecimalFormat을 사용하여 수동으로 반올림을 처리하거나, Java의 BigDecimal 클래스를 사용하여 더 정확한 결과를 얻을 수 있습니다. 감사합니다.

ConsumerTest.java (랜덤 테스트)

• mangsuyo: 동호님의 요청을 읽고 구글링을 열심히 해봤는데, 랜덤에 관한 테스트에 대해 명확하게 피드백을 드릴 수가 없네요 🥲 죄송합니다.
• PlusUltraCode: 아니에요 피드백에서 많이 배울 수 있어 감사합니다.

BonusBall.java (원시값 포장)

• mangsuyo: 원시값 포장을 이용하면 로직 중복을 막을 수 있을 것 같아요!
• PlusUltraCode: 에러 코드들을 만들면서 중복되는 내용이 많았었는데 어떻게 원시값을 포장할지 고민해봐야겠습니다!!

Lotto.java (updateCollectedCount 메서드)

• mangsuyo: Consumer를 담당하는 개발자(Lotto 외부)의 실수로 로또의 중요한 정보인 collectedCount가 잘못된 값으로 업데이트될 수 있다 생각해요. 그러면 잘못된 값으로 덮어씌워진 순간부터 우리는 기존 값을 알 수 없어요! 어떻게 개선시킬 수 있을까요?
• PlusUltraCode: 이 부분은 고민해봐야겠습니다. 직관적으로 떠오르는 내용이 없습니다 ㅠ.ㅠ