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

장재훈 - 자동차 게임 구현 #2

Open
wants to merge 30 commits into
base: Jaehooni
Choose a base branch
from

Conversation

Jaehooni
Copy link

@Jaehooni Jaehooni commented Mar 15, 2023

1주차 코드리뷰 스터디 결과물 제출 요청

반영 브랜치

Jaehooni:Jaehooni -> Dcom-KHU/Jaehooni

추가한 함수

Game directory

1. Car.Java

  • getPosition() : Car 객체의 position을 반환하는 함수
  • getName() : Car 객체의 name을 반환하는 함수
  • move() : random 값의 결과에 따라 Car 객체의 position을 움직이는 함수

2. Game.Java

  • make() : inputCarNames()의 결과를 기반으로 ArrayList<Car> 객체를 반환하는 함수
  • run() : inputRound()의 값만큼 CarList에서 move()와 printProcedure()를 실행하는 함수
  • start() : run()과 printResult(...)를 묶은 함수

Utility directory

3. InputCheck.Java

  • inputCarNames() : IterateCars(..) 함수가 오류를 throw하지 않으면 반환 아니면 다시 입력을 받게 하는 함수
  • checkName(String) : 입력으로 받은 자동차 이름이 양식에 맞지 않으면 오류를 throw하는 함수
  • inputRound() : checkRound(...) 함수가 오류를 throw하지 않으면 반환 아니면 다시 입력을 받게 하는 함수
  • checkRound(String input) : 입력으로 받은 라운드 양식이 맞지 않으면 오류를 throw하는 함수

4. Output.java

  • printProcedure() : ca rList들의 현재 position을 양식에 맞춰 출력하는 함수
  • printResult() : Car List 내의 Car 객체 간의 position을 비교하여 결과를 출력하는 함수
  • findMaxValue() : Car List 내에서 가장 높은 position을 반환하는 함수

5. Sort.java

  • sortByPosition() : car List들의 position 기반으로 sort를 진행하는 함수

Jaehooni added 14 commits March 14, 2023 23:28
Add new function to class Car
it makes self position increase by 1
when random number is bigger than 4
execute Car.move() method as many as input value
make Car object based on user input
Arraylist -> ArrayList
Added new methods to Car.java:
- getPosition()
- printProgress()
- printResult()
Added new methods to InputCheck.java:
- inputCarNames()
- iterateCars(ArrayList<String> names)
- inputRound()
- match(String input)
refactored methods:
- game(ArrayList<Car> cars)
- make()
@Jaehooni Jaehooni changed the title Jaehooni 장재훈 Mar 17, 2023
Comment on lines 39 to 40
ArrayList<String> names = InputCheck.inputCarNames();
ArrayList<Car> cars = new ArrayList<>();
Copy link
Collaborator

Choose a reason for hiding this comment

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

JAVA 에서는 List와 같은 Interface 타입을 변수의 타입으로 사용하길 권장하고 있습니다. 한번 찾아보시면 좋을것 같아요!

Copy link
Author

Choose a reason for hiding this comment

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

감사합니다! 아직 자바 코드 작성 경험이 적어 생기는 문제점인 것 같습니다 코드 수정 과정에서 수정하겠습니다 :)

@rohsik2
Copy link
Collaborator

rohsik2 commented Mar 18, 2023

커밋 메세지에도 많은 정보를 주시려 하시고, pull request도 상세히 작성하려 노력하신게 보이는것 같습니다. :) 조그마한 리뷰 남겨드렸습니다.

public Car(String name) {
this.name = name;
}
cars.sort((x, y) -> y.getPosition() - x.getPosition());
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 10 to 11
public InputCheck() {
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

이 메소드는 불필요해 보입니다. 객체 생성을 방지하고 싶으시면 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 13 to 32
public static ArrayList<String> inputCarNames() {
while (true) {
System.out.println("경주할 자동차 이름을 입력하세요.(이름은 쉽표(,) 기준으로 구분)");
ArrayList<String> names = new ArrayList<>(Arrays.asList(Console.readLine().split(",")));
try {
iterateCars(names);
return names;
} catch (IllegalArgumentException e) {
System.out.println("[ERROR] 이름은 5자 이하만 가능하다.");
}
}
}

public static void iterateCars(ArrayList<String> names) {
for (String name : names) {
if (name.length() > 5) {
throw new IllegalArgumentException();
}
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Exception을 던지실때 Error Message도 넣어서 던지실 수가 있어요! 에러 메세지를 넣어서 던지는건 어떻게 생각하시나요?

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 8
List<Car> cars = Game.make();
Game.start(cars);
Copy link

Choose a reason for hiding this comment

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

이 코드를 보면 cars 객체는 Game 클래스에서 가져와 Game 클래스의 메소드로 넣고 있어 불필요하게 변수 공간을 할당하고 저장하는 것처럼 보입니다. 혹시 이렇게 구현하신 이유가 있나요?

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 8 to 29
public static List<Car> make() {
List<String> names = InputCheck.inputCarNames();
List<Car> cars = new ArrayList<>();
for (String name : names) {
cars.add(new Car(name));
}
return cars;
}

private static void run(List<Car> cars) {
int num = InputCheck.inputRound();
for (int i = 0; i < num; i++) {
cars.forEach(Car::move);
Output.printProcedure(cars);
System.out.println();
}
}

public static void start(List<Car> cars) {
run(cars);
Output.printResult(cars);
}
Copy link

Choose a reason for hiding this comment

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

Application과 연계해서 살펴보면 Game을 통해 List 객체를 생성하고 Game 함수들은 모두 List 객체를 사용하는 것으로 보입니다. Game 클래스에 List 타입의 field를 하나 선언하고 다른 함수들은 field를 통해 접근하는 방법은 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

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

좋은 의견인 것 같습니다! Game Class에 멤버 변수를 하나 만들어 두어 접근하는 방법으로 바꾸었습니다!

- private List<Car> carList
- cars -> carList
Copy link

@Untaini Untaini left a comment

Choose a reason for hiding this comment

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

과거 코드에 비해 최대한 기능 별로 클래스를 분리하려는 모습이 인상 깊었습니다.

LGTM XD

public static void printProcedure(List<Car> carList) {
System.out.println("실행 결과");
for (Car car : carList) {
String distance = new String(new char[car.getPosition()]).replace("\0", "-");
Copy link

Choose a reason for hiding this comment

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

char 배열과 replace를 이용해서 거리를 나타내는 문자열을 만드는 점이 인상 깊었었습니다.

Copy link
Author

Choose a reason for hiding this comment

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

감사합니다 :)

@Jaehooni Jaehooni changed the title 장재훈 장재훈 - 자동차 게임 구현 Mar 22, 2023
Copy link

@sschan99 sschan99 left a comment

Choose a reason for hiding this comment

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

간결하고 좋은 코드인 것 같습니다!
LGTM :)

private void run() {
int num = InputCheck.inputRound();
for (int i = 0; i < num; i++) {
this.carList.forEach(Car::move);

Choose a reason for hiding this comment

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

불필요한 반복문 사용을 줄이기 위해 forEach 메서드를 사용한 점이 좋은 것 같습니다. 저도 활용해봐야겠네요!

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 10
private List<Car> carList = new ArrayList<>();

public void make() {
List<String> names = InputCheck.inputCarNames();

Choose a reason for hiding this comment

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

변수명에 자료형이 들어가지 않는 것이 더 좋다고 알고 있습니다! names처럼 복수형으로 표현하는 것은 어떨까요?
참고 : https://tecoble.techcourse.co.kr/post/2020-04-24-variable_naming/

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 18 to 25
int maxValue = findMaxValue(carList);

for (Car car : carList) {
if (car.getPosition() < maxValue) {
break;
}
winner.add(car.getName());
}

Choose a reason for hiding this comment

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

findMaxValue 메서드가 정렬과 최댓값 반환 두 동작을 수행하고 있는데, 메서드 이름은 최댓값 반환에 대해서만 나타나 있는 것 같습니다.
메서드의 구현을 읽지 않고도 어떤 로직으로 동작하는지 이해할 수 있게 바뀌면 좋을 것 같습니다.

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