[1단계 - 자동차 경주 구현] 다니(이다은) 미션 제출합니다. #184
Conversation
There was a problem hiding this comment.
안녕하세요 다니! 이번에 리뷰를 하게된 고래입니다 🐳
다양한 어노테이션을 사용하여 꼼꼼하게 테스트를 진행한 점이 인상적이네요~!
몇 가지 피드백 남겼는데요~ 제가 남긴 코멘트가 정답이라고 생각하지 마시고 다른 의견이나 궁금한 점이 있으면 언제든 DM주세요~
-
상수의 위치
과거에는 하나의 클래스에 상수들을 모아놓고 관리하기도 했다고해요.
개인적으로는 상수가 필요한 곳에 위치하는 것이 좋다고 생각합니다. 한 곳에 모아서 관리할 경우, 상수가 많아지면 오히려 관리하기 힘들 수 있을 것 같아요
반대로 필요한 클래스에 위치하면, 정확한 의미도 파악할 수 있고 불필요한 의존도 줄어들겠죠?
개발이 생각보다 정답이 없는 부분이 많아서 꼭 어떤 방식으로 하시기보다, 여러 방식을 직접 경험해보시고 장단점을 느껴보시길 바랍니다 😄 -
move()
테스트를 하기 위해 고민하신 것이 느껴졌습니다...ㅎㅎ
다양한 방법이 있겠지만, 움직이는 건 Car의 역할이라고 생각해요.
다니는 Service layer가 어떤 역할을 한다고 생각하는지 궁금합니다🤔 -
테스트 코드
규칙을 지켜서 나쁠건 없겠죠?ㅎㅎ
테스트 코드는, 코드를 처음 접하는 사람에게 설명서가 될 수도 있기 때문입니다.
다만 상황에따라 상대적으로 느슨하게 해도 된다고 생각해요~
예를 들면, 테스트에선 다양한 조건을 설정하고 진행해야하는 경우가 있습니다. 가끔은 이러한 조건들을 명시해야 한 눈에 들어올 때도 있더라구요 :) -
메서드 네이밍
변수명, 메서드명 짓는 부분에서 저를 포함한 많은 개발자 분들이 고민을 많이 하실거에요ㅋㅋㅋ
자바 메서드 네이밍에는 몇 가지 관례가 있어요. 그 외에는 기본적으로 동사로 시작하여 의미를 명확히 작성하시면 됩니다
2기분들이 작성하신 좋은 글이 있어서, 참고하시면 도움되실 것 같아요~!
| import java.util.stream.Collectors; | ||
|
|
||
| public class CalculatorController { | ||
| private static final String PATTERN = "//(.)\\\\n(.*)"; // 정규표현식 \\ -> \ , 자바 리터럴 \\ -> \ |
|
|
||
| public class CalculatorController { | ||
| private static final String PATTERN = "//(.)\\\\n(.*)"; // 정규표현식 \\ -> \ , 자바 리터럴 \\ -> \ | ||
| private static final int NULL_OR_EMPTY_RESULT = 0; |
| return getResult(input); | ||
| } | ||
|
|
||
| public int getResult(String input) { |
There was a problem hiding this comment.
한 메서드에서 input 검증부터, 파싱, 분리, 계산까지 하고있어요
동료가 getResult라는 메서드명을 보고 이러한 로직들을 포함한다고 예상할 수 있을까요?
역할을 분리하고 메서드명을 고민해보아요 😄
| } | ||
|
|
||
| private boolean hasCustomDelimiter(String input) { | ||
| Matcher m = Pattern.compile(PATTERN).matcher(input); |
There was a problem hiding this comment.
메서드를 호출할 때마다 매번 Pattern.compile()를 호출하는데 이는 비용이 비싸다고 합니다ㅎㅎㅎ
미리 만들어 놓고 그 비용을 한 번으로 줄일 수 있으면 더 좋을 것 같아요!
| } | ||
|
|
||
| public String getCustomDelimiter(String input) { | ||
| final int delimiterIndex = 1; |
There was a problem hiding this comment.
delimiterIndex라는 좋은 변수명을 지어주셨으니, 상수로 빼도 좋을 것 같아요 :)
|
|
||
| public void playUntilDone(Cars cars, Times times) { | ||
| while (!times.isZero()) { | ||
| carService.decideMovableCar(cars, null); |
There was a problem hiding this comment.
null을 전달, 반환할 경우 귀찮은 체크가 많아질 수 있습니다.
null보단 의미있는 객체를 넘기는 건 어떨까요?
|
|
||
| public class CarService { | ||
| public void decideMovableCar(Cars cars, List<Integer> randoms) { | ||
| List<Car> carList = cars.getCars(); |
There was a problem hiding this comment.
Cars라는 일급 컬렉션을 구현했으니, getter보단 Cars에게 메세지를 전달하는 방식으로 구현해보시는 건 어떨까요?
| } | ||
|
|
||
| private List<Integer> getRandoms(List<Car> cars) { | ||
| List<Integer> randoms = new ArrayList(); |
There was a problem hiding this comment.
| List<Integer> randoms = new ArrayList(); | |
| List<Integer> randoms = new ArrayList<>(); |
keyword. Generic
| } | ||
|
|
||
| public void playUntilDone(Cars cars, Times times) { | ||
| while (!times.isZero()) { |
| } | ||
|
|
||
| @Override | ||
| public String toString() { |
There was a problem hiding this comment.
toString()메서드가 View에 의존적으로 사용되는것 같아요~
toString()은 보통 객체의 상태를 표현하기 위해 사용되어, 정해진 규칙에 의해서 재정의하는게 좋습니다.
There was a problem hiding this comment.
65fb094 수정했습니다!
이렇게 변경하는 게 맞는 방식인가요?? 리팩토링하면서 궁금했어요....! 😳
|
안녕하세요, 김고래 ! 리뷰 감사합니다 😆 2번에 대해 고민해봤는데, 저는 Service Layer가 비즈니스 로직 처리를 담당하는 층이라고 생각했어요. 또, 김고래가 남겨주신 답변과 리뷰
이 내용들을 바탕으로 move에 관한 건 Car, Cars가 가져가야 한다고 결론을 내렸어요! 따라서 decideMoveableCar를 Cars에 옮기는 방향으로 리팩토링했습니다! 이번에도 귀한 시간 내주셔서 감사합니다!! 🙌 |
|
아마 내일 저녁에나 리뷰를 드릴 수 있을 것 같아서... 혹시 심심하실까봐... 몇가지 키워드 던져드릴게요ㅎㅎㅎㅎ;
자기 전에 간단히 적는 거라 두서가 없을 수 있어서, 궁금한 점은 언제든 DM주세요~! |
|
새벽에 남겨주신 코멘트
바탕으로 리팩토링 다시 해봤어요!
전략패턴은 아직 이해가 잘 되지 않아서 조금 더 공부해봐야 할 거 같아요...! |
|
|
||
| Cars cars = new Cars(carNames, positions); | ||
|
|
||
| assertThat(cars.giveWinners().get(0)).isEqualTo(winnerName); |
There was a problem hiding this comment.
| assertThat(cars.giveWinners().get(0)).isEqualTo(winnerName); | |
| assertThat(cars.giveWinners()).containsExactly(winnerName); |
이런 방식도 있습니다~
| .collect(Collectors.toList()); | ||
| } | ||
|
|
||
| public Cars(List<String> carNames, List<Integer> positions) { |
There was a problem hiding this comment.
움직이는건 Car가 판단해도 좋은데, 현재는 Cars라는 친구가 억지로 밀어서 가는 느낌?ㅎㅎㅎㅎ
좋은 예시를 들어드리고 싶은데 생각이 안나네요...ㅋㅋㅋ
우테코 피드백 자료도 참고해서 반영해보는 것도 좋을 것 같아요 😄
안녕하세요, 김고래!
첫 리뷰 요청으로 만나뵙게 된 다니입니다. 잘 부탁드립니다! 😊
저는 이번 자동차 경주를 구현하면서 TDD와 페어 프로그래밍을 처음 경험했습니다.
특히, 테스트 코드를 작성할 때 어려운 점이 많았던 것 같습니다.
그래도 페어 덕분에 기능 구현을 잘 마쳤다고 생각합니다!
구현에 앞서 기능 요구사항을 정리하고, MVC 패턴을 바탕으로 디렉토리 구조를 설계했습니다.
구현은 기능별로 페어와 드라이버, 네비게이터를 번갈아가며 진행했습니다.
미션을 하면서 궁금한 점이 몇 가지 생겼는데요, 질문 드려도 괜찮을까요?
궁금한 점 1.
상수를 선언할 때, 각 클래스 상단에 public static final 이런식으로 쓰지 않고
enum 클래스를 만들어 사용했는데요. enum을 활용하면 한 클래스에서 관리할 수 있어 편리할 것이라 생각했는데, 한편으로는 클래스를 옮겨다녀야 하니까 불편할 거 같기도 해요. 둘 중 어떤 방식이 더 좋은 건가요??궁금한 점 2.
자동차가 전진하는 기능을 구현할 때, Car 도메인에 move 함수를 선언해서 자동차를 움직이게 했고 CarService 서비스에 decidemoveableCar 함수를 선언해서 random 값을 기준으로 자동차가 전진하거나 멈추게 만들었는데요. 이렇게 하면 자동차 객체가 하는 일인
move가 도메인, 서비스 두 곳으로 나누어지는 느낌이어서 좋은 설계가 맞는지 고민이 됐어요. move에 대한 코드를 도메인, 서비스에 나누지 않고 Car 도메인에서 모두 작성하는 게 더 좋은 방법일까요??궁금한 점 3.
프로덕션 코드를 작성할 때는 프로그래밍 요구사항에 따라 함수의 길이가 10라인 이내가 되도록 했어요. 테스트 코드를 작성할 때는 이 요구사항을 일부 함수에서 지키지 못했는데요. 테스트 코드도 함수 길이 제한을 두고 작성하는 게 좋을까요??
궁금한 점 4.
네이밍을 할 때, 어떻게 정해야 직관적인 이름일지 고민이 많아요. 특히, getRandoms 함수처럼 어떤 값을 가져올 때 자꾸
get동사를 쓰게 돼요. 이런 경우 김고래는 어떤 동사를 사용하나요?? 네이밍 팁 공유해주시면 감사하겠습니다! 😄프리코스와 이번 미션을 진행하며 궁금했던 부분을 작성하다보니 내용이 약간 길어졌습니다..!
처음으로 코드 리뷰를 받는 건데, 지금 너무 설레요!
귀한 시간 내주셔서 감사합니다! 🤗