[Team8] Lotto Step1 입니다. #15
Conversation
javajigi
left a comment
javajigi
left a comment
There was a problem hiding this comment.
전체적으로 구현은 잘 했어요. 단, 결과 처리를 담당하는 객체를 하나 더 추가하면 어떨까라는 의견과 enum 사용에 대해 좀 더 고민해 보면 좋겠네요.
| - 1) 로또결과 (로또 객체와 완전 상관없는 부분) 따로 빼고 클라이언트에서 호출 | ||
| - 2) 네이밍 | ||
| - 3) 유틸 | ||
| - 4) 접근 제어 No newline at end of file |
|
|
||
| public class HitNumber { | ||
| final List<Integer> hitNumbers; | ||
| static final String DELIMITER = ", "; |
There was a problem hiding this comment.
클래스의 구현 순서는 다음과 같다. 다음 원칙에 따라 구현한다.
class A {
상수(static final) 또는 클래스 변수
인스턴스 변수
생성자
메소드
}
| try { | ||
| num = toInt(number); | ||
| } catch (NumberFormatException e) { | ||
| return false; |
There was a problem hiding this comment.
이 부분은 굳이 try/catch로 처리하지 않아도 되지 않을까?
그냥 exception throw하도록..
좀 과하다는 느낌이 듦
|
|
||
| import java.util.List; | ||
|
|
||
| public class Lotto { |
There was a problem hiding this comment.
HitNumber와의 차이점은??
같은 Lotto로 사용해도 되지 않나?
| resultAwardMap.put(3, basicAwards.getThree_correct()); | ||
| resultAwardMap.put(4, basicAwards.getFour_correct()); | ||
| resultAwardMap.put(5, basicAwards.getFive_correct()); | ||
| resultAwardMap.put(6, basicAwards.getSix_correct()); |
There was a problem hiding this comment.
convention 위반
변수 이름, 메소드 이름에 "_"를 사용하지 않는다.
camel 컨벤션을 따른다.
|
|
||
| public static int maxLottoToBuy(Money money) { | ||
| return money.getMoney() / LOTTO_PRICE; | ||
| } |
There was a problem hiding this comment.
수익률을 계산하고 결과 처리를 담당하는 새로운 객체를 추가하는 것은 어떨까?
LottoClient가 너무 많은 일을 하고 있는 것 같다.
| resultAwardMap.put(6, basicAwards.getSix_correct()); | ||
| } | ||
|
|
||
| private enum LOTTO_AWARDS { |
There was a problem hiding this comment.
enum을 적절하게 사용하지 못하고 있는 느낌이다.
enum을 어떤 용도로 사용할 것인지 좀 더 고민해 보면 좋겠다.
|
|
||
| public int getMoney() { | ||
| return money; | ||
| } |
There was a problem hiding this comment.
Money가 더 많은 일을 할 수 있지 않을까?
몇 장의 Lotto를 구매할 수 있는지, 수익율 등도 이곳에서 처리할 수 있지 않을까?
돈과 관련된 모든 로직을 구현한다면??
|
|
||
| @Test | ||
| public void 로또구입금액딱맞음() { | ||
| assertThat(LottoClient.maxLottoToBuy(new Money(5000))).isEqualTo(5); |
3 participants
8조 코드리뷰 부탁드립니다.