[2단계 - 자동차 경주 리팩터링] 이프(송인봉) 미션 제출합니다. #410
Conversation
* style: 상수 문자열 명칭 수정
* feat: RandomUtil 클래스 생성, 싱글톤 패턴
* refactor: 사용하지 않는 클래스 제거 * refactor: 불필요한 import 제거 * refactor: 누락된 final 선언 추가
Step1 마지막 리뷰에서의 질문 답변
Enum 필드는 final로 선언되어 재할당을 금했기 때문에, 외부에서 이를 가져다가 사용을 해도 때문에 외부에서 Enum 클래스의 필드 값을 직접 사용해도 괜찮다는 생각을 가졌지만, 그러한 이유는, 변경에 용이하다는 장점 때문입니다. |
- 이전에 커밋한 "Arrays.asList()를 List.of()로 변경" 내용 되돌리기
ksy90101
left a comment
ksy90101
left a comment
There was a problem hiding this comment.
이프 안녕하세요 :) 리뷰어 럿고입니다.
1단계때 이어 2단계떄는 테스트코드도 풍부해지고, 훨씬 깔끔한 코드가 된거 같습니다 :)
너무 고생많으셨습니다 💯
몇가지 피드백을 남겼습니다 :) 한번 확인해보시면 좋을듯 합니다.
아울러 혹시 이프의 생각이 궁금한게 생겼는데요.
현재 이프의 코드를 보면 모두 객체로 만들어서 파일이 굉장히 많은데요
이런 경우에 장단점이 있을꺼 같아요. 혹시 이프가 느끼신 장단점이 있으실까요?
| } | ||
|
|
||
| public int splitAndSum(String text) { | ||
| public int splitAndSum(final String text) { |
There was a problem hiding this comment.
코드를 작성하다보면 메소드 안에서, 변수 하나를 적어도 한번쯤은 가공해서 재사용하는 경우가 생기게 됩니다.
이렇게 되면 메소드의 시작부분과 끝부분에서 해당 변수의 값이 변경되어버려 로직을 들여다 보지 않는 이상,
해당 값을 유추하기 어려워집니다.
물론 값이 변경되는 부분에서 새로운 이름의 변수를 할당하여 시작부분의 변수와 끝부분의 변수를
분리할 수 있지만.. 개발자도 사람인지라 변수의 값이 변경해버리는 실수를 범할 수 있습니다.
때문에 변수에 final을 선언하게 되면, 재할당을 방어하여 이러한 실수를 차단할 수 있습니다.
물론 우리는 메소드의 길이를 짧게 유지하고 있습니다. 때문에 메소드의 로직을 이해하는 비용이 과연,
실수를 방지하고자 모든 변수에 final을 선언하는 일보다 더 큰가 하는 궁금증도 가지고 있지만..
비용의 관점 외에 다른 문제점이 없다면, 차라리 빠르게 습관화하는 것이
앞으로의 프로그래밍 관점에서 좋다고 판단했습니다!!
There was a problem hiding this comment.
👍 final을 모두다 붙이면 가지게 되는 단점도 잘 인지하고 계시는군요!!
개인적으로 너무 좋은 이유가 된거 같습니다.
그러나 너무 맹신은 안하시면 좋을거 같아요. 한번 사용해보시고, 여러 사람들의 의견을 들어보고 상황에 따라 제각기 사용하는게 가장 좋은거 같아요 👍
| import racingcar.service.GameService; | ||
| import racingcar.view.input.reader.CustomReader; | ||
|
|
||
| class GameControllerTest { |
There was a problem hiding this comment.
GameServiceInitTest에서 자동차 이름, 실행횟수 생성에 대한 예외테스트를 진행하고 있어서
GameControllerTest에서도 이를 진행하는 것은 중복확인이 될까봐 제외했는데요.
추가하는 것이 좋을까요??
| @ParameterizedTest | ||
| @MethodSource(PROVIDER_PATH + "provideForDuplicateExceptionTest") | ||
| void carNamesDuplicatedExceptionTest(final List<String> names) { | ||
| exceptionTest(names); |
There was a problem hiding this comment.
이 테스트에서 무조건 자동차 이름의 중복 예외라고 보장할 수 있을까요?
assertJ에서는 아래와 같은 메서드를 제공하고 있는데요. 혹시 message까지 확인한다면 정확하지 않을까요?
hasMessageContaining()
There was a problem hiding this comment.
아.. 이 부분을 깊이있게 생각하지 못했네요.
확인하고자 하는 예외가 발생했는지, 예외메시지를 통해 확인할 수 있도록 변경했습니다!!
There was a problem hiding this comment.
현업에서는 예외테스트를 어떻게 진행하는지 질문드려도 될까요?
현장에서 다루는 시스템은 엄청 커다랗고 복잡할 텐데, 각각의 예외를 어떤식으로 처리하는지 궁금합니다!!
There was a problem hiding this comment.
저희 회사 같은 경우에는 custom 예외를 많이 사용함으로 custom 예외를 검증하고 있습니다.
아울러 예외메시지까지 정확히 확인할 수 있도록 노력하고 있어요.
또한 에외테스트는 대부분 단위 테스트에서 모두 검증하고 있습니다.
통합테스트는 비용이 비싸다 보니, 우리가 예측하는 성공 테스트 케이스를 대부분 검증하려고 노력하고 있어요.
| import racingcar.view.input.reader.Reader; | ||
| import racingcar.view.output.OutputView; | ||
|
|
||
| public class AppConfig { |
There was a problem hiding this comment.
생성자를 통해 의존성을 주입받도록 설계해놓은 덕분에 Reader, MoveStrategy 인터페이스의 구현체를 변경시켜가며
메인 로직과 테스트 로직을 분리할 수 있었고, 로직에 대한 테스트를 진행할 수 있었습니다.
하지만 이 때문에, 최상위 클래스에서 DI를 관리해야할 필요가 생겨났고
이를 GameApplication에서 다루기에는 코드가 보기안좋아서..(하핳) AppConfig로 분리했습니다!
많이 이상한가요?!ㅜ 공부했으면 하는 개념을 알려주시면, 열심히 학습하도록 하겠습니다!
There was a problem hiding this comment.
👍 네 좋은거 같아요. 아직 이부분은 너무 신경을 안쓰시면 좋겠어요.
현재 미션의 목표는 TDD와 클린코드, 간단한 MVC 설계라고 생각이 들어요.
DI는 충분히 웹 프로그래밍으로 간다면 학습할 수 있으니깐 선택과 집중을 하는걸 추천드립니다 💯
확실히.. 파일이 많죠ㅜ 리뷰하시기 힘드셨을텐데 죄송합니다ㅜ
|
ksy90101
left a comment
ksy90101
left a comment
There was a problem hiding this comment.
이프 안녕하세요 :) 리뷰어 럿고입니다.
피드백에 대한 답변을 꼼꼼히 남겨주셔서 너무 감사드려요.
저도 관련해서 저의 생각을 작성해놨습니다.
이번 미션을 하시면서 많은 것을 배웠을거라고 생각이 듭니다.
너무 많은걸 학습하려고 하는것보다는 선택과 집중을 하셨으면 좋겠습니다
이만 이프의 자동차 경주 게임 미션은 마무리 하도록 하겠습니다 👍
언제든지 질문이나 도움이 필요하시면 슬랙을 통해 연락주세요~
고생많으셨습니다 💯
2 participants
Step2 미션에 대한 PR 리뷰 요청드립니다!!
(도중에 git 관리를 잘못해서 commit을 reset해버리는 바람에 제출이 지체되었습니다..)
다음은 Step1 코드에서 크게 달라진 부분을 정리한 내용입니다.
구조 및 로직 변경
Controller에 존재하던 view 관련 로직을 View 로 옮겼습니다.
Car 도메인의 전진 기능 메소드를 수정했습니다.
매개변수 및 지역변수에 final을 지정하여 불변성을 지니게 했습니다.
테스트 관련 변경 사항
Round / Car / Cars에 대한 테스트 코드 보충했습니다.
Cars에서 이뤄지던 자동차 이름의 null, 빈값, 길이 예외 테스트를 제거했습니다.
GameService에 대한 테스트 코드 작성했습니다.
GameControlelr에 대한 테스트 코드 작성했습니다.