-
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
[숫자 야구 게임] 양재승 과제 제출합니다. #11
base: main
Are you sure you want to change the base?
Conversation
…split each funtion
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.ArrayList; | ||
|
||
public class BaseballGame { | ||
ComputerFunction computerFunction = new ComputerFunction(); |
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.
생성자를 통해서 초기화하도록 바꿔봅시다! 클래스의 구성 요소들을 생각하면서 설계해보세요.
if(restart == NumberInfo.RESTART_TRUE_NUMBER.getNumberInfo()){ | ||
return true; | ||
} | ||
else if(restart == NumberInfo.RESTART_FALSE_NUMBER.getNumberInfo()){ | ||
return false; | ||
} | ||
|
||
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.
early return을 하기 때문에 else if
를 if
로 변경해도 문제가 없어보이네요.
static import
를 적용하면 더 깔끔해질 것 같습니다.
ArrayList<Integer> randomNumberlist = new ArrayList<Integer>(); | ||
computerFunction.createRandomNumber(randomNumberlist); | ||
computerFunction.countCalculate(randomNumberlist); |
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.
비어있는 ArrayList를 파라미터로 넘겨줄 필요가 있나요?
일반적으로 인터페이스를 좌측, 구현체를 우측에 작성합니다. List와 ArrayList의 관계에 대해서 알아보세요.
List<Integer> randomNumberList = new ArrayList<Integer>();
} | ||
|
||
private boolean askRestartGame(){ | ||
System.out.println(Message.RESTART_QUESTION_MESSAGE.getMessage()); |
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.
출력부분을 별도의 메소드로 분리해봅시다.
UserFunction userFunction = new UserFunction(); | ||
public void createRandomNumber(ArrayList<Integer> randomNumber){ | ||
while(randomNumber.size() < NumberInfo.SIZE_OF_NUMBER.getNumberInfo()){ | ||
int tmp = Randoms.pickNumberInRange(NumberInfo.START_OF_RANGE.getNumberInfo(), NumberInfo.END_OF_RANGE.getNumberInfo()); |
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.
이렇게 코드가 길어질 때는 파라미터 사이에 개행을 표시해도 괜찮습니다.
for (int i=0; i<NumberInfo.SIZE_OF_NUMBER.getNumberInfo(); i++) { | ||
int tmp = userNumber.charAt(i) - '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.
SIZE_OF_NUMBER
라는 외부의 값에 의존하는 클래스가 되어버렸습니다. 클래스의 필드로 설정하면 다른 클래스와이 결합도를 낮출 수 있지 않을까요?
public void notIntegerException(String Number){ //정수 이외 값 불가 | ||
for(int i=0; i<NumberInfo.SIZE_OF_NUMBER.getNumberInfo(); i++){ //입력 받은 값이 숫자 이외의 값인 경우 에외처리 | ||
if(!Character.isDigit(Number.charAt(i))){ | ||
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.
꼭 필요한 것이 아니라면 주석사용은 지양합시다. 주석이 없어도 잘 읽히는 코드를 만들어보세요!
public class ExceptionContoller { | ||
public void notIntegerException(String Number){ //정수 이외 값 불가 | ||
for(int i=0; i<NumberInfo.SIZE_OF_NUMBER.getNumberInfo(); i++){ //입력 받은 값이 숫자 이외의 값인 경우 에외처리 | ||
if(!Character.isDigit(Number.charAt(i))){ | ||
throw new IllegalArgumentException(); | ||
} | ||
} | ||
} | ||
public void reduplicationException(String Number){ //중복 불가 | ||
Set<Character> set = new HashSet<Character>(); | ||
for(int i=0; i<NumberInfo.SIZE_OF_NUMBER.getNumberInfo(); i++){ | ||
set.add(Number.charAt(i)); | ||
} | ||
if(!(set.size() == NumberInfo.SIZE_OF_NUMBER.getNumberInfo())){ | ||
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.
예외처리를 꼭 별도의 클래스에서 처리해야 하는지 의문입니다. 각 클래스마다 관심사가 다를텐데 그에 맞는 예외처리를 해당 클래스에서 직접 처리하는 것이 낫지 않을까요? 예외처리라는 자바의 기능적인 요소가 공통점이란 것 외에는 특별한 이유가 안보입니다.
public void zeroValueException(String Number){ | ||
for(int i=0; i<NumberInfo.SIZE_OF_NUMBER.getNumberInfo(); i++){ | ||
if(Number.charAt(i) == '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.
수의 범위를 설정하기 위해 상수들을 별도의 클래스로 NumberInfo 클래스에 정의해놓았습니다. 그러나 이 메소드는 0의 유무를 확인하는 것을 보니 NumberInfo의 숫자를 변경한다고해서 이 프로그램이 사용자의 의도에 맞게 바뀌진 않을 것 같습니다.
기능구현만 완료한 상태이지만, 객체지향적 프로그래밍은 신경쓰지 못하여 완성도가 떨어집니다.
유저 넘버를 받는 함수에서 예외처리 부분에 에러가 나는 상태입니다.
부족한 부분 보완하고 이번 주말동안 리펙토링에 전념하여 성실히 커밋하겠습니다!
테스트 코드에 모두 통과하도록 기능 구현 완료하였습니다.
함수들의 기능을 카테고리에 맞게 클래스로 분리하여 리펙토링 해봤습니다.
BaseballGame class 에는 게임 실행과 재실행 기능으로만 구현했습니다.
UserFunction class 에는 단순히 유저가 값을 입력받고 예외처리 하도록 구현했습니다.
ComputerFunction class 에는 랜덤값을 생성하고 UserFuntion에서 생성된 값을 받아와 결과값과 힌트값을 계산하도록 구현했습니다.
추가로 예외처리 함수들을 따로 class로 빼고 메세지,숫자정보 리터럴을 Enum으로 만들어 가독성과 유지보수에 유리하도록 짜보았습니다.
다만 제가 궁금한 점은, 이처럼 함수의 기능면에서 class로 분리하여 짜는것이 맞는 방향인지 궁금합니다!
특히 ComputerFunction class의 countCalculate함수의 가독성이 많이 떨어져 보이는데, 피드백 받고 전체적인 가독성을 높이는데에 고민해보겠습니다! 감사합니다!