[그리디]이채현 로또 미션 1, 2 단계 제출합니다.#159
Conversation
| private void showStatus(LottoTickets lottoTickets,Money money){ | ||
| Inputview inputview=new Inputview(); | ||
| List<Integer> winningNumber = inputview.getResult(); | ||
|
|
||
| Outputview outputview=new Outputview(); |
There was a problem hiding this comment.
InputView와 OutputView가 메서드마다 새로 생성되고 있는데, Controller 클래스의 필드로 빼서 재사용하면 더 깔끔할 것 같아요!
압력, 결과 집계, 수익률 계산, 출력까지 한 메서드에서 다 하고 있어서 역할이 너무 많아 보이는데요, 메서드 10줄 제한도 맞춰야 하니, 메서드를 분리해서 제약사항을 지킬 수 있도록 수정해 주세요
😊
| public void makeLottoTicket() { | ||
| List<Integer> pool = new ArrayList<>(); | ||
| int MinlottoNumber=1; | ||
| int MaxlottoNumber=45; | ||
| for (int i = MinlottoNumber; i <= MaxlottoNumber; i++) { | ||
| pool.add(i); | ||
| } | ||
| Collections.shuffle(pool); | ||
|
|
||
| lottoNumbers.clear(); | ||
| lottoNumbers.addAll(pool.subList(0, 6)); | ||
|
|
||
| Collections.sort(lottoNumbers); | ||
| } |
There was a problem hiding this comment.
현재 구조도 충분히 자연스럽게 보이지만
다만 번호 생성 방식이 메서드 내부에 직접 들어가 있어,
이후 규칙 변경이나 테스트가 필요할 때는 조금 다루기 어려울 수 있을 것 같아요.
그래서 우선은 번호 생성 로직을 별도 클래스로 분리해 보면 어떨까요?
| public LottoResult() { | ||
| init(); | ||
| } | ||
|
|
||
| public void init() { | ||
| for (LottoRank lottoRank : LottoRank.values()) { | ||
| lottoStatus.put(lottoRank, 0); | ||
| } | ||
| } |
There was a problem hiding this comment.
생성자는 원래 객체를 초기화하는 책임을 가지고 있어서, 지금 정도의 로직이라면 init()을 따로 분리하지 않고 생성자 안에서 바로 처리해도 충분할 것 같아요!
아니면 분리를 하더라도 init()처럼 조금 추상적인 이름보다는, 이 메서드가 무엇을 초기화하는지 드러나는 이름으로 지어주면 더 좋을 것 같습니다.
지금의 init()이라는 이름은 은 생성자와 역할이 크게 다르지 않게 느껴져서,
왜 따로 분리했는지가 코드만 봐서는 잘 드러나지 않는 것 같아요
그리고 중요한 점은
현재 이 메서드는 클래스 내부에서만 사용되고 있는데 public으로 열려 있어서 외부에서도 호출할 수 있는 상태인데요,
이런 식으로 클래스 내부에서만 사용되는 메서드라면 private으로 접근 범위를 제한해 주세요🙋♂️
+) private로 제한자를 변경하면 어떤 점이 좋아질 것 같으신기요??
There was a problem hiding this comment.
private으로 생성함으로써, 초기화가 다른 외부에서 되지 않도록 방지하는데 도움된다고 생각해요!
src/main/java/view/Inputview.java
Outdated
| private final int Minnumber=1; | ||
| private final int Maxnumber=45; | ||
| public int validateNumber(String input){ | ||
| int number=parseInt(input); | ||
| if(number<1||number>45){ | ||
| throw new IllegalArgumentException("[ERROR] 숫자는"+Minnumber+"~"+Maxnumber+"까지 입력 가능"); | ||
| } | ||
| return number; | ||
| } |
There was a problem hiding this comment.
이런 리뷰를 남겼었는데요,
P.S. 어떤 종류의 유효성 검사가 MVC 3가지 계층 중 어느 계층의 책임일지도 한번 고민해 보시면 좋을 것 같아요!
예를들어
로또 번호에 99 라는 정수가 입력된다면?
숫자를 구분하는 구분자가 , 가 아니라 # 같은 다른 문자라면?
이러한 각각의 상황들의 유효성 검사의 책임은 Domain? View? Controller? 중 어느곳에 위치하는 게 좋을지 고민해 보고 유효성 검사 로직을 추가해 주세요!
1~45 범위를 InputView에서 검증하고 있는데, 이 부분은 입력 형식 검증보다는 로또의 비즈니스 규칙에 더 가까워 보여요
개인적으로 View는 입력 형식 등을,
도메인은 비즈니스 제약을 검증하도록 나누어보면 더 자연스러울 것 같습니다
src/main/java/model/LottoRank.java
Outdated
| LottoRank(int matchCount,int winningMoney){ | ||
| this.matchCount=matchCount; | ||
| this.winningMoney=winningMoney; | ||
| } | ||
|
|
||
| public int getMatchCount(){ | ||
| return matchCount; | ||
| } | ||
| public int getWinningMoney(){ | ||
| return winningMoney; | ||
| } | ||
|
|
||
| public static LottoRank find(int match){ | ||
| for (LottoRank lottoRank: values()){ | ||
| if(lottoRank.matchCount==match){ | ||
| return lottoRank; | ||
| } | ||
| } | ||
| return MISS; | ||
| } | ||
|
|
There was a problem hiding this comment.
요기 생성자 파라미터 사이에 공백이 없어서 읽기가 좀 어렵네요
IntelliJ에서 cmd + option + L 단축키를 누르면 자동으로 코드 포맷을 맞춰줄 수 있습니다
또한 메서드들 사이의 공백도 어떤 곳은 없고, 어떤 곳은 두세 줄씩 있어서 전체적으로 일관성이 살 짝 부족해 보이는데, 일관성 있게 바꿔주면 좋을거 같아요 !
추가로 find()처럼 비즈니스 로직이 있는 public 메서드는,
getXXX()처럼 단순히 값을 반환하는 메서드보다 위에 배치하면 코드를 읽을 때 흐름을 파악하기 더 편한 것 같슴니다
P.S.
이런 공백이나 정렬까지 리뷰를 남겨서 조금 깐깐하게 느껴질 수도 있을 것 같은데요,
협업할 때는 이런 사소한 스타일이 맞춰져 있을수록 코드를 읽거나 수정할 때 피로도가 많이 줄어드는 것 같아서 함께 말씀드려봤습니다 🙂
(가독성이 좋은 코드를 짜는 습관이 사소한 것 같지만 프로젝트 때 크게 체감하실 수 있을거에요)
src/main/java/model/Money.java
Outdated
| public int getProfit(Map<LottoRank,Integer>status){ | ||
| int total=0; | ||
| for(LottoRank lottoRank:LottoRank.values()){ | ||
| int count=status.getOrDefault(lottoRank,0); | ||
| if(lottoRank==MISS){ | ||
| return total; | ||
| } | ||
| total+=lottoRank.getWinningMoney()*count; | ||
| } | ||
| return total; | ||
| } | ||
|
|
||
| public double calculateProfit(int profit) { | ||
| double rate = (double) profit / money; | ||
| return rate; | ||
| } |
There was a problem hiding this comment.
getProfit() 메서드는 현재 depth가 1을 넘고 있네요.
그리고 이미 등수별 개수가 담긴 Map을 받고 있어서, 이 부분은 더 간단하게 리팩토링해볼 수 있을 것 같습니다
또한 getProfit()과 calculateProfit() 모두 profit이라는 이름을 사용하고 있어서, 하나는 총 당첨금인지 다른 하나는 수익률인지 바로 구분하기 어려운 것 같아요.
각 메서드가 반환하는 의미가 드러나도록 네이밍도 함께 수정해보시면 좋을 것 같아요 👍
src/main/java/model/LottoTicket.java
Outdated
| int MinlottoNumber=1; | ||
| int MaxlottoNumber=45; |
There was a problem hiding this comment.
매직 넘버는 저번에 코멘트에 언급 했던 것 처럼 상수로 분리해 주세요!!
| public LottoTickets(int quantity) { | ||
| this.quantity = quantity; | ||
| } | ||
|
|
||
| public void buyTickets() { | ||
| for (int i = 1; i <= quantity; i++) { | ||
| LottoTicket lottoTicket = new LottoTicket(); | ||
| tickets.add(lottoTicket); | ||
| } | ||
| } |
2 participants
기존 pr이 closed되어서 문제 해결하고 다시 했습니다!