[김기표]로또 2단계 구현완료 리뷰 부탁드립니다. - #10
Conversation
|
|
||
| public boolean isContain(int number) { | ||
| return lotto.contains(number); | ||
| public boolean isContain(LottoNo number) { |
| lotto.stream().forEach(lottoNo -> stringBuilder.append(lottoNo.getNumber() + ", ")); | ||
| stringBuilder.delete(stringBuilder.length() - 2, stringBuilder.length()); | ||
| stringBuilder.append("]"); | ||
| return stringBuilder.toString(); |
There was a problem hiding this comment.
Builder 패턴과 메소드 체이닝 공부하시고 stringBuilder 다시 써보세요
There was a problem hiding this comment.
그리고 정말 toString을 이렇게 구현하시겠습니까?
| return winningLottos; | ||
| } | ||
|
|
||
| public void purchaseMenual(String[] texts) { |
There was a problem hiding this comment.
text 보다 좋은 네이밍이 있을 것 같습니다
|
|
||
| public void purchaseMenual(String[] texts) { | ||
| for (String text : texts) { | ||
| List<LottoNo> contrivedNumber = Splitter.splitNumber(text); |
There was a problem hiding this comment.
contrivedNumber 보다 좋은 네이밍이 있을 것 같습니다
| List<Integer> randomNumber = generateRandomNumbers(); | ||
| Collections.sort(randomNumber); | ||
| Lotto lotto = new Lotto(randomNumber); | ||
| public List<Lotto> purchaseAuto(int money, int numberOfManual) { |
There was a problem hiding this comment.
어디서는 menual이고 여기서는 manual이고 스펠링이 도대체 뭐죠?
| MISS(0, 0); | ||
|
|
||
| private int countOfMatch; | ||
| private int winningMoney; |
| list.stream().forEach(number -> lottoNoList.add(new LottoNo(number))); | ||
| return lottoNoList; | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
이게 과연 유틸인 것인가.. 고민해볼 필요가 있겠습니다
| } | ||
|
|
||
| @Test | ||
| public void 같은지() { |
There was a problem hiding this comment.
테스트 이름이 완전 무성의하고 불친절하네요
| public boolean isSameRank(Rank rank) { | ||
| return this.rank == rank; | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
get set 지워보고 생각해보십쇼 (객체지향 생활체조 규칙 get set 사용을 자제한다)
| import java.util.Collections; | ||
| import java.util.List; | ||
|
|
||
| public class LottoGame { |
There was a problem hiding this comment.
이 클래스가 너무 무거워보입니다. 너무 많은 책임을 갖고 있는건 아닌지 한번 봐보세요
|
가독성을 고려해서 리팩토링 해보세요. |
| return winningLottos; | ||
| } | ||
|
|
||
| public void purchaseMenual(String[] texts) { |
| } | ||
|
|
||
| public int changeUnit(int totalPrice) { | ||
| return totalPrice / 1000; |
| public LottoNo() { | ||
| } | ||
|
|
||
| public LottoNo(int number) { |
There was a problem hiding this comment.
들어오는 number가 1~45의 범위에 해당하는 지,아니라면 에러를 던져주는 것도 이 클래스에 있어야 하지 않을지
|
리뷰 반영했습니다. 리뷰 부탁드려요. |
hyukjin-lee
left a comment
hyukjin-lee
left a comment
There was a problem hiding this comment.
충분히 더 깔끔하게 잘 할 수 있어보이는데, 시간이 없어보이고 구현에 급급해보인 느낌이 고스란히 담겨있네요. 그래서 리뷰 달기가 꺼려지네요. 레이싱게임에 비해 퇴보하는 듯한 느낌을 받았습니다.
https://github.com/code-squad/java-lotto/blob/javajigi/src/main/java/lotto/LottoGame.java
소스코드 보고 깨달은점이나 느낀점 정리해서 자료실에 제출해주세요. 깨달은 점을 바탕으로 다시 리팩토링 하셔도 됩니다. 대신 해당 부분을 그렇게 리팩토링 한 이유에 대해 설명할 수 있어야합니다. 그리고 강의자료에서 로또 피드백 정독하시고 리팩토링 할 때 반영하세요.
| @@ -0,0 +1,21 @@ | |||
| package lotto.domain; | |||
|
|
|||
| public class LottoNo { | |||
| return new Lotto(randomNumbers); | ||
| } | ||
|
|
||
| public Lotto auto() { |
There was a problem hiding this comment.
generateAuto() 가 낫지 않을까요
| randomNumbers = new ArrayList<>(); | ||
| } | ||
|
|
||
| public Lotto manual(String continuousNumber) { |
There was a problem hiding this comment.
generateMenual 이 낫지 않을까요. menual인지 manual인지 통일 시켜달라니까 무시당했네요
4 participants
늦어서 죄송합니다.