[최보현] 자동차 경주 미션 제출 #133
[최보현] 자동차 경주 미션 제출 #133
Conversation
There was a problem hiding this comment.
보현님 미션 내용 잘 구현해주셨네요!😎
몇가지 코멘트 남겨두었으며, 아래는 질문주신 내용에 대한 제 생각입니다!
1. 랜덤값 테스트 관련
기능을 테스트하는것은 매우 중요한 부분이어서, 이를 위한 추상화를 하는것은 큰 문제가 없다고 생각해요! 지금은 랜덤한 숫자를 생성하고 특정 기준을 넘으면 전진시키는 역할을 모두 Car에서 수행하고 있는데, 랜덤한 숫자를 밖에서 만들어서 movePosition의 파라미터로 넘겨주는 방법도 있겠죠. 역할을 누구에게 부여할지 어떤 방식으로 구현할지는 매우 다양하기 때문에 여러가지 방법들을 시도해보시면서 장단점들을 느껴보면 좋을 것 같아요.
2. controller 와 app은 가볍게?
오프라인에서 말씀 드렸던것 처럼 controller는 콘솔, API 등 제공 방식에 따라 구현이 달라지는 부분이어서 최대한 비즈니스 로직을 넣지 않는게 좋다고 생각해요. Allocation 클래스의 경우에는 어플리케이션을 실행하는 역할을 하는 클래스여서 실행 외의 다른 로직은 최대한 넣지 않는게 좋아보여요.
3. mvc패턴 리팩토링
패키지 분리의 경우 잘 해주셨어요! 앞으로의 미션에서는 구조가 조금 더 복잡해질 수 있으니, 미리 MVC 패턴에 대해 학습해보시고 다음 미션 진행해보시면 좋을 것 같긴 합니다👍
|
|
||
| public class RacingGameController { | ||
|
|
||
| public static final String DELIMITER = ","; |
There was a problem hiding this comment.
DELIMITER가 어떤 목적의 구분자인지 표현할 수 있게 상수명을 변경해보면 어떨까요?
추가적으로 input string에 대한 parsing 역할을 contoller에게 부여한 이유가 궁금해요!
There was a problem hiding this comment.
문자열에서 구분자 , 를 기준으로 차 이름을 나누므로 CAR_NAME_DELIMITER 로 수정했습니다.
CarRaceGame 생성자에 List가 파라미터인데 리스트를 만들고 넣어줘야 한다고 생각해서 controller에 로직이 들어갔습니다. parsing 역할을 따로 클래스로 분리하여 도메인 패키지에 포함시키고 controller에서 그 클래스의 메서드를 이용하는 것으로 수정했습니다 !
|
|
||
| public static final String DELIMITER = ","; | ||
|
|
||
| public void startRacingGame(){ |
There was a problem hiding this comment.
startRacingGame, printGameRounds , printWinners 메서드로 분리하여 수정했습니다 !
| int round = inputView.enterRoundNumber(); | ||
|
|
||
| carRaceGame.validateRoundNumber(round); | ||
|
|
||
| outputView.printGameResultTitle(); | ||
|
|
||
| for (int i = 0; i < round; i++) { |
There was a problem hiding this comment.
Controller에서 round를 사용하고 CarRaceGame에서는 round를 검증하고 있는데요.
이렇게 구조를 잡으신 이유가 궁금해요!
There was a problem hiding this comment.
round 변수 사용 이전에 검증하면 된다고 생각했습니다.
round 역시 Car 객체의 name과 마찬가지로 검증 후 할당하게끔 하고 싶은데 어떤 식으로 해야할지 잘 모르겠습니다 🥲
src/main/java/domain/Car.java
Outdated
|
|
||
| public class Car { | ||
|
|
||
| private String name; |
There was a problem hiding this comment.
불변인 변수는 final로 명시해주면 좋아요! (다른 클래스에도 함께 반영 부탁드려요)
왜 좋을지 생각해보시고 답변 달아주시면 좋을것같아요:)
There was a problem hiding this comment.
final을 명시하면 재할당 할 수 없기 때문에 안전하고, 불변이라는 것을 명확하게 전달하기 때문에 가독성 면에서도 좋을 것 같습니다 !
| public interface NumberGenerator { | ||
| int generate(); | ||
| } |
There was a problem hiding this comment.
랜덤한 로직을 통제하기 위해 인터페이스를 통해 추상화한 부분 좋습니다!
이렇게 메서드 하나만을 가진 인터페이스는 "함수형 인터페이스"로 만들어볼 수 있는데요. 이에대해 학습해보시면 좋을 것 같아요!
There was a problem hiding this comment.
함수형 인터페이스로 만들면 따로 클래스를 만들 필요 없이 람다식으로 간결하게 활용할 수 있군요! 🤔
다만 제 코드에서는 활용하기 힘들 것 같은데 맞나요...?
|
|
||
| public class RandomNumberGenerator implements NumberGenerator { | ||
|
|
||
| private static final Random RANDOM = new Random(); |
There was a problem hiding this comment.
Random 객체를 상수로 잘 만들어주셌네요👍
한번만 생성하고 재활용해도 되는 객체의 경우 상수로 만들어 활용하면 불필요한 객체생성 및 메모리를 줄일 수 있어서 좋습니다!
| private List<Car> createCars(NumberGenerator... generators) { | ||
| List<Car> cars = new ArrayList<>(); | ||
| for (int i = 0; i < generators.length; i++) { | ||
| cars.add(new Car("차" + (i + 1), generators[i])); | ||
| } | ||
| return cars; | ||
| } |
There was a problem hiding this comment.
사용되는 순서대로 바꿔보았는데 이런 수정 방향이 맞을까요?
| import static org.assertj.core.api.Assertions.assertThat; | ||
| import static org.assertj.core.api.Assertions.assertThatThrownBy; | ||
|
|
||
| public class CarTest { |
There was a problem hiding this comment.
@nested로 각 테스트를 논리적 개념에 따라 나누어보면 어떨까요?
src/test/java/domain/CarTest.java
Outdated
| @Test | ||
| void movePosition_랜덤숫자4미만일때_정지() { | ||
|
|
||
| // given | ||
| NumberGenerator generator = new FixedNumberGenerator(2); | ||
| Car car = new Car("차3", generator); | ||
|
|
||
| // when | ||
| car.movePosition(); | ||
|
|
||
| // then | ||
| assertThat(car.getPosition()).isEqualTo(0); | ||
| } |
There was a problem hiding this comment.
BDD 패턴을 명시해주신걸로 보이는데요. 사용해주신 이유가 궁금해요!
There was a problem hiding this comment.
이 구조로 작성하면 테스트할 때의 사고흐름을 잘 보여준다고 생각했습니다 !
안녕하세요 엘리님☺️
두 번째 과제를 진행하면서 궁금했던 점과 개선하고 싶은 부분입니다.
1. 랜덤값 테스트 관련
랜덤값을 테스트하기 위해 여러가지 시도를 해 보았습니다.
먼저, 기존에는
Car클래스의movePosition()메서드에서 [매개변수로RandomGenerator클래스를 받아 그 내부의getRandomNumber()메서드로 랜덤숫자를 받아오고, 그 숫자를 움직이는 기준과 비교해 위치를 이동시킨다] 라는 행위를 했습니다. 이를 테스트가 어려운 부분(랜덤값을 받아온다)과 쉬운 부분(숫자와 기준값을 비교해 위치를 이동시킨다)으로 분리해getRandomNumber(),movePosition()두가지 메서드로 나눴습니다.이렇게 되면 여전히 테스트가 어려운 부분에 대해서는 테스트를 진행하기가 힘들고,
getRandomNumber()메서드는 Car 객체와 관련이 없기 때문에 책임을 명확하게 하기 위하여 따로 분리하였습니다. 그리고 요구사항에 맞는 랜덤 숫자를 생성하는 기능과 테스트를 위해 고정된 숫자를 생성하는 기능을 위해NumberGenerator라는 인터페이스를 생성하고, 각각의 기능을 구현해 사용했습니다.그러면서 Car 객체의 생성자에
NumberGenerator가 포함되었는데, 이렇게 테스트를 위해 생성자에 인터페이스가 포함되는 것이 과연 효율적인 방법인가 라는 생각이 들어 이것에 대한 엘리님의 생각이 궁금합니다.2. controller 와 app은 가볍게?
제 코드 상에서는 controller에서 view와 model의 소스 외에 직접 입력받은 이름들을 Car리스트에 담아주는 로직이 존재하는데, 이런 로직도 model에서 처리하게끔 작성하는게 더 명확한 코드 작성법인지 궁금합니다. 또한 app의 main 메서드에서는 게임을 실행하는 역할만 해야한다고 생각해서 간결하게 작성하였습니다.
3. mvc패턴 리팩토링
mvc 패턴에 대한 명확한 이해없이 제 판단대로 나누어서 구현하였는데 , 이 부분은 코드 리뷰 진행 후 다시 공부하여 수정해보고 싶습니다.
이번에도 꽤나 헤매면서 구현하였는데, 설계를 선행하고 최대한 구조 변화없이 설계대로 진행할 수 있도록 노력해 보겠습니다.
감사합니다 !
1차 코드 리뷰 후 수정 사항
1. mvc 패턴에 맞게 수정하기
오프라인 미팅에서 해주신 말씀을 토대로 '로직과 데이터는 domain에 포함시키기' 를 지키려고 했습니다.
기존 controller에 있던 '문자열을 구분자 기준으로 파싱하는 역할'을 domain의
CarNameParser에서 처리하게끔 했습니다. domain은 그대로, 서비스하는 방법에 따라 view와 controller는 달라질 수 있다는 설명이 인상 깊었고 mvc 패턴에서 각각의 역할을 바로 이해할 수 있었습니다. 감사합니다.2. 변수에 값 할당하기 이전에 검증하기
제가 기존에 갖고 있던 생각은 '사용하기 이전에 검증하면 괜찮지 않을까?' 였습니다. 안전성과 무결성을 위해 반드시 할당 이전에 검증이 필요하다는 것을 공부하였고, 이에 맞게 코드를 수정하였습니다. 다만 round의 경우 검증 후 할당하기 위한 방법을 잘 모르겠는데, 이 부분은 더 생각해보고 수정해보도록 하겠습니다.
3. 가독성 좋은 코드
이번 미션의 요구사항이었던 Java Style Guide 원칙에 맞게 수정하도록 노력했습니다. 패키지 이름을 모두 소문자로 변경하였고, 와일드 카드 임포트된 부분을 수정했습니다. 이 외의 코드리뷰에서 언급된 가독성이 안 좋은 부분들 또한 모두 수정했습니다. 의도를 명확하게 전달하기 위해 가독성 좋게 코드를 작성하도록 노력하겠습니다.