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 이현 #6

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

Java baseball game 이현 #6

wants to merge 10 commits into from

Conversation

hyuunnn
Copy link

@hyuunnn hyuunnn commented Oct 3, 2022

기능은 금방 구현했는데 코드를 어떻게 배치하면 효율적일까 고민하다 보니 늦었네요..

열심히 고쳐보겠습니다..

@hyuunnn hyuunnn self-assigned this Oct 3, 2022
@shkisme shkisme self-requested a review October 3, 2022 15:35
@shkisme shkisme requested review from geunskoo and llddang October 5, 2022 09:46
Copy link
Contributor

@shkisme shkisme left a comment

Choose a reason for hiding this comment

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

리뷰가 늦어진 점 죄송합니다...! 🙇‍♀️
전체적으로 코드가 굉장히 깔끔하게 잘 작성되어 있다는 느낌을 받았어요...! 👍👍👍
테스트 코드도 한번 작성해보시면 좋을 것 같아요😄


public static void main(String[] args) {
Baseball baseball = new Baseball();
baseball.run();
Copy link
Contributor

Choose a reason for hiding this comment

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

main 함수가 깔끔하게 잘 작성된 것 같습니다! 👍

}

public String inputString(final Scanner scanner) {
System.out.printf("숫자를 입력해주세요 : ");
Copy link
Contributor

Choose a reason for hiding this comment

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

이거 좀 사소한 부분인데, printfprint로 바꾸셔도 될 것 같습니다!😂

Copy link
Author

Choose a reason for hiding this comment

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

반영하겠습니다.


public void validateInputNumber(final String inputNumber) {
validateLength(inputNumber);
validateNumberRange(inputNumber);
Copy link
Contributor

Choose a reason for hiding this comment

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

오류들도 메서드로 각 각 분리한 점 좋습니다👍


public String randomize() {
final Random r = new Random();
final Set<Integer> numberList = new LinkedHashSet<>();
Copy link
Contributor

Choose a reason for hiding this comment

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

Set으로 중복되는 값은 아예 리스트에 추가하지 않게끔 코드를 구현하셨네요! 👍👍
그런데 혹시 LinkedHashSet 자료구조를 사용하신 이유가 있으신가요?

Copy link
Author

Choose a reason for hiding this comment

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

입력한 순서대로 데이터를 저장하기 위해 사용하였습니다.
https://hun-developer.tistory.com/38 사이트 참고하였습니다.

Copy link
Contributor

Choose a reason for hiding this comment

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

HashSet은 순서를 보장하지 않는다는 특징이 있군요... 아직 자료구조는 잘 몰라서, 이 부분 알아갑니다!👍

return randomNumberAt == inputNumberAt;
}

public boolean checkBall(final String inputNumber, final char randomNumberAt) {
Copy link
Contributor

Choose a reason for hiding this comment

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

뭔가 대부분의 파라미터로 넘겨받는 값에 final 키워드를 사용하신 것 같아요. 혹시 이렇게 키워드를 붙이신 이유가 있으신가요?

Copy link
Author

@hyuunnn hyuunnn Oct 6, 2022

Choose a reason for hiding this comment

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

"사이드 이펙트가 발생하지 않는다!"라는 생각으로 사용했는데
지금 생각해보면 남용했다는 느낌도 드네요.
자바를 사용한 타 프로젝트들을 봐도 매개변수에 final은 안쓰는 것 같네요..

Copy link
Contributor

Choose a reason for hiding this comment

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

생각해봤는데, 말씀하신 것처럼 내부에서 재할당해서 그걸 사용하는 것을 막는 것에는 효과가 좋은 것 같아요. 코드를 딱 봤을 때 재할당 금지! 이런 느낌을 확 전달하는 것 같아 나쁘진 않은것 같습니다! 적절하게 사용하면 좋은 것 같아요👍

scanner.close();
}

public void run() {
Copy link
Contributor

Choose a reason for hiding this comment

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

이 메서드는 main에서 사용할 때 Baseball.run() 이렇게 사용되기 때문에 네이밍을 baseBallRun이 아니라 run으로 하셨나요?

Copy link
Author

@hyuunnn hyuunnn Oct 6, 2022

Choose a reason for hiding this comment

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

맞습니다. Baseball 객체를 생성해서 시작하기 때문에 run이 더 간결하고 직관적이라고 생각했습니다.
파이썬으로 개발할 때도 run 메서드를 따로 만드는 편인데, 좋은 방법인지는 모르겠습니다 ㅎㅎ..

Copy link
Author

Choose a reason for hiding this comment

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

지금 생각해보면 startBaseballstart가 더 간결하지 않나 생각이 드네요

Copy link
Contributor

Choose a reason for hiding this comment

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

BaseBall 클래스 안의 메서드들이므로, runstart로 네이밍 해도 좋은 것 같네요!

final private int GAME_END = 2;
final private int BALL_LENGTH = 3;

public String randomize() {
Copy link
Contributor

Choose a reason for hiding this comment

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

private으로 설정해도 되는 메서드들이 전부 public으로 열려 있는 것 같아요!

Copy link
Author

@hyuunnn hyuunnn Oct 7, 2022

Choose a reason for hiding this comment

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

아 수정하겠습니다!

System.out.println("3개의 숫자를 모두 맞히셨습니다! 게임 종료");
System.out.println("게임을 새로 시작하려면 1, 종료하려면 2를 입력하세요.");
final int number = scanner.nextInt();
int number = Integer.parseInt(scanner.nextLine());
Copy link
Contributor

@shkisme shkisme Nov 5, 2022

Choose a reason for hiding this comment

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

찾기 어려운 오류였네요... nextInt를 사용하면 개행 처리를 하지 않는다... 저도 몰랐던 부분이였는데 주의하면서 사용해야겠습니다!👍

System.exit(0);
} else {
throw new IllegalStateException("입력 값이 1 또는 2가 아닙니다.");
}
}

private void start(final String randomNumber) {
final Scanner scanner = new Scanner(System.in);
private void start(String randomNumber) {
Copy link
Contributor

Choose a reason for hiding this comment

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

자바의 경우, 메소드가 순서에 영향을 받지 않습니다. 자바 컴파일러가 자동으로 모든 메소드를 앞쪽에 선언하기 때문입니다! 예를 들어 start를 호출하고있는 run함수 위에 start 함수를 배치할 필요가 없습니다.
또한 클린 코드 책 5장을 보면, 사용하는 함수를 바로 밑에 선언하는 것을 추천하고 있습니다!
참고하시면 좋을 것 같아요😊

Copy link
Author

Choose a reason for hiding this comment

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

감사합니다 :)

Copy link
Contributor

@shkisme shkisme left a comment

Choose a reason for hiding this comment

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

다음 챌린지로 넘어가셔도 좋을 것 같습니다!! 수고하셨습니다 👍👍
머지는 나중에 제가 한꺼번에 할게요!

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