Skip to content

[미션 2] 로또게임 구현 완료했습니다. 리뷰 요청드립니다.#11

Open
vvvpiano wants to merge 9 commits intosproutt:gooroomfrom
vvvpiano:step2
Open

[미션 2] 로또게임 구현 완료했습니다. 리뷰 요청드립니다.#11
vvvpiano wants to merge 9 commits intosproutt:gooroomfrom
vvvpiano:step2

Conversation

@vvvpiano
Copy link
Collaborator

@vvvpiano vvvpiano commented Sep 3, 2020

  1. 완전히 자유롭게 구현하는게 아니라 힌트에 나온대로 LottoNo, Lotto, WinningLotto 등 클래스들을 위주로 구현하려 하니까 어려웠다. 힌트가 어쩌면 제약사항이 되어 더 고민하며 설계하게 하는 계기가 되는것 같다.
  • 저번 자동차게임처럼 사용자에게 입력 받은 원시값을 바로 Lotto에 넣어서 LottoNo를 만드는게 적절한지, LottoNo를 만들어서 Lotto에 넣어주는게 적절한지 고민이 되었다. (전자를 택했는데 그 이유는 LottoNo는 Lotto에 속하는 데이터인데 밖에서 LottoNo객체를 만들어서 Lotto에 넣어준다면 Lotto안의 데이터에 관한 처리를 Lotto밖에서 하기 때문이고 이는 부적절하다고 판단했기 때문)
  • 당첨 번호에 해당하는 로또를 WinningLotto 객체로 추상화한다라는 말의 의미가 헷갈렸다. WinningLotto객체가 로또 한 회차의 당첨번호(ex 1,2,3,4,5,6, + 보너스 7)를 의미하는지, 발급한 로또 중에 상금 당첨이 된 로또들을 말하는지 한참을 고민하고 서치도 나름 해봤는데 결국은 후자를 선택했다. 전자는 WinningNos라는 객체로 만들었다.
  • 10장의 로또를 발급했고 그 중 3장이 상금이 있는 로또들이었다고 가정하면, 나의 LottoGame에서는 Lotto객체 10개가 생성이 되고, 후에 로또 당첨 결과를 LottoAnalyzer를 통해 분석하여 WinningLotto 3개를 생성한다. 사실 가지고 있는 데이터도 공통점이 있고 현실세계에서의 관점을 반영해서라도 WinningLotto는 Lotto를 상속해서 만들고 싶었다. 그런데 객체가 생성되는 시점이 달라 생성자의 파라미터로 들어갈 데이터 형태가 달라져서 상속이 곤란해졌다. 인풋을 받은 직후 생성하는 Lotto와 달리 이미 프로그램 속에 Lotto 객체로만 존재하는 데이터를 통해 만들어야 했던 WinningLotto는 List를 가지고 만들기 부적절한 것 같아서 상속은 포기했다.
  1. array대신 ArrayList를 사용하고 int대신 Integer를 사용함으로써 얻는 이득을 체감할 수 있는 미션이었다. stream api도 정말 많이 쓰게 되고, Collection의 기능을 사용하여 줄일 수 있는 코드가 정말 많았다.

  2. enum을 모든 언어 통틀어서 처음 보고 사용해보았는데 처음에는 낯설었는데 java class와 비슷한 개념이어서 몇 번 연습하니까 곧 적용할 수 있었다. 다만 enum 상단부에 FIRST(6, 2000000000), SECOND(5, 30000000)...와 같이 선언하는 문법이 잘 이해가 안됐는데 서치해보니까 public static final Rank FIRST = newRank(6, 2000000000);와 같은 의미라는걸 알 수 있었다. 그리고 생성자가 private인 것도 신기했다. 참고자료

  3. 데이터를 직접 사용하는 경우(값 출력이나 값을 꺼내와 합산해야 할 때)가 아니면 최대한 getter를 사용하지 않도록 노력했는데 이게 정말!! 어려웠다. 처음 작성할 때도 최대한 안쓰려고 노력했고 리팩토링 하면서도 getter를 없애려고 했다.

  4. 아래 두 글을 주로 참고하여 적절한 matcher와 테스팅 메소드를 사용해보려고 했는데 주로 사용하는것만 자꾸 쓰게 되는 것 같긴 했다. 이번에는 Collections의 자료구조를 사용했으니까 assertArrayEquals를 사용할 수 있을 줄 알았는데 동작하지 않아서 새로 만든 클래스들에 대해 equals를 정의했어야 했다. (이 부분은 Intelij에서 자동으로 코드를 완성해줘서 아주 편리했다)
    assertThat 사용법
    JUnit으로 유닛테스팅하기

  5. 저번 미션에는 void메소드가 많아서 테스트가 어려웠던 경험이 있었다. 피드백 문서와 저번 미션 경험을 통해 얻은 교훈으로 이번에는 테스팅이 필요한 메소드들은 최대한 output이 있는 메소드로 만들려고 노력했다.

Copy link
Contributor

@hyukjin-lee hyukjin-lee left a comment

Choose a reason for hiding this comment

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

고생하셨습니다~ 구현 잘 하셨네요.

그냥 코드를 읽으면서 순간순간 생각난걸 그대로 적은거라서
잘못된 내용이 있을 수도 있습니다.
리뷰 반영하면서 의문이 드는 점은 물어보면 좋을 것 같습니다.

이제 과정이 종료되어가니 한 가지 당부의 말씀드리자면,

객체지향은 현업의 수많은 상황들을 경험하지 않고서는
제대로 이해하기 어렵습니다. 그리고 정답도 없구요.
지금 저희가 하는 과정도 요구사항에 비해 많이 과한 코드인 것은 분명합니다.
그저 연습을 하기 위해 이렇게 코드를 짜는 것일 뿐이지요.
그렇기 때문에 지금 하는 것들을 '절대적인 지식이다!' '이런 코드가 짱이야!' 라고 생각하지는 않으시길 바랍니다.
축구선수가 되기 위해 운동장에서 혼자 공놀이 하는 정도로 여기시고
항상 공부하실 때 본인이 알고 있는 지식에 대해 의문을 가져보고
새로운 지식과 깨달음에 열려있으시면 좋을 것 같네요.

이번 과정 정말 수고 많으셨습니다.
추가 리뷰 요청은 언제든 환영입니다 ~

(구름님은 제가 여태 만났던 멘티들 중에서 가장 이 과정의 의미를 잘 이해하셨고, 열심히 하셨고, 적극적이셨고, 훌륭하셨습니다. 덕분에 많이 배워갑니다 감사합니다)

}

private List<Lotto> issueManualLottos(List<List<Integer>> manualLottoNos) {
List<Lotto> ManualLottos = new ArrayList<>();
Copy link
Contributor

Choose a reason for hiding this comment

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

변수명이 대문자로 시작하네요 ~

import java.util.stream.Collectors;
import java.util.stream.IntStream;

public class LottoGenerator {
Copy link
Contributor

Choose a reason for hiding this comment

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

Lotto 에 factory method 을 추가하여 생성해보면 어떨까요?
ex> Lotto.ofAuto(), Lotto.ofManual("1,2,3,4,5,6)

public class LottoNo {
private Integer value;

public LottoNo(int number) {
Copy link
Contributor

Choose a reason for hiding this comment

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

LottoNo 를 생성할 때 range 에 대한 조건을 검증하면 좀 더 의미 있는 wrapping class 가 되지 않을까요?

@@ -0,0 +1,25 @@
package lottodomain;

public class LottoNo {
Copy link
Contributor

Choose a reason for hiding this comment

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

문제 :
"모든 원시값과 문자열을 포장한다." 원칙에 따라 인스턴스를 생성하다보니 너무 많은 객체가 생성되고, GC가 되어 성능상 문제가 발생한다.
특히 로또 번호 하나까지 객체로 포장할 경우 생성되는 인스턴스의 수는 상당히 늘어나는 문제가 발생한다.

위와 같은 문제는 어떻게 해결할 수 있을까요? (키워드 : static, map)

package lottodomain;

public class LottoNo {
private Integer value;
Copy link
Contributor

Choose a reason for hiding this comment

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

불변의 값을 wrapping 한 클래스이므로 final int value; 로 선언해보면 어떨까요?

System.out.printf("총 수익률은 %.1f%%입니다.\n", earningRate);
}

private static void showWRanks(LottoAnalyzer lottoAnalyzer) {
Copy link
Contributor

Choose a reason for hiding this comment

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

analyzer 가 outputview 에 와서 비즈니스 로직을 수행하고 있는데요
게임의 결과를 담는 LottoResults(DTO) 클래스와 Lotto list 를 감싸는 Lottos 클래스를 만들어서
outputview 에서 비즈니스 로직이 실행되지 않도록 변경해보면 어떨까요?

lottos.add(new Lotto(Arrays.asList(1, 2, 3, 7, 8, 9))); // FIFTH
lottos.add(new Lotto(Arrays.asList(1, 2, 7, 8, 9, 10))); // MISS
winningNos = new WinningNos(Arrays.asList(1, 2, 3, 4, 5, 6), 7);
lottoAnalyzer = new LottoAnalyzer(lottos, winningNos);
Copy link
Contributor

Choose a reason for hiding this comment

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

좋은 유닛테스트 작성법에는 F.I.R.S.T 라는 원칙이 있는데요
https://coding-start.tistory.com/261
첨부하고 갑니다 ~

private LottoGame lottoGame;
List<List<Integer>> manualLottoNumbers;

private void assertArrayEquals(List<Lotto> expectedLottos, List<Lotto> subList) {
Copy link
Contributor

Choose a reason for hiding this comment

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

private method 는 public 보다 아래에 위치시켜주는게 가독성에 좋습니다 ~


import static org.junit.Assert.*;

public class WinningNosTest {
Copy link
Contributor

Choose a reason for hiding this comment

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

전체적으로 코드에 비해 테스트 쪽이 조금 아쉬워서
시간이 되신다면 TDD 방법론을 통해 로또나 레이싱 게임을 다시 구현해보는 것도 많이 도움이 되실 것 같습니다

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.

2 participants