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

손현수) 자동차 경주 시뮬레이션 기능 추가 #1

Open
wants to merge 27 commits into
base: Untaini
Choose a base branch
from

Conversation

Untaini
Copy link

@Untaini Untaini commented Mar 15, 2023

반영 브랜치

Untaini:Untaini -> Dcom-KHU:Untaini

추가된 기능

  • 자동차 경주를 하고 우승자들을 출력합니다.
  • 경주에 필요한 자동차 이름과 라운드 횟수는 사용자로부터 입력을 받습니다.
  • 잘못된 자동차 이름과 라운드 횟수는 다시 입력 받습니다.

자동차 이름의 조건

  • 자동차의 이름은 5 자리 이하의 문자열이어야 한다.
  • 자동차의 이름은 공백일 수 없다.

라운드 횟수의 조건

  • 라운드 횟수는 자연수이어야 한다.

클래스 목록

racingcar.game package

  • Game : 게임의 전반적인 운영을 담당합니다. (라운드 관리부터 우승자 출력까지)
  • Round : 한 라운드 안에서 할 행동을 담당합니다.
  • Car : 경주에 사용되는 자동차를 담당합니다.

racingcar.tool package

  • InputUtils : 사용자가 유효한 입력을 할 때까지 요청하고, 입력 값을 적절한 객체로 변환하는 것을 담당합니다.
  • PrintManager : 출력할 문자열 관리와 출력을 담당합니다.
  • StringConvertor : 문자열을 적절한 객체로 변환하는 것을 담당합니다.
  • StringValidator : 문자열의 유효성 검사를 담당합니다.

racingcar package

  • Application : 게임을 생성하고 시작하는 것을 담당합니다.

리뷰 받고 싶은 점

  • 코드 상에서 예외 처리가 되지 않은 조건이 있는지
  • 어떠한 함수들을 다시 클래스로 묶을 수 있을지
  • 클래스 이름과 함수 이름이 적절한지
  • 각 클래스에서 사용한 설계 방식이 실제로 적절한지

Untaini added 13 commits March 15, 2023 02:03
create docs/README.md for implementation function docs

new implementation function docs:
  - input car names and check its available
  - input round count and check it available
  - repeat round count that user input
  - move all cars
  - print car status
  - find winners
  - print winners
Add new function:
 - get car names input from user
 - print error message if illegal input then get input again
 - parse input by car names
 - convert car names into Car object list

Illegal input condition:
 - Input's last character is ','
 - Car name's length is 0 or over 5
Functions to be changed return type (ArrayList<Car> -> List<Car>)
 - getCarListFromInput
 - convertCarNamesIntoCarList

These don't have to be ArrayList type
Add InputUtils class using singleton pattern

Move from Application class to InputUtils class
 - checkCarNameAvailable
 - checkCarNameInputAvailable
 - convertCarNamesIntoCarList
 - createCar
 - getCarListFromInput
 - printCarNameInputDescription

for to reduce function count from Application class
Add new function:
 - get total round input from user
 - parse input by int value
 - print error message if illegal input then get input again

Illegal input condition:
 - Input can't parse integer
 - Total round value is not a natural number
Add move function:
 - pick one value in range [0, 9]
 - move +1 position if value >= 4

Add toString function
 - return car name and moving position string

Add getPosition function
 - get position value of car instance
Add getName function
 - get name value of car object
Add new function
 - repeat rounds as many times as user input
 - each round, all cars move and prints out their own status
change incorrect function name in Car class
 - public String getnam() -> public String getName()
Add new function:
 - find car objects that have the biggest position value
 - print selected car objects' name
Change error tag
 - [Error] -> [ERROR]
Change import and function call
 - import static pickNumberInRange -> import Randoms
 - pickNumberInRange -> Randoms.pickNumberInRange
Change newline format
 - CRLF -> LF

Change function declare order
(InputUtils class)
rohsik2 added a commit to rohsik2/2023-spring-codeReview-study that referenced this pull request Mar 17, 2023
printManager 가 모든 print method들을 물고 있도록 만들어, 출력과 관련한 책임을 지도록 만듦

Related to : Dcom-KHU#1 Dcom-KHU#5
@Untaini Untaini changed the title Untaini 손현수자동차 경주 Mar 17, 2023
@Untaini Untaini changed the title 손현수자동차 경주 손현수) 자동차 경주 시뮬레이션 기능 추가 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 16 to 23

private static void playAllRounds(int totalRound, List<Car> carList) {
System.out.println("\n실행 결과");
while (--totalRound >= 0) {
playRound(carList);
}
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

대부분의 Application method들이 carList를 파라미터로 받는 static 함수들입니다. 게임을 나타내는 클래스를 하나 작성하시고, 파라미터가 아니라 fiel로 carList를 작성해 보시는건 어떠신가요?

Comment on lines 36 to 42
private String getMoveString() {
List<String> movePositionList = Arrays.stream(new String[this.position])
.map(s -> "-")
.collect(Collectors.toList());

return String.join("", movePositionList);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

stream을 잘쓰시네요! join method를 활용한 "-"만들기 좋은것 같습니다!

Comment on lines 9 to 20
public class InputUtils {
private static InputUtils inputUtils;

private InputUtils() {}

public static InputUtils getInstance() {
if (inputUtils == null) {
inputUtils = new InputUtils();
}
return inputUtils;
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

유틸리티 클래스는 싱글턴 패턴으로 구현하신것 같습니다. 싱글턴 패턴을 쓰신 이유를 혹시 알 수 있을까요? Static 클래스로 구현하지 않은 이유가 궁금합니다!

Comment on lines 52 to 56
private Car createCar(String carName) throws IllegalArgumentException {
checkCarNameAvailable(carName);

return new Car(carName);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Car 객체를 생성하는 코드가 Utility Class에 있는데, Factory Method는 주로 해당 클래스 안에서 작성하거나, Factory Class를 만드는 경우가 많은것 같습니다. utility class 안에서 작성하신 이유를 혹시 알 수 있을까요?

Comment on lines 87 to 93
private Integer convertRoundInputIntoRoundInteger(String roundInput) throws IllegalArgumentException {
try {
return Integer.parseInt(roundInput);
} catch (NumberFormatException exception) {
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.

public
class NumberFormatException extends IllegalArgumentException {
    static final long serialVersionUID = -2848938806368998894L;

    /**
     * Constructs a <code>NumberFormatException</code> with no detail message.
     */
    public NumberFormatException () {
        super();
    }

사실 NumberFormatException은 IllegalArgumentException의 Child Class입니다. 해당 메소드는 불필요하다고 보여집니다.

Copy link
Author

Choose a reason for hiding this comment

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

parseInt에서 발생하는 exception message를 변경하고 싶은데 다른 방법이 있을까요?

Copy link
Collaborator

Choose a reason for hiding this comment

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

아항 parseInt에서 나오는 메세지를 바로 변경...이 안되니 원하는 메세지를 넣어서 에러 핸들링을 하시려면, 지금 방식이 맞는것 같네요!

Untaini added 11 commits March 19, 2023 00:28
add StringValidator class for validating string object

add new function:
 - canCarName
   (similar to InputUtils.checkCarNameAvailable)
add PrintManager class for printing

collect print func from other classes
 - printCarNameInputDescription (in InputUtils)
 - printTotalRoundInputDescription (in InputUtils)
 - printErrorMessage (in InputUtils)

 - printAllCarsStatus (in Application)
 - printWinnersName (in Application)
 - printGameResultHead (in Application)
add StringConvertor class for converting string object into other object

add new function:
 - convertIntoCarList
  (similar to InputUtils.convertCarNamesIntoCarList)

 - convertIntoNaturalNumber
  (similar to InputUtils.convertRoundInputIntoRoundInteger)
deal with round activities

add new function:
 - getCurrentRound
 - play
 - moveAllCars
deal with the whole game

add new function:
 - start
 - findWinners
add createCar static function in Car class
 - return new Car object that have string arg
 - check the arg can car name
remove unused function in InputUtils

removed function:
 - printCarNameInputDescription
 - printTotalRoundInputDescription

 - convertCarNamesIntoCarList
 - convertRoundInputIntoRoundInteger

 - checkCarNameInputAvailable
 - checkCarNameAvailable
 - checkTotalRoundAvailable

 - createCar
use Game instance instead of static function call
remove unused function in Application

removed function:
 - playAllRounds
 - findWinners
 - getWinnersString

 - playRound
 - moveAppCars

 - printAllCarsStatus
 - printWinners
separate from printWinnersName function
add tool, game package because of too many class in racingcar package

tool package
 - InputUtils
 - PrintManager
 - StringConvertor
 - StringValidator

game package
 - Game
 - Round
 - Car
change function name more proper

change return type:
 - boolean -> void
return Arrays.stream(carNames.split(","))
.map(String::trim)
.map(Car::createCar)
.collect(Collectors.toList());

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.

위의 코드를 말로 풀면 다음과 같습니다.

  1. carNames 문자열을 ','으로 나눕니다.
  2. 나눠진 문자열의 양쪽 공백을 삭제합니다.
  3. 각 문자열을 이름으로 가지는 Car 객체를 생성합니다.
  4. 리스트로 만들어 반환합니다.

코드를 해석하는데 조금 도움이 됐으면 좋겠네요 :)

Choose a reason for hiding this comment

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

감사합니다 :)

}

private List<Car> findWinners() {
if (carList.size() == 0) {

Choose a reason for hiding this comment

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

예외 처리에서 CarList의 size()가 0인 경우는 이미 배제하셨던데 혹시 findWinners() 함수에서 0인 경우를 따로 처리하신 것이 어떤 의미가 있는건지 알고 싶습니다..!!

Copy link
Author

Choose a reason for hiding this comment

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

그 이유는 Game 객체를 생성할 때 받은 carList가 빈 리스트인지는 확인하지 않았기 때문입니다.

적어도 원소 하나는 있어야 그 아래 코드에서 오류가 발생하지 않기 때문에
빈 리스트인 경우에는 if문을 통해 빈 리스트를 반환해 주었습니다.
사실 carList를 반환해주어도 되지만, 비어있다는 것을 명확하게 표현하기 위해 Collections.emptyList()를 사용했습니다.

Copy link

@Jaehooni Jaehooni left a comment

Choose a reason for hiding this comment

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

class들을 game, tool 의 두 디렉토리로 나눈 이유가 있을까요??

}
}

private String getMoveString() {

Choose a reason for hiding this comment

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

함수형 프로그래밍을 통해서 깔끔하게 표현하신 것이 인상 깊었습니다.
그리고 이 함수를 PrintManager가 아닌 Car에 구현하신 이유가 있을까요?

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의 정보를 문자열로 표현하는 방법을 toString에서 정의하고 있으니, 거리를 문자열로 표현한 것도 Car에서 구현하면 되겠다고 생각했습니다.

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

public static void printTotalRoundInputDescription() {

Choose a reason for hiding this comment

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

printCarNameInputDescription()이나 이 함수 같이 안내 문구만 각각 한 줄씩 따로 함수로 생성한 이유가 있을까요?

Copy link
Author

Choose a reason for hiding this comment

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

PrintManager는 출력과 관련된 함수들을 정의해둔 클래스입니다. 그래서 출력되는 문자열을 바꾸고 싶을 때는 PrintManager만 수정하면 되게끔 만들었습니다.

Copy link

@Jaehooni Jaehooni left a comment

Choose a reason for hiding this comment

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

LGTM!

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.

전체적으로 SRP 원칙을 잘 지켜서 클래스들의 관계가 명확하고 좋아 보입니다!
LGTM :)

Comment on lines +10 to +16
public static void printCarNameInputDescription() {
System.out.println("경주할 자동차 이름을 입력하세요.(이름은 쉼표(,) 기준으로 구분)");
}

public static void printTotalRoundInputDescription() {
System.out.println("시도할 회수는 몇회인가요?");
}

Choose a reason for hiding this comment

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

SRP를 위해 클래스의 책임이 명확히 구분된 것 같아서 좋습니다! 저도 해봐야겠네요

Comment on lines 36 to 43
int maxPosition = carList.stream()
.map(Car::getPosition)
.max(Integer::compareTo)
.get();

return carList.stream()
.filter(car -> car.getPosition() == maxPosition)
.collect(Collectors.toList());

Choose a reason for hiding this comment

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

stream을 잘 활용한 좋은 예인 것 같습니다. :)


public class Game {

private final List<Car> carList;

Choose a reason for hiding this comment

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

변수명에 자료형이 들어가지 않는 것이 더 좋다고 알고 있습니다! 복수형으로 표현하는 것은 어떨까요?
참고 : 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.

덕분에 변수 네이밍에 대해서 검색을 좀 더 할 수 있었습니다!
List 타입에 대해서는 List라는 이름을 사용하는 것을 허용한다는 글도 있었습니다.
하지만 차후 List 타입이 아닌 Set이나 다른 타입을 사용할 가능성이 있는 변수에 대해서는 복수형을 사용하는 것이 더 낫다고 하네요 :)
그래서 이 부분은 복수형으로 표현하는 것이 유지보수를 위해서는 더 나은 선택으로 보입니다. 감사합니다!

Untaini added 3 commits March 25, 2023 13:51
target func:
 getWinnersString -> joinCarsName
change var name:
 carList -> cars (in all classes)
 movePositionList -> movingPositions (in Car)

change func name:
 getMoveString -> getMovingPositionLine (in Car)
 getWinnersString -> getWinnersNameLine (in PrintManager)
 convertIntoCarList -> convertIntoCars (in StringConvertor)
target classes:
 - InputUtils
 - ListConvertor
 - PrintManager
 - StringConvertor
 - StringValidator
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