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 송세연 #3

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

Conversation

amaran-th
Copy link

생각보다 기능 별로 매번 커밋을 날린다는 걸 준수하는 게 어렵네요...
다음 챌린지 땐 더 노력해보겠습니다!

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.

짧은 시간이었을텐데 구현하시느라 정말 수고하셨습니다!!!👍👍👍
클래스를 조금만 더 세밀하게 분리하고, 메서드가 하나의 역할만 하도록 수정해보는 것은 어떨까요?
테스트 코드도 한번 작성해보시면 좋을 것 같습니다😄

while(true){
BaseballTool tool=new BaseballTool();
tool.makeRamdomNumbers(); //랜덤한 숫자를 생성한다.

Copy link
Contributor

Choose a reason for hiding this comment

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

소개해드렸던 formatter를 사용해보시는게 좋을 것 같습니다! 포메터 적용을 하지 않으면, 수정사항이 띄어쓰기까지 반영되면서 나중에 확인할 때 불편하다는 단점이 있습니다.
https://withhamit.tistory.com/411


public class BaseballTool{
private Scanner sc=new Scanner(System.in);
private int[] solution=new int[3];
Copy link
Contributor

Choose a reason for hiding this comment

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

배열 대신 List를 사용해보시는 건 어떨까요? 이번 프로젝트에서는 문제 될 게 없지만, Effective java item 28을 보면, 배열보다는 리스트를 사용하라는 이야기가 나와있습니다. 책을 갖고 계시지 않을 것 같아 아래에 블로그 주소를 참고하셔서 한번 공부해보셔도 좋을 것 같아요.

https://velog.io/@new_wisdom/Effective-java-item-28.-%EB%B0%B0%EC%97%B4%EB%B3%B4%EB%8B%A4%EB%8A%94-%EB%A6%AC%EC%8A%A4%ED%8A%B8%EB%A5%BC-%EC%82%AC%EC%9A%A9%ED%95%98%EB%9D%BC

if(inputNums[i]==solution[i]) strike++;
else{
for(int j=0;j<this.solution.length;j++) {
if (inputNums[i] == solution[j]){
Copy link
Contributor

Choose a reason for hiding this comment

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

프로그래밍 요구사항을 보면, 다음과 같이 나와있습니다.

  • indent(인덴트, 들여쓰기) depth를 3이 넘지 않도록 구현한다. 2까지만 허용한다.
    • 예를 들어 while문 안에 if문이 있으면 들여쓰기는 2이다.
    • 힌트: indent(인덴트, 들여쓰기) depth를 줄이는 좋은 방법은 함수(또는 메소드)를 분리하면 된다.

클래스를 분리하고, 메서드를 역할에 따라 나눠보시다 보면 해결될 문제같습니다!

}

public void makeRamdomNumbers(){
Random random=new Random();
Copy link
Contributor

Choose a reason for hiding this comment

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

메서드를 호출할 때마다 Random 클래스를 생성할 필요가 없지 않을까요? Scanner 처럼 두는 것도 좋아 보입니다.

import java.util.Scanner;

public class BaseballTool{
private Scanner sc=new Scanner(System.in);
Copy link
Contributor

Choose a reason for hiding this comment

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

sc와 같은 필드들은 선언되고 값이 수정될 필요가 없으니, final로 선언해주는 것은 어떨까요?

}
}
public boolean checkStrike(){
if(this.strike==3) {
Copy link
Contributor

Choose a reason for hiding this comment

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

checkStrike 메서드는 아래와 같이 코드를 줄일 수 있을 것 같습니다!

return strike == 3;

for(int i=0;i<this.inputNums.length;i++){
if(inputNums[i]==solution[i]) strike++;
else{
for(int j=0;j<this.solution.length;j++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

여기에서는 enhanced for loop으로 코드를 작성해보는 것은 어떨까요?

for (int k : this.solution) {
  if (inputNums[i] == k) {
    ball++;
    break;
  }
}

코드가 좀 더 깔끔해질 것 같은데 어떠신가요?

tool.calcResult(); //게임 결과를 계산한다.
tool.printResult(); //게임 결과를 출력한다.
}
if(tool.getSignalCode()==2) break;
Copy link
Contributor

Choose a reason for hiding this comment

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

종료 조건이 이렇게 명확한 경우에 do-while 문으로 코드를 작성해 볼 수 있지 않을까요?

public static void main(String[] args) {
try{
while(true){
BaseballTool tool=new BaseballTool();
Copy link
Contributor

Choose a reason for hiding this comment

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

마찬가지로 while 문을 돌 때마다 BaseballTool을 새로 생성할 필요가 있을까요?

public void makeRamdomNumbers(){
Random random=new Random();
for(int i=0;i<3;i++) {
this.solution[i] = random.nextInt(9) + 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

this. 로 필드 값을 가져오지 않아도 되지 않나요? this.로 값을 가져오신 이유가 따로 있나요? 궁금합니다!

amaran-th added 5 commits September 26, 2022 21:19
- Random() 인스턴스를 멤버변수화
- 변경되지 않는 멤버변수에 final 키워드 추가
- do-while문으로 수정
- 메서드 간략화
}
}

public boolean checkStrike() {

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) {
BaseballGame game=new BaseballGame();
try {
while(game.tryGame() != 2);

Choose a reason for hiding this comment

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

tryGame이 int를 반환하는것과 그걸 2와 비교하는 것이 어떤 의미인지 파악하기 어려웠어요. enum을 만들면 좀 더 명확하게 표현할 수 있지 않을까요?

Copy link
Contributor

@shkisme shkisme Sep 29, 2022

Choose a reason for hiding this comment

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

저도 말씀드리고 싶었던 부분이였습니다! 아래처럼 enum을 생성해서 이것을 이용하면 가독성을 높일 수 있을 것 같습니다.

enum Signal{
    END(2);
    private int signal;

    Signal(int signal) {
      this.signal= signal;
    }

    public int getSignal() {
      return this.signal;
    }
}

int num;
if (inputString != null && inputString.matches("[1-9]+")) {
num = Integer.parseInt(inputString);
if (num < 1000 && num > 99) {

Choose a reason for hiding this comment

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

matches를 [1-9]{3}으로 검사하면 num에 대한 검사를 안해도 괜찮지 않을까요?

- 메서드 이름 명확하게 변경
- ENUM 사용하여 가독성 향상
- 정규식 수정하여 코드 단순화
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.

굿! 수고하셨습니다! 자바 챌린지 2로 넘어가셔도 좋을 것 같아요!
머지는 제가 나중에 한꺼번에 진행하겠습니다.

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.

3 participants