[미션2] 로또게임 구현완료했습니다. 리뷰부탁드립니다. - #2
Conversation
hyukjin-lee
left a comment
hyukjin-lee
left a comment
There was a problem hiding this comment.
고생하셨습니다~ 구현 잘 하셨네요.
일하다가 잠깐 쉬는 김에 리뷰를 달다보니까 어느 새 32개나 달았네요... (죄송)
그냥 코드를 읽으면서 순간순간 생각난걸 그대로 적은거라서
잘못된 내용이 있을 수도 있습니다.
리뷰 반영하면서 의문이 드는 점은 물어보면 좋을 것 같습니다.
이제 과정이 얼추 종료되어가니 한 가지 당부의 말씀드리자면,
객체지향은 현업의 수많은 상황들을 경험하지 않고서는
제대로 이해하기 어렵습니다. 그리고 정답도 없구요.
지금 저희가 하는 과정도 요구사항에 비해 많이 과한 코드인 것은 분명합니다.
그저 연습을 하기 위해 이렇게 코드를 짜는 것일 뿐이지요.
그렇기 때문에 지금 하는 것들을 '절대적인 지식이다!' '이런 코드가 짱이야!' 라고 생각하지는 마시길 바랍니다.
축구선수가 되기 위해 운동장에서 혼자 공놀이 하는 정도로 여기시고
항상 공부하실 때 본인이 알고 있는 지식에 대해 의문을 가져보고
새로운 지식과 깨달음에 열려있으시면 좋을 것 같네요.
이번 과정 정말 수고 많으셨습니다.
| import java.util.List; | ||
|
|
||
| public class LottoGame { | ||
| private List<Lotto> lottos; |
There was a problem hiding this comment.
로또 List 를 Lottos (일급 컬렉션) 로 구현해보면 어떨까요?
https://jojoldu.tistory.com/412
There was a problem hiding this comment.
lotto객체리스트를 lottos로 만든다면....
제가 lotto객체랑 당첨로또 객체랑 비교를 해서 똑같은공의 개수를알아내고 rank를 구하도록 구현을했거든요
그럼 당첨로또랑 비교하는 로직을 lottos안에 넣어야할까요?
근데 그렇게 하면 lottos가 로또게임 모든걸 다 처리해버려서요...ㅠ
There was a problem hiding this comment.
ㅠㅠ lotto List 를 get해서 처리하는 로직들을 Lottos 로 옮길 수 있는데,
그렇게 하면 lottos 로 많은 책임들이 이동하게 된다는 말씀이시죠?
이것은 제가 간단하게 몇마디로 답을 드릴 수 있는게 아니라
코드 설계의 근본적인 문제일 수 있으니 고민해보시면 좋을 것 같아요.
다 구현하기 나름이라서요 ~ 이것저것 해보시고 그러다가 좋은 구조가 나올 수도 있습니다
해보다가 정~ 안되시면 그 때 같이 코드 만져봅시다 !
| checkMoney(inputMoney, sliplottos); | ||
| lottos = new ArrayList<>(); | ||
| this.lottos = makeSlipLottos(sliplottos); | ||
| this.lottos = makeQuickPickLottos((inputMoney - LOTTO_PRICE * sliplottos.size()) / LOTTO_PRICE); |
There was a problem hiding this comment.
생성자에서 너무 많은 일을 하고 있는데요,
생성자는 단순히 값을 대입해서 객체를 생성하는 용도 + 입력값 validation 정도로만 구현되면 어떨까요?
| } | ||
|
|
||
| private void checkMoney(int inputMoney, List<String> sliplottos) { | ||
| if (0 > inputMoney - (sliplottos.size() * LOTTO_PRICE) || inputMoney < LOTTO_PRICE) { |
There was a problem hiding this comment.
조건이 한눈에 알아보기 힘든데, 이런 경우에 extract method 하면 가독성에 도움이 됩니다.
|
|
||
| public class LottoGame { | ||
| private List<Lotto> lottos; | ||
| private int InputMoney; |
| import java.util.HashMap; | ||
| import java.util.Map; | ||
|
|
||
|
|
| System.out.println(Rank.FOURTH.getCountOfMatch() + "개 일치 (" + Rank.FOURTH.getWinningMoney() + "원)- " + rankingResult.get(Rank.FOURTH) + "개"); | ||
| System.out.println(Rank.THIRD.getCountOfMatch() + "개 일치 (" + Rank.THIRD.getWinningMoney() + "원)- " + rankingResult.get(Rank.THIRD) + "개"); | ||
| System.out.println(Rank.SECOND.getCountOfMatch() + "개 일치 (" + Rank.SECOND.getWinningMoney() + "원)- " + rankingResult.get(Rank.SECOND) + "개"); | ||
| System.out.println(Rank.FIRST.getCountOfMatch() + "개 일치 (" + Rank.FIRST.getWinningMoney() + "원)- " + rankingResult.get(Rank.FIRST) + "개"); |
| public static void printFinalWinner(RacingGame racingGame, List<Car> cars){ | ||
| System.out.print(String.join(",", racingGame.searchWinners(cars)) + "가 최종 우승했습니다."); | ||
| public static void printLottos(int inputMoney, int slipCount, List<LottoDTO> lottos) { | ||
| int quickPickCount = (inputMoney - LOTTO_PRICE * slipCount) / LOTTO_PRICE; |
There was a problem hiding this comment.
view 에 이러한 계산로직이 없으면 좋을 것 같은데 방법이 없을까요?
거대한 로또 게임으로 발전한다고 생각해보면,
quikPickCount 계산 로직이 바뀌면 이렇게 구현된 모든 곳을 찾아서 바꿔줘야합니다.
이게 tell don't ask 에 어긋나는 케이스고,
view에서 이러한 로직을 모르는게 낫지 않을까요?
| winningLotto = new WinningLotto(winNumbers); | ||
| } | ||
|
|
||
| @Test(expected = IllegalArgumentException.class) |
There was a problem hiding this comment.
exception 으로 인해 테스트를 읽다가 흐름이 위로 향하게 되어서 가독성이 떨어집니다.
assertThrow 를 사용해보면 어떨까요?
| } | ||
|
|
||
| @Test | ||
| public void 이등만_당첨된경우() { |
There was a problem hiding this comment.
~경우 ~가 ~된다 로 서술하면 테스트 fail 되었을때 한눈에 알아보기 쉬울 것 같습니다
| public void 중복된번호의_로또를_만든다() { | ||
| List<LottoNo> testLotto = Arrays.asList(new LottoNo(1), new LottoNo(2), new LottoNo(3), new LottoNo(23), | ||
| new LottoNo(23), new LottoNo(41)); | ||
| Lotto lotto = new Lotto(testLotto); |
There was a problem hiding this comment.
생성자에 로직이 다 담겨있어서 테스트가 이렇게 생성자 테스트만 존재하는 것 같네요
There was a problem hiding this comment.
이렇게 되면 생성자 조금만 손대면 모든 테스트가 다 깨지게 되어서 Test 코드를 귀찮은 존재로 여기게 됩니다.
Test 하기 쉬운 코드, Test 유지보수가 쉬운 코드로 리팩토링 해보면 좋을 것 같습니다.
2 participants
역시 저번보다 난이도가 높네요!ㅠ 더 오래걸렸습니다 ㅋㅋ 로또에 관심이없어서 이번에 규칙도 찾아보면서 구현했는데 로또는 정말 당첨되기 어렵다는걸 한번더 달았네요 ㅋ 어려웠던부분, 고민을 많이했던 부분에 대하여 정리하자면
객체비교, 정렬
그냥 int숫자를 정렬하는게 아니라 객체를 비교 하고 정렬하는 부분을 할줄 몰라가지고.. 시작할때는 이걸로 애먹었네요 굳이 숫자 하나하나 객체로 감싸야되나? 생각했는데 짬뽕님이 올려주신자료도 보고 만들고나서 보니까 이게 오히려더 편하다는것을 깨달았습니다.
tell, don't ask
tell, don't ask는 racing game할때부터 의문을 갖고 이해가 안가고 어렵게 느껴졌었는데요, 질문도 했었고 머리로는 좀 이해하겠지만 setter은 안쓸 수가 있겠는데 getter는 안쓸수가 없지 않나? 라는 생각을 했었습니다. 이 미션을 진행하면서 tell, don't ask에대하여 더 많이 이해한것같아요! 더 객체지향적으로 만들기위한 방법이라는걸 깨달았습니다. getter을 최대한 안쓰고 메시지를 보내려고 노력했습니다! 하지만 아예 안쓰진 않았어요 ㅠ ㅎㅎ
mvc
view는 모델을 몰라야되고 , model은 view를 몰라야합니다.
2-1 result view
로또 결과를 출력할때 get을 너무 남발해야할수 밖에 없을것같아 view에게 데이터를 전달하는 다른 방법이 있나, 객체를 데이터로 취급할수있나, mvc에 대하여 찾아보다 DTO를 알게되서 그것을 사용했습니다. ResultDTO를 사용하여 당첨통계를 출력했습니다.
전체 로또를 출력할때 만들고나서 보니까 제가 result view에 Lotto객체리스트를 넘겨줬더라구요. 출력하려면 로또를 알아야 출력하는건데 로또가 일급컬렉션이니까 view는 알아도되지 않을까? 라는 생각을 하긴했습니다만.. 아닌것같아서 Lotto도 DTO를 만들어서 사용해주었습니다.
전 한번 코딩하는데도 엄청오래걸리기도 하고 또 남의 코드를 읽는다는게 정말 힘들더라구요ㅠ 머리가 안돌아가요ㅠ ㅋㅋ racingGame은 misson0에 비해 클래스도 많고 커져서 힘들었습니다ㅜ( 남의 코드를 잘 읽는것도 실력이죠 제가 지금 부족해서 한번 읽을때 오래 읽는거니까 많이 읽어봐야겠다는 생각을 했습니다.) 그래서 멘토님께 항상 감사함을 느낍니다 ㅎㅎ 저는 하나도 힘겹게 읽는데 여러명꺼 읽으시고 리뷰까지 ㄷㄷ 감사합니다~!