-
Notifications
You must be signed in to change notification settings - Fork 7
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
[숫자 야구 게임] 이세원 과제 제출합니다 (1) #5
base: main
Are you sure you want to change the base?
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.
잘 구현하셨습니다!
유지보수가 용이하게 코드를 리팩토링 해봅시다. 개선해야될 부분들이 많이 있지만 일단 리뷰해드린 내용을 바탕으로 개선해보세요 :)
public int[] makeQuestion(){ | ||
int[] question = new int[3]; | ||
for(int i = 0; i<3; i++){ | ||
int n = Randoms.pickNumberInRange(1, 9); | ||
question[i] = n; | ||
} | ||
|
||
// 중복체크 필요 | ||
boolean flag = duplicateCheck(question); | ||
if(flag){ | ||
return question; | ||
} | ||
return makeQuestion(); | ||
} |
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.
질문의 개수가 4개로 바뀌어도 유지보수가 용이하게 리팩토링 해봅시다.
boolean flag = duplicateCheck(question); | ||
if(flag){ | ||
return question; | ||
} | ||
return makeQuestion(); |
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.
변수명을 flag
라고 지었을 때 어떤 역할을 하는지 파악하기 어렵습니다. isDuplicated
로 바꾸면 가독성이 향상될 것 같습니다.
public boolean printResult(String check){ | ||
String[] split = check.split(""); | ||
|
||
if(split[0].equals("3")){ | ||
System.out.println("3스트라이크"); | ||
System.out.println("3개의 숫자를 모두 맞히셨습니다! 게임 종료"); | ||
return true; | ||
} | ||
else if(split[0].equals("0")&&split[1].equals("0")){ | ||
System.out.println("낫싱"); | ||
return false; | ||
}else if(split[0].equals("0")){ | ||
System.out.println(split[1]+"볼"); | ||
return false; | ||
}else if(split[1].equals("0")){ | ||
System.out.println(split[0]+"스트라이크"); | ||
return false; | ||
}else{ | ||
System.out.println(split[1]+"볼 "+split[0]+"스트라이크"); | ||
return false; | ||
} | ||
} |
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.
early return을 하기 때문에 else if
와 else
가 없어도 정상적으로 동작합니다.
else if
-> if
else
-> 제거
System.out.println("3스트라이크"); | ||
System.out.println("3개의 숫자를 모두 맞히셨습니다! 게임 종료"); |
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.
기능은 얼추 잘 구현한거 같네요 👍
남은 시간 리팩토링에 집중해봅시다.
지금 단 코멘트말고도 리뷰할 건 많은데 너무 길어질까봐 일단 이 정도만 달았어요.
리뷰 없는 함수들도 다시한번 검토해보세요.
public int[] makeQuestion(){ | ||
int[] question = new int[3]; | ||
for(int i = 0; i<3; i++){ | ||
int n = Randoms.pickNumberInRange(1, 9); |
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.
range가 2-7, 1-7로 바뀌어도 큰 변동이 없도록 수정해보세요.
} | ||
throw new IllegalArgumentException(); | ||
} | ||
throw new IllegalArgumentException(); |
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.
아래 비교 예시를 보고 이 함수를 리팩토링 해보세요.
AS IS
if (con1) {
if (con2) {
// 조건이 많아질 수 록 depth가 깊어져서 가독성이 떨어짐
return result;
}
throw new IllegalArgumentException();
}
throw new IllegalArgumentException();
TO BE
if (!con1) {
throw new IllegalArgumentException();
}
if (!con2) {
throw new IllegalArgumentException();
}
return result;
if(question[i]==inputNumbers[j]&&i==j){ | ||
strike++; | ||
} | ||
else if(question[i]==inputNumbers[j]&&i!=j){ | ||
ball++; |
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.
question[i]==inputNumbers[j]
조건이 중복되는데 중복되지 않도록 수정해보세요
} | ||
|
||
// 결과를 어떻게 return 해줘야할까 | ||
return Integer.toString(strike)+(ball); |
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 Integer.toString(strike)+(ball); | ||
} | ||
|
||
public boolean printResult(String check){ |
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개의 역할을 하고 있습니다. 모두 분리해보세요.
- ball/strike 개수에 따라서 어떤 메세지를 출력해야하는지 결정
- 메세지 출력
- 종료 조건 판단
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) 제출합니다
commit과 pr이 어색해서 분리해서 제출하지 못했는데
다음부터는 변경시마다 commit하여 제출하도록 노력하겠습니다.
코드리뷰 부탁드리겠습니다,,!
*배열 >> ArrayList
*변수명 변경
*else if , else 미사용
*조건문 형식 변경
*중복 조건문 삭제
*strike 와 ball 을 가지는 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.
객체를 적극적으로 활용해봅시다. 메소드는 파라미터와 반환값이 존재하고 이를 어떻게 하면 활용할 수 있을지 고민하면 좋겠습니다.
코드는 이전보가 훨씬 나아진 것 같습니다 :)
boolean result = false; | ||
while(!result){ | ||
|
||
// input | ||
Input input = new Input(); | ||
System.out.print("숫자를 입력해주세요 : "); | ||
ArrayList<Integer> inputNumbers = input.getInputNumbers(); | ||
|
||
// 스트라이크, 볼 체크 | ||
Check check = new Check(); | ||
Count count = check.checkResult(question, inputNumbers); | ||
|
||
// 결과처리 | ||
Print print = new Print(); | ||
String printResult = print.printResult(count); | ||
System.out.println(printResult); | ||
|
||
// 종료조건 처리 | ||
if(count.getStrike()==3){ | ||
result = true; | ||
} | ||
} |
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.
while문을 탈출하려면 break
를 사용하면 됩니다. result를 굳이 선언할 필요는 없어보입니다.
// 스트라이크, 볼 체크 | ||
Check check = new Check(); | ||
Count count = check.checkResult(question, inputNumbers); |
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.
Check
를 클래스의 필드로 설정해도 되지 않을까요?
} | ||
|
||
public boolean gameResume(){ | ||
System.out.println("게임을 새로 시작하려면 1, 종료하려면 2를 입력하세요."); |
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 check | ||
if(str.length()!=3){ | ||
throw new IllegalArgumentException(); | ||
} | ||
if(str.contains("0")){ | ||
throw new IllegalArgumentException(); | ||
} |
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과 3이라는 숫자에 의존하는 클래스가 되었습니다. 게임 규칙을 길이가 4인 3-9사이 숫자
로 변경해도 약간의 코드수정만 필요하도록 리팩토링 해보세요.
int strike = count.getStrike(); | ||
String result = ""; | ||
|
||
if(strike==0&&ball==0){ | ||
result += "낫싱"; | ||
} | ||
if(ball>0) { | ||
result += ball + "볼 "; | ||
} | ||
if(strike>0){ | ||
result += strike + "스트라이크"; | ||
} | ||
if(strike==3){ | ||
result += "\n3개의 숫자를 모두 맞히셨습니다! 게임 종료"; | ||
} | ||
return result; | ||
} | ||
} |
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.
메소드를 활용하면 result
라는 변수를 이렇게 선언할 필요도 없지 않을까요?
이세원 과제 제출합니다 (1)