Step2 로또(자동) 피드백부탁드립니다~ #3244
Step2 로또(자동) 피드백부탁드립니다~ #3244
Conversation
| private void saveWinningNumber(String winningNumber){ | ||
| for (String number : winningNumberArray(winningNumber)){ | ||
| this.winningNumber.add(Integer.valueOf(number)); | ||
| } | ||
| } | ||
| private String[] winningNumberArray(String winningNumber){ | ||
| if(!validationWinningNumber(winningNumber)){ | ||
| new Exception("입력값이 유효하지 않습니다."); | ||
| }; | ||
| return winningNumber.split(","); | ||
| } | ||
| private Boolean validationWinningNumber(String winningNumber){ | ||
| return winningNumber.split(",").length == 6; | ||
| } |
There was a problem hiding this comment.
LottoStatistics에서 로또 결과값 카운팅, winningNumber 관련된 책임까지 가지면 너무 많은 책임을 가지게 되지 않을까요?
이름 그대로 로또 결과값 카운팅에 대한 책임만 가지고 winningNumber와 관련된 책임은 분리하면 어떨까요?
| for (int i = 0; i < 6; i++) { | ||
| result.add(lottoNumber.get(i)); | ||
| } |
| void addRankQuantity(List<List<Integer>> lottoNumber) { | ||
| for (int i = 0; i < lottoNumber.size(); i++) { | ||
| checkWinningRank(lottoNumber, i); | ||
| } | ||
| } | ||
| int winningAmount(){ | ||
| int amount = 0; | ||
| for (Rank rank : Rank.values()) { | ||
| amount = amount + rank.getTotelMoney(); | ||
| } | ||
| return amount; | ||
| } | ||
| private void checkWinningRank(List<List<Integer>> lottoNumber, int i) { | ||
| if(sameNumberQuantity(lottoNumber.get(i)) >= 3){ | ||
| rankCount.get(sameNumberQuantity(lottoNumber.get(i))).addCount(); | ||
| } | ||
| } |
There was a problem hiding this comment.
for loop 없이 이 로직을 Stream의 groupingBy와 Function.identity를 활용해서 구해볼 수 있을 것 같네요. 한번 학습해보시면 좋겠습니다.
| static Map<Integer, Rank> rankCount = new HashMap<>(); | ||
|
|
||
| static { | ||
| rankCount.put(6, Rank.FIRST); | ||
| rankCount.put(5, Rank.SECOND); | ||
| rankCount.put(4, Rank.THIRD); | ||
| rankCount.put(3, Rank.FOURTH); | ||
| } |
There was a problem hiding this comment.
Rank 자체에 몇개가 일치하는지에 대한 정보가 포함되어 있는데요. 6, 5, 4, 3을 키로 할 이유가 있을까요~?
There was a problem hiding this comment.
Map<Rank, Integer> 형태로 사용하는게 좀 더 적절하지 않을까 하는 생각이 듭니다.
| public void addCount(){ | ||
| count++; | ||
| } |
There was a problem hiding this comment.
enum에 이렇게 프로퍼티를 넣고 값을 증가시키거나 하는 로직의 구현은 피해주세요.
이렇게 사용하면 좋지 않은 이유와 이렇게 구현하면 여러 로또 게임에 대한 결과 집계를 할 수 있을지에 대해서 상수의 특성을 고려해 고민해보시면 좋겠습니다.
| return 0; | ||
| } | ||
|
|
||
| public Float winningRank(List<List<Integer>>lottoNumber){ |
There was a problem hiding this comment.
스페이스 간격이 빠져 있네요. IDE의 자동 정렬 기능을 활용하시고 조금 더 꼼꼼하게 코드를 작성해주세요.
| } | ||
|
|
||
| private int sameNumberIncrease(int number){ | ||
| if(this.winningNumber.contains(number)){ |
There was a problem hiding this comment.
어떤 코드는
for (어떤 코드는 아래와 같이 일관성이 없습니다.
if(IDE의 자동 정렬 기능을 활용하시고 조금 더 꼼꼼하게 코드를 작성해주세요.
| import java.util.Collections; | ||
| import java.util.List; | ||
|
|
||
| public class Lotto { |
There was a problem hiding this comment.
Lotto는 1-45사이의 6개의 숫자를 가져야 합니다. winningNumber도 동일하구요.
지금 Lotto 클래스는 이 로또라는 규칙을 잘 표현하고 있지 않습니다. 로또를 만들기 위한 LottoFactory에 가까워 보여요. List<Integer>를 사용하기 보다는 로또의 특성을 가지는 Lotto 클래스를 만들어보시면 좋겠습니다.
|
|
||
| public class LottoStatistics { | ||
|
|
||
| List<Integer> winningNumber = new ArrayList<>(); |
There was a problem hiding this comment.
위에서 Lotto의 특성을 갖는 클래스를 추출해보라고 말씀드렸는데요. 이를 활용해 WinningNumber 클래스를 만들어보세요.
상속 혹은 컴포지션을 쓸 수 있을 것 같은데요, 컴포지션을 사용해보시고 상속 대비 어떤 이점이 있는지도 한번 찾아보세요.
| for (int i = 0; i < 6; i++) { | ||
| result.add(lottoNumber.get(i)); | ||
| } |
There was a problem hiding this comment.
대부분 for loop를 사용해주셨는데요. Stream을 쓸 수 있는 상황에서는 for loop의 사용을 지양하고 Stream을 활용해보시면 좋겠습니다.
No description provided.