Skip to content
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

Java baseball game 김명서 #10

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

mia2583
Copy link

@mia2583 mia2583 commented Dec 3, 2022

코드 작성 해 놓고 commit과 pull request 하는 거 깜빡했네요...
늦어서 죄송합니다!!

지금은 시험 기간이니까 코드 리뷰는 천천히 시간 나실 때 해주셔도 됩니다.
저도 아마 시험 끝나고 나서 리뷰 바탕으로 코드 수정 가능할 거 같습니다.

다른 challenge들도 시험 끝나면 최대한 빨리 시도해볼게요!

- 스트라이크와 볼 개수 계산과 출력
- 잘못된 입력 시 IllegalArgumentException 발생
@mia2583 mia2583 requested a review from shkisme December 3, 2022 12:53
@mia2583 mia2583 self-assigned this Dec 3, 2022

public class Answer {
private int answerNumber;
private static Random random = new Random();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

선언과 동시에 바뀌지 않는 변수라면, final을 사용해도 좋을 것 같아요.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

항상 final과 static은 약간 아무 생각 없이 사용했었는데 덕분에 정확한 정의와 사용 용도 검색해서 이해했네요! 수정했습니다.

Comment on lines 37 to 38
Evaluate.countStrike(guess, hundreds, tens, units);
Evaluate.countBall(guess, hundreds, tens, units);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

static 메서드를 사용하여 클래스의 인스턴스 변수 값을 수정하는데, 코드의 규모가 커지면 사이드 이펙트 문제가 생길 수 있을 것 같아요.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

전혀 고려하지 않았던 요소인데 알려주셔서 감사합니다ㅎㅎ
Evaluate.java 코드를 다시 보니까 단순히 strike와 ball 개수를 세서 클래스 인스턴스 변수 값을 수정하는 게 다였더라고요. 그래서 필드를 사용하지 않는 클래스로 만들어버렸습니다!

Comment on lines 15 to 16
int inputN = validInput(scanner.next());
inputNumber = inputN;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

inputNumber = validInput(scanner.next());

inputNumber에 값을 넣는 코드라면, 처음부터 넣으면 될 것 같아요.

String reuslt = "";

result 변수는 사용하지 않아서 지워도 될 것 같아요.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

리뷰 감사합니다! 수정했어요

Comment on lines 7 to 11
private static int inputNumber;

public static int getInputNumber() {
return inputNumber;
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

inputNumber 변수가 getInput 메서드에 의해 값이 저장되고 getInputNumber 메서드로 값을 가져온다면, static을 빼도 될 것 같아요. (inputNumber 변수의 값을 가져올 때 getInputNumber 메서드를 사용하기 때문입니다.)

추가로 static 키워드는 사용해야 하는 명확한 이유가 없다면, 쓰지 않는게 좋아요. 아래 링크는 관련 글입니다 :)
https://unabated.tistory.com/1041
https://kellis.tistory.com/127
https://12bme.tistory.com/94

Copy link
Author

@mia2583 mia2583 Jan 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

올려주신 글들을 읽으니 static 사용을 줄이는 것이 좋겠다는 생각에 동의하게 되네요. 그치만 여전히 어느 상황에 반드시 사용해야 하는지는 잘 모르겠네요... 코드 보면서 여러 번 고민 해 봐야겠습니다! 피드백 주신 부분은 수정했습니다!

- 단순 대입용으로만 쓰는 변수 inputN 삭제
- 사용하지 않는 변수 result 삭제
- 불필요한 static 키워드 삭제
- 새로운 변수 선언이 아닌 원래 변수에 대입하여 원하는 동작이 되도록 한다.
- 외부에서 사용하지 않는 메소드 private로 선언
- 선언, 할당 후 변하지 않는 함수 final 선언
- 외부에서 클래스 내의 필드 값 변경하지 않도록 수정
- 필드를 생성하지 않고 단순히 strike와 ball 계산 후 값 return
@mia2583 mia2583 force-pushed the java_baseball_game_김명서 branch from c7d7f13 to 03e9195 Compare January 3, 2023 01:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants