-
Notifications
You must be signed in to change notification settings - Fork 248
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
[디디] 로또 수동 입력 #168
[디디] 로또 수동 입력 #168
Conversation
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.
2단계도 잘 구현해주셨습니다.
몇 가지 피드백드렸으니 확인하셔서 반영해주세요~
for (String input : list) { | ||
lottos.add(create(input)); | ||
} | ||
for(int i =0; i< autoCount;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.
띄어쓰기 컨벤션 위반입니다.
public int getManualLotto() { | ||
return manualLotto; | ||
} | ||
|
||
public int getAutoLottoCount() { | ||
return autoLotto; | ||
} |
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인데 서로 네이밍 형식이 다르네요!
@@ -70,4 +70,15 @@ public static Lottos create(int count) { | |||
} | |||
return new Lottos(lottos); | |||
} | |||
|
|||
public static Lottos create(List<String> list, int autoCount){ |
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
라는 이름만 보고 어떤 파라미터인지 유추하기 힘들지 않을까요?
} | ||
|
||
public static void checkRange(String input) { | ||
if(Integer.parseInt(input) < 0) |
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.
여기만 괄호가 없는 이유가 무엇일까요? 일관된 포맷을 가져가면 좋을 것 같습니다~
@@ -19,4 +21,18 @@ public static String getBonusNumber() { | |||
System.out.println("보너스 번호를 입력해 주세요."); | |||
return scanner.nextLine(); | |||
} | |||
|
|||
public static String getCount() { |
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.
getCount()
는 모호한 네이밍입니다. 적절한 네이밍으로 바꿔보면 어떨까요?
for (String input : list) { | ||
lottos.add(create(input)); | ||
} | ||
for(int i =0; i< autoCount;i++) { | ||
lottos.add(create()); | ||
} |
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.
메서드를 추출해 가독성을 높여보면 어떨까요?
public static List<String> getManualLottosNumber(int manualLottoCount) { | ||
List<String> input = new ArrayList<>(); | ||
System.out.println("수동으로 구매할 번호를 입력해 주세요."); | ||
for(int i=0;i<manualLottoCount;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.
컨벤션 위반입니다.
컨벤션에 맞게 띄어쓰기, 중괄호 수정 메서드, 변수명 명확하게 수정 LottoFactory 내의 메소드 makeManualLottos 추출, addAutoLotto 추출
컨벤션에 맞게 띄어쓰기, 중괄호 수정 메서드, 변수명 명확하게 수정 LottoFactory 내의 메소드 makeManualLottos 추출, addAutoLotto 추출
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.
피드백 반영을 잘해주셨네요.
사소한 피드백 드렸으니, 여기까지 반영해주세요~
@@ -0,0 +1,13 @@ | |||
package lotto.domain.result; | |||
|
|||
public class Count { |
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.
이 클래스는 왜 추가되었을까요..?
StringUtil.checkRange(input); | ||
} | ||
|
||
private void validateMoney(String input, int totalLotto) { |
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.
input
을 더 의미있는 이름으로 바꿔보면 좋을 듯 합니다.
input parameter 명 ManualLotto로 수정
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.
피드백 반영 잘해주셨네요!
간단한 피드백 남겼으니, 이전에 피드백 드린 불필요한 파일 삭제와 함께 반영해주세요 :)
public boolean isBonus(){ | ||
if(this==BONUS){ | ||
public boolean isBonus() { | ||
if (this == BONUS) { |
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 this == BONUS
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.
안녕하세요! 피드백 잘 받았습니다! 저번에도 DM보내드렸는데 Count클래스 가 1단계때 함께 제출이 안된것 같아요! push 과정에서 제가 실수한 것 같아요 ㅠㅠ 보내주신 피드백은 수정했는데 Count 파일은 1단계 피드백 반영에 필요한 클래스라 뒤늦게 추가했습니다. 아마도 기존에 Count 클래스가 없어서 갑자기 추가되어서 삭제해달라고 피드백 주신 것 같은데 원래 포함되는게 맞는 클래스입니다!! 혼란스럽게 해서 죄송합니다 ㅠㅠ
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단계때 사용되었다는 말씀이신거죠~? Count
클래스를 사용하는 로직을 못 본 것 같아서 요청드린건데 포함되는게 맞다면 삭제하지 않으셔도 됩니다!
Rank 의 isBonus메서드 리턴 방식 수정
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.
디디 미션진행하시느라 고생많으셨습니다!
머지할게요 :)
감사합니다