-
Notifications
You must be signed in to change notification settings - Fork 329
[빙봉] 로또 1단계 - 자동 미션 제출합니다. #149
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- 로또 티켓 갯수를 구해주는 calculateLottoTicket() 메서드 추가 - refactor : WinningLottoTicket 클래스에서 정규표현식 제거
- WinningLottoTicket 클래스에 private 접근제한자 추가
- LottoNubmers 클래스명을 LottoNumbersGenerator로 변경 - getTotal 메서드명 수정 - getCorrectCount 메서드에서 인덱스로 비교하는 것을 contains로 수정
- String.Utils의 isBlank 메서드를 spark 패키지에서 apache 패키지로 변경
jihan805
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요! 전체적으로 깔끔하게 구현 잘하셨어요 💯
몇가지 피드백 남겼으니 확인해주세요 :)
src/main/java/domain/LottoType.java
Outdated
| import java.util.function.Function; | ||
|
|
||
| public enum LottoType { | ||
| MATCH_THREE(3, "3개 일치 (5000원) - ", value -> value * 5000), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 돈을 나타내는 숫자의 경우 천 단위를 구분하기 위해 "1_000" .
- LottoType 보다는 MatchType, RankType 등의 네이밍이 더 어울리지 않을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
가독성을 향상시키기 위해 숫자에 언더스코어를 추가했고 등수마다 상금이 달려있어 RankType이라는 네이밍으로 변경했습니다!
|
|
||
| WinningLottoTicket winningLottoTicket = new WinningLottoTicket(InputView.inputWinningNumber()); | ||
| winningLottoTicket.initializeBonusBall(InputView.inputBonusNumber()); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
개선 :
WinningLottoTicket winningLottoTicket =
new WinningLottoTicket(InputView.inputWinningNumber(), InputView.inputBonusNumber());
WinningLottoTicket 안에서 보너스번호와 우승번호의 중복 체크를 하는 것이 더 적절할것 같아요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
넵 isMatchBonusBall() 메서드로 중복체크 하도록 수정했습니다!
| LottoTickets lottoTickets = new LottoTickets(originalLottoTickets); | ||
|
|
||
| LottoResults lottoResults = lottoTickets.match(winningLottoTicket); | ||
| HashMap<String, Integer> map = lottoResults.getCountMap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
LottoResults lottoResults = lottoTickets.match(winningLottoTicket);를 LottoPlayer 혹은 LottoManager, LottoMachine 등등의 클래스로 추출할 수 있지 않을까요?
-
Map을 이용한 구현 💯
LottoResults가 List대신 Map<LottoType, Integer> map을 변수로 가지고 있도록 구현해보면 어떨까요?
그럼 게임 결과를 종합/추출 하는 과정이 훨씬 깔끔해질 것 같아요!
추가로 선언시 HashMap대신 Map을 이용하도록 해요! (이유에 대해서도 찾아보면 어떨까요?)
전체적인 구현 👍 다만 결과와 관련된 부분에 대해서 한번 더 고민해보면 좋을 것 같아요 !
피드백으로 인해 변경해야 하는 테스트 코드가 생긴다면 테스트 코드에도 반영해주세요 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
match() 메서드를 LottoManager 클래스로 옮기다보니 LottoTickets 클래스에는 LottoTicket를 List로만 갖고 있고 따로 검증할 로직이나 비즈니스 로직이 없습니다. 이렇게 검증할 로직이나 비즈니스 로직이 없어도 괜찮은지 궁금합니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 선언을 HashMap 대신 Map을 이용하는 이유
Map은 인터페이스고 HashMap은 Map을 구현한 클래스입니다.
- 인터페이스라서 선언하게 되면 확장할 수 있습니다. 즉 HashMap 뿐만 아니라 TreeMap 등으로 확장할 수 있습니다. Ex)
Map<String, Integer> map = new TreeMap<>(); - 선언에 구체적인 타입을 명시할 경우, 호출하거나 확장하는 데 있어 불필요한 제약조건을 추가하는 것이 됩니다.
조금이라도 의도가 명확해지는 코드를 작성하기 위해 질문하시지 않았을까 생각해봅니다!
| return sum; | ||
| } | ||
|
|
||
| public static long getProfit(HashMap<String, Integer> map, int money) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수익률 계산 등 돈과 관련한 모든 로직을 Money 클래스를 이용할수도 있을것 같아요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수정했습니다!
| } | ||
|
|
||
| private void validateEmpty(String input) { | ||
| if (StringUtils.isBlank(input)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
StringUtils.isEmpty() 에 대해서 찾아보면 어떨까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
공백입력에 대해서도 에러로 처리해주고 싶어 StringUtils.isEmpty()가 아닌 StringUtils.isBlank() 메서드를 사용했습니다! 지금 보니 StringUtils.isBlank() 메서드는 null도 처리해주는데 null을 처리해주는 메서드가 따로 있는 걸 발견해 validateNull() 메서드를 삭제하고 validateEmpty() 메서드명을 validateBlank()로 수정했습니다.
리뷰어님의 질문 의도가 메서드명이 validateEmpty()로 되어있는데 정작 메소드 명은 StringUtils.isBlank()를 사용하고 있어서 StringUtils.isEmpty()에 대해서 찾아보라고 하신건지, 아니라면 어떤 의도로 말씀하신 건지 궁금합니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
두 경우를 나누어서 구현할 필요가 있을까 하여 남겼습니다
isEmpty()의 docs를 읽어보면
"This method accepts any Object as an argument, comparing it to* {@code null} and the empty String"
남겨주신 의도를 읽어보니 isBlank를 사용하여 구현 👍
|
|
||
| @DisplayName("로또 당첨갯수 확인 메서드 테스트") | ||
| @ParameterizedTest | ||
| @CsvSource(value = {"MATCH_THREE,1"}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
조금 더 다양한 케이스를 테스트 해보면 좋을 거 같아요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수정했습니다!
…, WinningLottoTicket의 생성자에서 bonusBall을 받도록 수정
…toManager 클래스 추가, 테스트코드 수정 - LottoResult 클래스 명을 LottoCount로 변경 - LottoResults 클래스에서 add() 메서드 삭제 - LottoTicket 클래스에 있던 getCorrectCount() 메서드를 WinningLottoTicket 클래스로 옮김 - LottoTickets 클래스에 있던 match() 메서드 삭제 - LottoResultsTest 클래스 삭제 - LottoTicketsTest 클래스 삭제
jihan805
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요 피드백 반영 잘하셨어요 👍
몇가지 피드백 확인 후 다음단계 진행하면 좋을 것 같아요 !!!
| import java.util.List; | ||
|
|
||
| public class LottoTickets { | ||
| private List<LottoTicket> lottoTickets; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
필요하다면 검증로직이 있는게 당연하지만, 검증할 로직이나 비즈니스 로직이 없다고 해서 틀린 것은 아니라고 생각합니다! :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
답변 감사합니다!
| } | ||
|
|
||
| private void validateEmpty(String input) { | ||
| if (StringUtils.isBlank(input)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
두 경우를 나누어서 구현할 필요가 있을까 하여 남겼습니다
isEmpty()의 docs를 읽어보면
"This method accepts any Object as an argument, comparing it to* {@code null} and the empty String"
남겨주신 의도를 읽어보니 isBlank를 사용하여 구현 👍
|
|
||
| addTicketNumber(winningTicket, inputWinningTicket); | ||
| this.winningTicket = new LottoTicket(winningTicket); | ||
| this.bonusBall = new BonusBall(this.winningTicket.getLottoTicket(), inputBonusBall); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BonusBall의 생성자에 winningTicket 을 전달할 필요가 있을까요?
해당 클래스에서도 충분히 보너스볼이 중복인지 확인이 가능할것 같아요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
넵 객체의 역할로 봤을 때 WinningLottoTicket 클래스가 winningTicket과 bonusBall 둘 다 갖고 있으므로 WinningLottoTicket 클래스에서 중복을 판단하는 것이 맞는 것 같습니다! 수정했습니다!
| public static Map<RankType, Integer> match(LottoTickets lottoTickets, WinningLottoTicket winningLottoTicket) { | ||
| LottoResults lottoResults = new LottoResults(); | ||
| lottoResults.putLottoResults(getLottoCounts(lottoTickets, winningLottoTicket)); | ||
| return lottoResults.getLottoResults(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
get하여 리턴하는 것도 좋지만, LottoResults 클래스를 리턴해도 좋을 것 같아요 !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
실수로 여기 남겨주신 코멘트를 삭제했네요 ㅠㅠ 내용 기억하였다가 답변드릴게요!
| return lottoResults.getLottoResults(); | ||
| } | ||
|
|
||
| private static List<LottoCount> getLottoCounts(LottoTickets lottoTickets, WinningLottoTicket winningLottoTicket) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Map<RankType, Integer> map 자료구조를 이용하여 결과를 만들어낸 부분 👍
하지만, List를 사용하지 않고 바로 Map 결과를 만드는 방법도 있을 것 같아요! 고민해보면 어떨까요?(LottoCount 클래스를 사용하지 않을수 있을 것 같아요!!)
또 lottoResults.putLottoResults() 과 같은 방식의 구현도 💯 이지만, return new LottoResults(lottos, winning~) 과 같이 LottoResult 클래스에서 결과를 구해도 좋을 것 같아요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LottoCount에 들어가는 로직을 RankType(Enum)으로 빼서 RankType을 반환하도록 구현했습니다!
src/main/java/domain/LottoCount.java
Outdated
| this.ballCount = validateFiveWIthBonusBall(ballCount, matchBonusBall); | ||
| } | ||
|
|
||
| private int validateFiveWIthBonusBall(int correctCount, boolean matchBonusBall) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
validateFiveWIthBonusBall -> validateFiveWithBonusBall 소문자 i
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LottoCount 클래스는 사라졌지만 대소문자 지적 감사합니다! 컨벤션을 지키도록 노력하겠습니다!
| validateBlank(inputBonusBall); | ||
| List<Integer> winningTicket = new ArrayList<>(); | ||
|
|
||
| addTicketNumber(winningTicket, inputWinningTicket); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이런 경우 List winningTicket = new ArrayList<>(); 후 winningTicket을 메서드에 전달하는 방법도 있지만
addTicketNumber 메서드의 리턴 타입을 List로 구현할수도 있지 않을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
리턴 타입을 List로 구현하라고 말씀하신 이유는 winningTicket 인자를 빼서 관계를 최소화하기 위한 뜻인가요?
| private static final int MIN_LOTTO_NUMBER_RANGE = 1; | ||
| private static final int MAX_LOTTO_NUMBER_RANGE = 45; | ||
|
|
||
| private List<Integer> lottoTicket; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"규칙 3: 모든 원시값과 문자열을 포장한다." 원칙을 지키는 연습을 해본다.
숫자 값 하나를 추상화한 LottoNumber 객체를 추가하고, 객체의 생성자에서 1 ~ 45의 값인지에 대한 유효성 체크를 하는 것은 어떨까요?
또한 LottoNumber 인스턴스를 생성한 후 재사용할 수 있도록 구현해보면 좋을것 같아요!
Bonusball 역시 LottoNumber를 활용할 수 있지 않을까요? (예, LottoNumber bonusNumber)
힌트 : Map과 같은 곳에 인스턴스를 생성한 후 재사용하는 방법.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
넵 말씀하신대로 구현했습니다! 1~45까지 만든 숫자를 재사용하기 위해서 LottoNumberGenerator에 getter를 만들어 같은 인스턴스로 접근하도록 하긴 했지만 이것이 맞는 방법인지, 더 괜찮은 방법은 없는지 궁금합니다. 조언해주시면 감사하겠습니다!
- LottoCount에 대한 메서드 삭제 - LottoResults 클래스의 putLottoResults 메서드 수정 - OutputView에 printWinningStatistics 메서드에서 Map을 반복할 때 entry 방법이 아닌 foreach로 변경
- List를 메서드 안에서 만들고 반환하도록 로직 수정
…ator 에서 같은 인스턴스로 가져오도록 getter 메서드 추가, OutputView 에서 Map이 아닌 RankType으로 출력 수정
jihan805
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
피드백 반영 잘하셨어요 💯 머지할게요!!
간단한 코멘트 남겼으니 확인해주세요 :)
| LottoResults lottoResults = new LottoResults(); | ||
| lottoTickets.getLottoTickets() | ||
| .forEach(lottoTicket -> lottoResults.putLottoResults(lottoTicket, winningLottoTicket)); | ||
| return lottoResults; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
getter를 어디에서 사용하냐? 보다 이미 LottoResults라는 클래스로 포장하였으므로 get하는 대신 내가 잘 포장한 클래스를 리턴하는것이 더 적절하지 않을까요?
LottoManager.match의 결과는 LottoResults를 반환해준다는 것도, 메서드 내부를 보지 않고도 바로 파악할 수 있고요.
추가로 LottoResults에게 putLottoResults하는 대신, 게임 결과를 구하는 역할을 모두 위임하면 어떨까요? 그렇다면 LottoResults 클래스에서 게임 결과를 구하는 로직에 대한 테스트 코드도 필요할 것 같아요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
LottoResults 처럼 클래스로 포장해 리턴시키면 메서드 내부를 보지 않고도 클래스 이름 자체로 정보를 줄 수 있기때문에 파악하기 쉬워 더 적절하다고 생각합니다. 지적 감사합니다!
-
'상태를 변경하는 메서드를 노출시키면 안되며 객체의 역할 분배를 고려하자.' 알려주셔서 감사합니다!
| validateBlank(inputWinningTicket); | ||
| validateBlank(inputBonusBall); | ||
|
|
||
| List<LottoNumber> winningTicket = createWinningTicket(inputWinningTicket); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
처음에 사용하였던 방식처럼 구현해야하는 경우도 분명히 존재하겠지만, 지금 구현에서는 return 타입을 반환하는 것이 조금 더 적절하다고 생각합니다 !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수정했습니다!
| .collect(Collectors.toList()); | ||
| } | ||
|
|
||
| public static LottoNumber getOriginLottoNumber(int index) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
혹은 LottoNumber 내 Map<Integer, LottoNumber>
정적 팩터리 메서드를 이용해 생성하고자 하는 번호(number)를 get하여 return 하는 방법도 있을 것 같습니다.
지금의 구조라면, LottoNumber를 사용하기 위해서는 LottoNumbersGenerator와 관련이 없더라도 무조건 LottoNumbersGenerator로 부터 get 해야 하지 않을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LottoNumber 내 정적 팩토리 메서드를 사용해 Map으로 만드니 List의 인덱스가 아니라 Map의 get으로 접근해 너무 편리해졌습니다. 감사합니다!
| LottoTickets lottoTickets = generateLottoTicketsByMoney(money); | ||
| OutputView.printBuyTicketCount(money.calculateLottoTicket()); | ||
| OutputView.printLottoTickets(lottoTickets.getLottoTickets()); | ||
| OutputView.printWinningStatistics(LottoManager.match(lottoTickets, inputWinningLottoTicket()).getLottoResults(), money.getMoney()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 한줄로 축약하는 것도 좋지만,
조금 가독성을 높이 기 위해 나누어보면 어떨까요? - printWinningStatistics에 get 하여 넘기는 대신 그냥 LottoResults를 넘겨도 괜찮을 것 같아요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
가독성 좋게 나눈다고 나눴지만 잘 나눴는지는 모르겠습니다. 피드백 해주시면 감사하겠습니다!
-
수정했습니다!
|
|
||
| private static double getTotalWinningPrice(Map<RankType, Integer> map) { | ||
| double sum = INIT_SUM_VALUE; | ||
| for (Map.Entry<RankType, Integer> entry : map.entrySet()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 네이밍시, map 과 같이 자료형을 나타내지 않도록 해요!
- for(RankType type : map.keySet()) {
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
변수명에 자료구조의 단어가 들어가면 안 된다는 걸 알고 있었는데 깜박하고 지나친 것 같습니다. 지적 감사합니다!
-
간단하게 RankType을 모두 돌자고 생각해서 이 이후에
for (RankType type : RankType.values()) {}로 생각하고 있었는데 다시 생각해보니 써머님과 같이 매개변수로 온 Map을 가지고 도는 방법for (RankType type : map.keySet()) {}이 map에 들어간 것만 확인하기 때문에 더 안전한 코드라고 생각합니다. 조언 감사합니다!
|
|
||
| import org.apache.commons.lang3.StringUtils; | ||
|
|
||
| public class BonusBall { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BonusBall 클래스를 유지해도 좋지만, LottoNumber를 추출한 이상
LottoNumber bonusNumber와 같이 사용해도 좋을것 같아요 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
단순히 int형으로 들어오는 번호와 입력(String)으로 들어오는 보너스 번호를 생성자를 통해 나눠 구현했습니다!
|
|
||
| public class LottoNumberTest { | ||
|
|
||
| @DisplayName("LottoNumber 생성자 테스트") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
생성자에 대해 무엇을 테스트하는지 테스트 메서드 네이밍만 보고는 알 수 없어요.
어떤 테스트를 해서 어떤 결과를 기대하는지 등 테스트 메서드 네이밍에 대해서 찾아보고 참고해보면 어떨까요?
전체적으로 확인해주세요 :)
다음 단계 미션 진행전
- 피드백을 반영하여 커밋로그로 기록
- 다음단계 진행
해주세요 !!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
테스트 메서드 네이밍에 여러 방법이 있는데 그 중 Should, When으로 써보긴 했습니다만 한국어와 맞지 않는 느낌이 들었습니다. 현업에서는 보통 어떤 방식으로 쓰는지 궁금합니다!
안녕하세요 써머님! 빙봉입니다!
많은 피드백 부탁드립니다! 감사합니다!