[자동차 경주] 인상진 미션 제출합니다. #8
Conversation
HanHyunsoo
left a comment
HanHyunsoo
left a comment
There was a problem hiding this comment.
안녕하세요 상진님 리뷰어 한현수입니다.
전체적인 코드를 봤을 때 완벽한 MVC패턴은 아니지만 각 역할을 잘 분리하신 것 같아요. 하지만 의문인 점이 몇가지 있어요.
- view부분에서 static 메서드를 사용한 이유? -> view 객체를 생성해서 controller에 클래스변수로 등록해서 메서드를 사용하면 되는것이 더 나은 것 같습니다.
- Cars에서 우승자를 구하는 메서드가 모델의 역할인지 의문이 들어요 Controller에서 처리하는 것이 역할적으로 맞는 것 같습니다.
- 입력에 대한 검증이 완벽하지 않은 것 같아요 더 찾아보시면 상진님이 고려한 예외 말고도 다른 예외가있을 것으로 예상합니다. 테스트코드를 작성하면서 찾아보면 더 빠르게 찾을 수 있을 거에요.
- 이건 코드와 별 상관없긴한데 .DS_Store 파일 삭제하시고 gitignore에 추가하시길 ㅎㅎ.. 맥에서 인덱싱 하기위한 파일인데 앞으로 프로젝트 시작하실때 .DS_Store파일 gitignore에 먼저 추가하시면 git에 트래킹하는 것을 방지할 수 있어요.
| OutputView.printWinnerCars(findWinners()); | ||
| } | ||
|
|
||
| public void Racing(int repeat) { |
There was a problem hiding this comment.
변수명 첫글자가 대문자인데 큰 이유가 없다면 소문자(카멜 케이스)로 통일하는게 좋을 것 같아요.
|
|
||
| import camp.nextstep.edu.missionutils.Console; | ||
|
|
||
| import java.awt.*; |
There was a problem hiding this comment.
GUI 라이브러리를 사용하지 않는 것 같은데 불러온 이유가 따로 있을까요?
| this.cars = new ArrayList<>(); | ||
| } | ||
|
|
||
| public void inputCarName(String[] carNames) { |
There was a problem hiding this comment.
해당 메서드를 생성자로 변경해도 괜찮을 것 같아요. 그러면 inputCarName 메서드를 호출할 필요가 없겠죠?
| return Randoms.pickNumberInRange(0, 9); | ||
| } | ||
|
|
||
| public List<String> findWinners() { |
There was a problem hiding this comment.
Car 클래스를 반환하는 것이아니라 이름을 반환하는 것이니 좀 더 명확한 의미를 가지도록 메서드 명을 변경해보세요!
|
|
||
| public class CarNameValidate { | ||
|
|
||
| public static void validateCarName(String[] carNames){ |
There was a problem hiding this comment.
메서드 명만 보면 차 이름 하나로 Validation하는 메서드인줄 알았는데 이름 여러개를 검증하다보니 헷갈리는 것 같아요. 한개의 매개변수에 대해서만 검증하는 로직으로 변경하거나 메서드 명을 변경하시면 좋을 것 같아요.
|
|
||
| import camp.nextstep.edu.missionutils.Console; | ||
|
|
||
| public class InputView { |
There was a problem hiding this comment.
View에 해당하는 클래스 메서드들을 static으로 한 이유가 궁금해요. non-static으로 하고 객체를 생성해서 사용하는게 좀더 좋지 않을까요?
There was a problem hiding this comment.
확장성을 고려하지 못한거 같았습니다. static메서드를 사용하지 않고 객체를 만들어서 메서드를 사용하는게 더 좋을거 같네요! 길어지는게 싫어서 썼던거 같습니다.
| } | ||
|
|
||
| public static void showCarPosition(Car car) { | ||
| for (int i = 0; i < car.getPosition(); i++) { |
There was a problem hiding this comment.
좀 더 리팩토링 할 수 있을 것 같아요 String 관련 메서드를 찾아보세요
|
|
||
| import java.util.List; | ||
|
|
||
| public class OutputView { |
There was a problem hiding this comment.
showCar, showCarPosition에서 문자열 조합 관련 로직이 보이는데 출력 메서드(ex - println)는 많은 자원을 소모해요
그래서 문자열을 조합한다음에 마지막에 한번에 출력하도록 리팩토링 해봐요
| @@ -1,7 +1,10 @@ | |||
| package racingcar; | |||
|
|
|||
|
최근 너무 할 일이 많아져서 급한 대로 최대한 고쳐봤습니다.. 다음 주쯤에 다시 천천히 보면서 코드를 점검해 보겠습니다. |
|
리뷰 감사합니다! |
2 participants
MVC 패턴을 사용해서 구현해 보는 방식이 익숙지 않아서 많이 헤맨 거 같습니다.
Controller layer는 View와 Model layer에 역할을 부여해 주며 작동시키도록 만들었고 전체적으로 Model layer에 서비스로 직을 집어넣었습니다. Controller를 좀 더 나눌 수 있을 거 같은데 잘 안됐습니다. 각 계층 간의 의존성을 낮추려 노력했는데 이에 맞게 MVC 패턴을 잘 적용시켰는지 궁금합니다!
TDD 위주의 설계를 하지는 못했지만 계발 중 고민이 많았던 부분을 테스트 코드로 구현해 봤습니다.