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

유승근 - 자동차 경주 게임 #3

Open
wants to merge 16 commits into
base: yuseunggeun
Choose a base branch
from

Conversation

yuseunggeun
Copy link

@yuseunggeun yuseunggeun commented Mar 16, 2023

스프링 스터디 1주차

  • Car Class는 자동차의 이름, 위치를 가지며 난수를 생성하고 위치를 업데이트함
  • Game Class는 Car 객체를 배열로 관리하며 입력, 업데이트, 결과 출력 등 대부분의 기능을 담당
  • 진행상황 표시를 위해 문자를 반복한 문자열을 만드는 메소드 repeatString을 따로 정의

코드 설명

Application.java

Application Class

  • main(String[] args) = Game 객체를 만들고 initiateGame()을 호출

Game Class

  • currentMove = 현재 게임의 진행 회수
  • maxMove = 입력받는 게임의 목표 회수
  • cars = 게임에 사용되는 Car 객체들을 저장하는 배열
  • initiateGame() = 필요한 입력들을 받아 자동차들과 목표 회수를 설정하는 함수
  • makeCars(Striing[] carNames) = 입력받은 자동차 이름들로 Car 객체를 만들고 저장
  • inputCarNames() = 자동차 이름을 입력받는 함수
  • checkValidNames(String[] cars) = 유효한 이름이 아니면 예외를 발생시키는 함수
  • inputMaxMoves(String userInput) = 게임의 목표 회수를 입력받는 함수
  • checkValidMoves(String userInput) = 입력한 회수가 유효하지 않으면 예외를 발생시키는 함수
  • findMaxPosition() = 최대 전진 회수를 찾는 함수
  • playGame() = 반복문을 돌며 maxMove만큼 진행상황을 출력하는 함수
  • printProgress() = 자동차의 position을 업데이트하고 진행상황을 출력하는 함수
  • findWinner() = 최대 전진 회수만큼 전진한 자동차 리스트를 반환하는 함수
  • printWinners() = 최종 우승자를 출력하는 함수

Car.java

Car Class

  • name = 자동차의 이름
  • position = 자동차의 위치
  • getName() = 이름을 반환
  • printCar() = 자동차의 진행상황을 출력
  • updatePosition() = 전진조건에 해당하면 position을 1증가시킴
  • canAdvance() = 난수를 생성하고 그 난수가 4이상인지 여부를 반환
  • getPosition() = position을 반환
  • getPositionString() = 진행상황을 "-"으로 표현한 것을 반환
  • repeatString(int count, String str) = 문자를 반복한 String을 반환

코드 리뷰

  • 파일/클래스/메소드 분리의 개선방향이 알고 싶습니다
  • 함수 네이밍이 적절한지 알고 싶습니다

코드 리뷰후 개선점

  • 기초적인 Class 분리
  • 적절한 함수/변수 네이밍
  • 적절한 접근제어자 설정(public 남용 X)
  • 객체선언시 최상위 인터페이스로 선언 권장

@yuseunggeun yuseunggeun changed the title Yuseunggeun 유승근 - 자동차 경주 게임 Mar 17, 2023
Copy link
Collaborator

@rohsik2 rohsik2 left a comment

Choose a reason for hiding this comment

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

LGTM! :)

Comment on lines 14 to 24
class Game {
private int currentMove = 0;
private int maxMove;
private Car[] cars;
private ArrayList<String> winners;
Game() {}
public void initiateGame(){

makeCars(inputCarNames());
maxMove = inputMaxMoves();
playGame();
Copy link
Collaborator

Choose a reason for hiding this comment

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

하나의 파일에는 하나의 public Class만 존재하거나, Sub class만 두는것이 일반적입니다. 혹시 Application안에서 다른 sub class 형태로 Game을 구현하신 이유가 있으신가요?

Copy link
Author

Choose a reason for hiding this comment

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

아직 java가 익숙치 않아서 그랬던 것 같습니다. 확실히 분리하는 편이 더 적절한 것 같아요!

public String[] inputCarNames() {

System.out.println("경주할 자동차 이름을 입력하세요.(이름은 쉼표(,) 기준으로 구분)");
String userInput = readLine();
Copy link
Collaborator

Choose a reason for hiding this comment

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

static import 보다는 Console 객체를 import 하는 것이 조금 더 좋아보입니다. readLine이 Game안에 있는 메소드인지 아닌지 알기 조금 어려운것 같습니다.

Copy link
Author

Choose a reason for hiding this comment

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

알겠습니다. 난수 생성부분도 비슷하게 고치는게 괜찮을 것 같네요!

checkValidNames(cars);
} catch(IllegalArgumentException e) {
System.out.println(e.getMessage());
return inputCarNames();
Copy link
Collaborator

Choose a reason for hiding this comment

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

재귀를 이용한 구현도 재미있는것 같습니다!

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 40 to 46
public static String repeatString(int count, String str) {
StringBuilder result = new StringBuilder();
for(int cnt = 0; cnt < count; cnt++){
result.append(str);
}
return result.toString();
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

repeat String이 자동차가 가지고 있어야 하는 함수인지는 잘 모르겠습니다. Utility class로 분리하거나, private으로 숨겨보는건 어떠신가요?

Copy link
Author

Choose a reason for hiding this comment

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

Car 객체에 국한되지 않고 범용적으로 사용하려고 구현했던거라 분리하는 쪽이 더 적절한 것 같네요! 작성하면서도 Car와 아무 상관없지않나?하는 생각이 들었었습니다 :)

Comment on lines 27 to 30
public boolean canAdvance() {

return pickNumberInRange(0, 9) >= 4;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

해당 함수도 외부에 공개될 필요가 없다면 private을 고민해 보시는것도 좋을것 같습니다.

Suggested change
public boolean canAdvance() {
return pickNumberInRange(0, 9) >= 4;
}
public boolean canAdvance() {
return pickNumberInRange(0, 9) >= 4;
}

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

@choi5798 choi5798 left a comment

Choose a reason for hiding this comment

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

아직 Java 에 익숙치 않으신 것 같은데 그래도 깔끔하게 잘 짜신 것 같습니다. LGTM!

private int currentMove = 0;
private int maxMove;
private Car[] cars;
private ArrayList<String> winners;

Choose a reason for hiding this comment

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

객체들을 선언할 경우에는 보통 인터페이스로 구현되어 있는 가장 최상위 부모를 기준으로 정의해주는 것이 좋아요. SOLID 원칙 중 하나인데 이 번에 참고하시면 좋을 것 같아요

Copy link
Author

Choose a reason for hiding this comment

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

네 참고하겠습니다!

private int maxMove;
private Car[] cars;
private ArrayList<String> winners;
Game() {}

Choose a reason for hiding this comment

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

보통 Java에서는 class의 생성자를 만들지 않으면 default 생성자가 기본적으로 존재하는데 따로 비어있는 생성자를 만드신 이유가 있을까요?

Copy link
Author

Choose a reason for hiding this comment

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

변수들을 초기화하려고 만들어 두었다가 깜빡한것 같습니다. 지우는 편이 깔끔하겠네요!

playGame();
}

public void makeCars(String[] carNames) {

Choose a reason for hiding this comment

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

같은 class 안에서만 쓰이는 함수라면 접근 범위를 private으로 하는 걸 고민해 보시는 것도 좋을 것 같습니다

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 44 to 45
return inputCarNames();
}

Choose a reason for hiding this comment

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

재귀로 다시 입력을 받는 부분이 신기한 것 같습니다

Copy link
Author

Choose a reason for hiding this comment

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

감사합니다 :)

}
}

public int inputMaxMoves() {

Choose a reason for hiding this comment

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

숫자를 입력받아 return까지 하는 함수라면 입력부분을 따로 빼낸 뒤 함수 이름을 input으로 시작하는 것 보다는 get으로 시작하는 것은 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

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

input부분을 분리하려고 생각했는데 메소드 이름도 적절히 바꾸면 더 좋을 것 같네요. 좋은 의견 감사합니다!

return this.name;
}

public void printCar() {

Choose a reason for hiding this comment

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

자동차의 어떤 성분을 출력하는지 함수 이름에 명시되어 있으면 좋을 것 같습니다!

Copy link
Author

Choose a reason for hiding this comment

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

네 알겠습니다!

position++;
}
}
public boolean canAdvance() {

Choose a reason for hiding this comment

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

전진 가능한지 조건만 딱 깔끔하게 반환하는게 보기 좋은 것 같습니다

Copy link
Author

Choose a reason for hiding this comment

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

감사합니다 :)

}

public static String repeatString(int count, String str) {
StringBuilder result = new StringBuilder();

Choose a reason for hiding this comment

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

여기서 StringBuilder 의 변수이름을 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 +25 to +28
public String getPositionString() {

return Utility.repeatString(position, "-");
}
Copy link

Choose a reason for hiding this comment

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

Utility.repeatString을 사용해서 출력하는 방법 좋은 것 같습니다!

Copy link
Author

Choose a reason for hiding this comment

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

감사합니다 :)


int maxPosition = 0;
for (Car car : cars) {
maxPosition = Math.max(maxPosition, car.getPosition());
Copy link

Choose a reason for hiding this comment

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

if문으로 maxPosition 값을 갱신하는 것이 아닌, Math.max를 사용해 maxPosition을 갱신하는 방법 좋은 것 같습니다!

Copy link
Author

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.

4 participants