Step2: 로또(자동) #3964
Step2: 로또(자동) #3964
Conversation
- null check 메서드를 private으로 변경하고 빈 문자열 검사도 추가하였습니다. - MathExpression 내 operation을 String이 아닌 Enum으로 변경하였습니다.
- Collections.shuffle 을 사용하여 랜덤한 값의 번호를 6개 가지는 로또를 생성하도록 개발하였습니다. - 로또 객체 생성시 숫자를 오름차순 정렬하도록 하였습니다.
- 로또 생성시 입력되는 숫자를 체크하는 익셉션 로직을 추가하였습니다. - 중복 숫자가 있을 경우 익셉션을 발생시키는 로직을 추가하였습니다.
- 우승 번호를 입력받아 로또를 체크할 수 있는 LottoChecker을 만들었습니다. - 우승 로또가 비어있을 경우 Exception을 발생시키도록 했습니다. - 우승 로또를 입력받는 InputView를 개발하였습니다.
- 로또 체커 생성시 구매된 로또와 당첨 로또숫자를 비교하여 숫자를 세도록 하였습니다. - 당첨 로또에 관련된 일부 메서드와 변수의 이름을 바꾸었습니다.
- 입력받은 금액에 맞게 로또를 사는 로직을 개발하였습니다.
- 로또의 수익률을 계산하는 로직을 개발하였습니다.
- main 과 result view 를 추가로 개발하였습니다. - print를 위한 몇몇 메서드를 추가하고, 일부 오류를 수정하였습니다.
|
#tag @neojjc2 |
| * 입력 문자열의 숫자와 사칙 연산 사이에는 반드시 빈 공백 문자열이 있다고 가정한다. | ||
| * 나눗셈의 경우 결과 값을 정수로 떨어지는 값으로 한정한다. | ||
| * 문자열 계산기는 사칙연산의 계산 우선순위가 아닌 입력 값에 따라 계산 순서가 결정된다. 즉, 수학에서는 곱셈, 나눗셈이 덧셈, 뺄셈 보다 먼저 계산해야 하지만 이를 무시한다. | ||
| * 예를 들어 2 + 3 * 4 / 2와 같은 문자열을 입력할 경우 2 + 3 * 4 / 2 실행 결과인 10을 출력해야 한다. |
There was a problem hiding this comment.
요구사항 정리도 잘 해주셨고, 이전단계 개선사항도 잘 정리 해주셨네요 💯
|
|
||
| assertThat(LottoPrizeCalculator.calculateRateOfReturn(new LottoChecker(purchasedLotto, winnerLotto))).isEqualTo(1); | ||
| } | ||
| } |
There was a problem hiding this comment.
만들어진 도메인 객체에 비해 테스트가 좀 부족한 것 같습니다.
각 도메인들이 가지고 있는 method들이 최대한 테스트가 되면 좋을 것 같네요 🙇
LottoTest를 통해 모든 도메인 객체들이 복합적으로 테스트가 되고 있는데요,
이렇게 되면 각 도메인 객체들의 기능이 개별적으로 정상동작 되는지 확인이 어렵습니다.
아마 테스트 작성에 어려움도 겪으셨을 것 같은데요 😅
보통 이런경우 객체들의 역할이 잘 분배가 되었는지, 하나의 객체가 너무 많은 일을 하고 있는 건 아닌지 관점에서 분리하고 정리해보시면 테스트 작성이 쉬워지실 것 같습니다.
프로그래밍 요구사항
모든 기능을 TDD로 구현해 단위 테스트가 존재해야 한다. 단, UI(System.out, System.in) 로직은 제외
| } | ||
|
|
||
| return purchasedLottos; | ||
| } |
There was a problem hiding this comment.
LottoInputView가 로또를 생성하는 책임까지 가져가는 건 좀 과한 것 같습니다.
다른 도메인 객체나 혹은 서비스코드에서 로또를 생성하고 싶을 때는 LottoInputView를 아예 사용안하거나
꼭 console 입력이 아닌 다른 입력을 수단으로 생각하고
Lotto를 사고 싶다면 이 코드를 복붙해서 사용해야 하기 때문에 계속적으로 중복 될 것 같아요 🤔 (createLottoWithScan도 마찬가지 일 것 같습니다.)
LottoInputView는 사용자로부터 입력이 잘 되었는지, 값이 비어있지는 않은지 정도만 확인하고 Main에서 다른 객체를 통해 생성되도록 하는게 더 좋을 것 같습니다 😄
프로그래밍 요구사항
핵심 로직을 구현하는 코드와 UI를 담당하는 로직을 구분한다.
| StringBuilder sb = new StringBuilder(); | ||
| sb.append(lotto.getNumberByIndex(0).toString()); | ||
|
|
||
| for (int i = 1; i < 6; i++) { |
| System.out.println(); | ||
| System.out.println("당첨 통계"); | ||
| System.out.println("---------"); | ||
| for (int i = 3; i < 7; i++) { |
There was a problem hiding this comment.
3, 7도 매직넘버 입니다. 지수님과 저야 요구사항을 다 알고 있기 때문에 3, 7이 의미하는 값이 어떤지 알지만, 아마 이 부분을 처음 보게될 다른 동료개발자들은 이해하려면 전체 코드 분석을 다 해야 하기 때문에 많은 어려움을 겪을 것 같습니다.
이 부분은 가독성 개선이 필요할 것 같아요.
| private int countMatch(Lotto lotto) { | ||
| int count = 0; | ||
| for (int i = 0; i < 6; i++) { | ||
| count += countMatchNumber(lotto.getNumberByIndex(i)); |
There was a problem hiding this comment.
lotto.getNumberByIndex(i) 매번 index 값인 i를 통해서 값을 가져와야 하는 구조입니다 😄
get을 사용하지 말라는 요구사항 때문에 이렇게 작업해주신 것 같은데,
제 개인적인 생각으로는 get을 아예 사용안하는 건 불가능 하다고 생각합니다.
사용하더라도 안전한 방식으로 사용한다면 크게 문제는 없다는 입장인데요,
get을 사용하되 numbers를 불변객체, 혹은 복제하여 반환한다면 (원본에 이상이 없도록) 괜찮다는 입장입니다.
// Lotto
public List<Integer> getNumbers() {
return Collections.unmodifiableList(this.numbers);
}
// AS IS
for (int i = 0; i < 6; i++) {
count += countMatchNumber(lotto.getNumberByIndex(i));
}
// TO BE
for (Integer number : lotto.getNumbers()) {
count += countMatchNumber(number);
}| public class LottoFactory { | ||
|
|
||
| private static final List<Integer> numbers = | ||
| IntStream.rangeClosed(1, 45) |
| private void duplicateNumberCheck(List<Integer> numbers) { | ||
| if (numbers.stream().distinct().count() < 6) | ||
| throw new IllegalArgumentException("중복된 숫자가 있습니다."); | ||
| } |
There was a problem hiding this comment.
당첨번호도 Lotto객체와 동일한 속성을 가지고 있습니다.
같이 검증되면 더 좋을 것 같습니다 😄
| } | ||
|
|
||
| private void duplicateNumberCheck(List<Integer> numbers) { | ||
| if (numbers.stream().distinct().count() < 6) |
| List<Integer> copyOfNumbers = new ArrayList<>(numbers); | ||
| Collections.shuffle(copyOfNumbers, random); | ||
| return createLotto(copyOfNumbers.subList(0, 6)); | ||
| } |
There was a problem hiding this comment.
로또 생성 때 적용되는 번호 뿐만 아닐, 당첨 번호도 동일한 번호 (1~45) 검증이 필요할 것 같습니다 🙇
테스트 코드도 이전단계로 돌리면서 하면 너무 개발의 흐름이 깨질 우려가 있습니다 😄 물론 TDD 이기 때문에 테스트 작성 (혹은 수정) -> 코드 수정 순으로 가는게 맞겠지만, 테스트 코드를 수정하면서 내가 수정한 코드가 영향을 미치는 범위도 파악할 수 있고, 수정내용에 따라서
문의 주신 부분에 대해서는 리뷰 코멘트 남겨드렸습니다 😄 |
Step2: 로또 (자동)
질문사항
확실히 2단계로 넘어오니까 이전 메서드와 테스트의 큰 틀을 유지하며 추가 개발을 하는 것이 쉽지 않았습니다. 만약 개발 중간에 이전 메서드를 많이 변경해야 한다는 판단이 들 때, 테스트 코드까지 이전 단계로 돌아가서 새로 만들고 거기서부터 다시 커밋하는게 나을까요?
개발중 final 객체인 LOTTO_PRICE 가 inputView에도 필요하고, prizeCalculator에도 필요하여 양쪽에 선언하였습니다. 이렇게 양쪽으로 선언을 하면 유지보수 할 때 좋지 않을 것 같은데요.. 이런 상수가 여러개라면 enum 혹은 다른 클래스로 빼보는 것을 생각할 것 같은데, 지금은 LOTTO_PRICE 한 개 뿐이라.. 조언을 듣고 수정하고 싶어 우선 그대로 push를 하였습니다. 이런 경우 어떻게 관리하시는 편이신가요?