[자동차 경주] 육새라 미션 제출합니다. #1462
Open
[자동차 경주] 육새라 미션 제출합니다. #1462
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* InputValidator.java: 정규표현식을 잘못 사용한 부분을 고침 * InputValidatorTest.java: checkHasNumberOnly() 기능 테스트 추가 * ViewConstants.java: - 시도 횟수 입력값이 갖춰야 할 조건의 정규표현식을 상수로 선언 - 상수를 사용 목적에 따라 구분하여 선언 순서를 개선
* InputController.java: 입력값을 변환하는 메서드들을 구현 * InputValidator.java: 변경된 로직에 맞는 파라미터 자료형 적용 * Application.java: 분리된 역할에 따라 작동하도록 코드 수정
* Move.java: 이동에 관련된 책임을 모두 갖도록 isPossible() 추가 * NumberValidator.java: 불필요한 클래스라 삭제됨 * Application.java: 책임 재분배에 따라 작동하도록 코드 수정
* InputValidator.java: 이름을 2개 미만으로 입력하면 예외 발생하도록 수정 * InputController.java: 문자열 분리 후 이름 개수 검증하도록 순서 변경 * ViewConstants.java: 이름 개수 에러 메시지 내용에 맞게 상수 이름 변경 * README.md: 기능 설명하는 내용을 의도에 맞게 수정
* Car.java: 이름이 null이거나 공백 문자만 있어도 예외 발생하게 수정 * CarTest.java: validateLengthOf()의 테스트 추가 * RacingConstants.java: 이름의 최소 길이를 나타내는 상수 선언 추가 * ViewConstants.java: 이름 길이 에러 메시지에 최소 길이 기준 추가 * README.md: 기능 설명하는 내용을 의도에 맞게 수정
* 불필요한 클래스 삭제 - Judge.java, CurrentCar.java, NumberValidatorTest.java * 자동차의 상태를 변경하거나 조회하는 기능 추가 - Car.java, Racers.java * 책임 재분배에 따른 코드 수정 - Application.java, Racing.java, Move.java, OutputView.java * 상수 추가 및 선언 순서 개선 - ViewConstants.java
* 숫자 생성 기능과 이동 가능 여부 검증하는 기능을 합침 - Move.java, NumberGenerator.java => RacingRule.java * 책임 재분배에 따른 코드 수정 - Application.java, Racing.java, Racers.java * 불필요한 클래스 삭제 - NumberGeneratorTest.java
* Application.java: 앱을 시작하고 끝내는 역할만 하도록 수정 * AppConfig.java: 앱에 필요한 객체들을 구성 * RacingInitializer.java: 게임에 필요한 초기값 등록 * CarRacing.java: 게임을 시작하고 끝내는 메서드 추가
* config: - AppConfig.java - InputValidator.java - RacingInitializer.java - RacingConstants.java * controller: - InputController.java * service: - Car.java - CarRacing.java - Racers.java - RacingRule.java * view: - InputView.java - OutputView.java - ViewConstants.java
* CarRacing.java: 인스턴스 변수 선언 순서 및 생성자 파라미터 순서 변경 * AppConfig.java: 변경에 따른 코드 수정
SeongUk52
reviewed
Nov 4, 2024
| @@ -0,0 +1,14 @@ | |||
| package racingcar.model; | |||
|
|
|||
| public class RacingConstants { | |||
There was a problem hiding this comment.
해당 클래스는 모델 계층이 아니기 때문에 패키지를 바꾸시던지 클래스 명에 어떤 모델에 대한 상수인지 표시해주시면 좋다고 생각합니다.
Comment on lines
+13
to
+15
| public InputView inputView() { | ||
| return new InputView(); | ||
| } |
There was a problem hiding this comment.
호출할 때 마다 인스턴스를 새로 생성하는데 매번 새로운 인스턴스를 만드는게 좋은 설계인지 고민을 해보셨으면 좋겠습니다.
그리고 메서드 이름을 좀 더 명확하게 해주시면 좋을 것 같습니다.
|
|
||
| import java.util.List; | ||
|
|
||
| public class Racers { |
There was a problem hiding this comment.
현재 Racers라는 클래스가 가진 역할이 이름과 어떤 연관이 있는지 잘 모르겠습니다.
There was a problem hiding this comment.
'경주에 참여하는 자동차들'이라는 의미로 네이밍했는데 잘 전달되지 않았던 것 같네요! 다음에는 더 좋은 이름을 고민해볼게요 :)
|
|
||
| public void printWinners(List<String> winnerNames) { | ||
| String winners = String.join(NAME_DELIMITER + WHITE_SPACE, winnerNames); | ||
| System.out.printf("%s%s", WINNER_IS, winners); |
There was a problem hiding this comment.
중요한건 아니지만
Suggested change
| System.out.printf("%s%s", WINNER_IS, winners); | |
| System.out.printf(WINNER_IS + winners); |
와 같이 작성하는 방법도 있습니다.
Comment on lines
+29
to
+43
| public CarRacingService carRacingService() { | ||
| return new CarRacingService(); | ||
| } | ||
|
|
||
| public RacingRule racingRule() { | ||
| return carRacingService().setRacingRule(); | ||
| } | ||
|
|
||
| public Racers racers() { | ||
| return carRacingService().registerRacers(carRacingController().extractCarNames()); | ||
| } | ||
|
|
||
| public int totalRounds() { | ||
| return carRacingService().registerTotalRounds(carRacingController().convertToNumber()); | ||
| } |
There was a problem hiding this comment.
CarRacingService의 인스턴스가 매번 새로 생성되어 비효율적이라고 생각됩니다.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
중점적으로 시도한 것
효율적인 관리를 위한 패키지 분리 시도
특정 패턴을 따랐다고 하기에는 부족한 부분이 많지만 굳이 따지자면 MVC 패턴에 가까운 것 같습니다.
데이터베이스, 레포지토리 개념에 대한 이해가 부족해서 관련된 부분은 제외하고, 아래 5가지 패키지로 분리해 보았습니다.
일급 컬렉션 사용
위의 장점을 활용해보고 싶어서
Racers클래스를 일급 컬렉션으로 구현했습니다.List<Car> cars외 다른 필드는 존재하지 않음.Car객체들만 가능한 자료구조를 만듦.Racers내에서 처리.커밋 메시지 작성
1주차 피드백 공유 문서를 참고하여 커밋 메시지를 의미 있게 작성한다.
아쉬운 부분
AppConfig에서는 controller나 service 객체까지만 생성하고 나머지 비즈니스 로직은 service와 controller 객체가 처리해야 한다.따라서
AppConfig의 아래 기능들은CarRacingService의 역할에 해당하므로 책임 이동이 필요하다.racingRule(),racers(),totalRounds(),carRacing()run()RacingRule에서 랜덤값 생성 기능을 분리하고, 생성된 랜덤값 검증 기능만 담당하는 것이 좋겠다. 이유는 아래와 같다.Randoms가private함수 내에서 호출되어 기능 테스트가 불가능하다.하지만
Randoms를 내부에서 사용함으로써 이에 대한 테스트와 결과값 보장이 불가능하다.Car가 입출력과 관련된 로직을 모르는 것이 좋기 때문에getStatus()가 완성된 문자열이 아닌 데이터만 전달할 수 있도록 해야한다.Car.moveForwardIf()의 반환값은 테스트를 위해서만 사용되므로 값을 반환하지 않아도 된다.CarRacing클래스가 필요했을까?CarRacingService가Racers와RacingRule을 참조하여 처리할 수 있지 않았을까?