[2단계 - 자동차 경주] 이든(최승준) 미션 제출합니다. #815
Conversation
- RandomMove 전략, NoneMove 전략, Move 전략으로 분리 - 전략패턴 적용을 통해 테스트 만을 위해 캡슐화가 깨져있던 moveForward 메서드의 캡슐화
PgmJun
changed the title
| final var carRacing = new CarRacing(new InputView(), new OutputView()); | ||
|
|
||
| final var cars = carRacing.start(); | ||
| carRacing.announceWinners(cars); |
There was a problem hiding this comment.
밖으로 분리해주신 이유가 궁금해요?
start라는 기능이랑 우승자를 알리는 기능이 분리되어야 할 필요가 있다고 생각이 들었어요!
start는 말그대로 자동차 레이싱을 시작하는 기능이고, 레이싱 이후 우승자를 알리는 기능은 별도로 구현해두어야 하나의 메서드에 하나의 책임을 줄 수 있을 거라고 생각이 되었어요!
| return new TryCount(Integer.parseInt(input)); | ||
| } catch (ValidateException exception) { | ||
| outputView.printErrorMessage(exception); | ||
| return createTryCount(); |
There was a problem hiding this comment.
재귀로 발생하는 문제를 막으려면 이것 또한 자체적으로 최대 횟수를 줄 수 있는데요, 지금은 굳이 그럴 필요까진 없을 것 같아요~!
There was a problem hiding this comment.
재귀로 발생하는 문제를 막으려면 이것 또한 자체적으로 최대 횟수를 줄 수 있는데요, 지금은 굳이 그럴 필요까진 없을 것 같아요~!
자체적으로 최대 횟수를 주는 것도 좋은 방법이 되겠네요!ㅎㅎ 좋은 인사이트 얻어갑니다!
| import domain.accelerator.Accelerator; | ||
| import util.RandomNumberUtils; | ||
|
|
||
| public class RandomMoveAccelerator implements Accelerator { |
There was a problem hiding this comment.
이든에게 궁금한 점이 있어요ㅎㅎ
우선 전략패턴을 학습해보고 적용까지 해 주신 것은 좋네요👍
다만, 고민해봤으면 하는 부분이 있어요.
- 비즈니스 로직에서의 요구사항은 무엇일까?
우리의 요구사항은 난수만큼 각 자동차들이 이동하고, 그 결과를 유저에게 보여주는 것인데요, 난수 생성 이외에 다른 케이스에 대응하기 위해 전략패턴을 사용해야할까?라는 생각이 들어요.
추가로 테스트 하기 좋은 코드에 대해서 생각해봐도 좋을 것 같은데요, 지금은 난수로 이동하는 차들이 제대로 이동하는 지 테스트하는 부분이 컨트롤하기 어려운 부분이에요. 그래서 이를 해결하기 위해 테스트 환경에서만 동작하는 FixedNumberAccelerator 하나만 있어도 좋을 것 같아요. 그리고 그 클래스는 비즈니스로직이 담긴 패키지가 아닌 test 패키지에 구현되어있어도 될 것 같고요.
지금처럼 Accelator라는 인터페이스에 비즈니스 로직에서 동작하는 액셀, 그리고 테스트에서 임시로 동작하는 액셀 정도만 있어도 충분하다는 생각이 들어요~
구현할 때 여러 방면에 대해서 고민해보시면 좋을 것 같아서 제 생각을 한번 남겨봐요ㅎㅎ
There was a problem hiding this comment.
이든에게 궁금한 점이 있어요ㅎㅎ
우선 전략패턴을 학습해보고 적용까지 해 주신 것은 좋네요👍 다만, 고민해봤으면 하는 부분이 있어요.
- 비즈니스 로직에서의 요구사항은 무엇일까?
우리의 요구사항은 난수만큼 각 자동차들이 이동하고, 그 결과를 유저에게 보여주는 것인데요, 난수 생성 이외에 다른 케이스에 대응하기 위해 전략패턴을 사용해야할까?라는 생각이 들어요.추가로 테스트 하기 좋은 코드에 대해서 생각해봐도 좋을 것 같은데요, 지금은 난수로 이동하는 차들이 제대로 이동하는 지 테스트하는 부분이 컨트롤하기 어려운 부분이에요. 그래서 이를 해결하기 위해 테스트 환경에서만 동작하는 FixedNumberAccelerator 하나만 있어도 좋을 것 같아요. 그리고 그 클래스는 비즈니스로직이 담긴 패키지가 아닌 test 패키지에 구현되어있어도 될 것 같고요. 지금처럼 Accelator라는 인터페이스에 비즈니스 로직에서 동작하는 액셀, 그리고 테스트에서 임시로 동작하는 액셀 정도만 있어도 충분하다는 생각이 들어요~
구현할 때 여러 방면에 대해서 고민해보시면 좋을 것 같아서 제 생각을 한번 남겨봐요ㅎㅎ
Test를 위한 전략은 test 패키지에 구현하는 부분이 많이 와닿네요. 저도 전략패턴을 적용하면서, 테스트에서만 사용할 기능인데 비즈니스 로직 단에 추가하는 것이 맞는가에 대해서 많이 고민했었는데요! test 패키지에 구현해두면 test에서만 사용하는 부분이라는 점을 더욱 정확히 명시할 수 있다는 생각이 드네요!
또 테스트에서 사용하는 전략이 하나만 있어도 좋을 것 같다는 의견에도 적극 동의합니다! 다만 아직 어떤 식으로 구현해야할 지 생각해보지 않아서 조금 생각해봐야겠네요:)
좋은 리뷰, 인사이트 제공해주셔서 정말 감사합니다:)
| public void pushAccelerator() { | ||
| moveForward(accelerator.push()); | ||
| } | ||
|
|
||
| public void moveForward(int power) { | ||
| if (power >= MIN_MOVABLE_POWER) { | ||
| private void moveForward(int power) { |
There was a problem hiding this comment.
외부에서 사용되지 않는 메서드를 private으로 바로바로 바꿔주시는 것은 매우 좋네요 👍
There was a problem hiding this comment.
외부에서 사용되지 않는 메서드를 private으로 바로바로 바꿔주시는 것은 매우 좋네요 👍
전략패턴 적용 덕분에 캡슐화를 지킬 수 있었습니다 😄
주요 리팩토링 내용
⭐️ 자동차 이동에 필요한 랜덤값 생성을 테스트하기 위해
전략 패턴을 적용RandomMove전략Move전략NoneMove전략이를 통해 비즈니스 로직에서 사용하지 않지만 테스트 코드를 위해 접근제어자를
public으로 설정하였던Car객체의moveForward(int power)메서드를private으로 설정하여 캡슐화가 깨졌던 문제를 해결⭐️
Exception발생 시, 다시 입력받도록 구현.depthException발생 시, 예외 메시지 출력 후 재입력 받도록 구현depth1 제한으로 인해, 재귀를 사용하였습니다🥲 재귀로 인해 발생할 수 있는stackoverflow해결책은 고민중인데 제출 시간 제한 때문에 우선 리뷰 요청 후 더 고민해서 해결해보겠습니다!리뷰 포인트