[라빈] 자동차 경주 - TDD 미션 리뷰 요청입니다. #68
Conversation
문자열 덧셈 계산기 구현
…Calculate.java 에서 모든 메소드를 \n -> \\n 으로 수정
… 우승자 이름을 만드는 기능을 메소드로 추출
… / 한 자리의 숫자를 입력시 그 숫자를 그대로 반환하는 덧셈 기능 구현 및 테스트 코드 작성
…성 / 커스텀 구분자로 문자열을 나눠서 숫자들의 합을 반환 테스트 코드 추가(프로덕션 코드 작성 해야함)
…스트 목적에 맞게 DisplayName 수정
…/ CarName 을 테스트 하는 코드 추가
…acingGame 에서 RacingLab 만큼 게임이 실행되었는지 확인하는 메소드 테스트 추가
| Assertions.assertThatThrownBy(() -> { | ||
| position.move(number); | ||
| }).isInstanceOf(IllegalArgumentException.class); |
There was a problem hiding this comment.
move 메소드 내에 예외가 발생하는 부분이 없어 테스트시 에러가 나네요.
테스트 시 에러 발생 유무는 세팅, 버전에 따라 다를 수 있을 것 같긴하지만, move에서 예외 발생이 불가능하므로 지금의 테스트는 잘못되었습니다.
There was a problem hiding this comment.
휴! 안녕하세요 라빈입니다. 주말에도 꼼꼼한 피드백 감사하고 정말 많은 도움이 되었습니다. 제가 처음 피드백과 금요일에 포비의 수업을 듣고 처음 구조에서 많이 변경하면서 이런 실수가 나왔습니다. RandomNumber 를 클래스로 추출해서 RandomNumber 가 생성될 때 이미 예외를 검사하고 있어서 Position 을 이동시킬 때 이제 예외가 발생하지 않도록 해놓고서 테스트 코드를 지우지 않았네요. 지적 감사합니다! 더 꼼꼼하게 보고 피드백 요청하도록 하겠습니다.
| this.position = new Position(); | ||
| } | ||
|
|
||
| public void moveCar(int number) { |
There was a problem hiding this comment.
지금과 같은 메소드명을 사용하면 car.moveCar(1)형태로 사용하게 됩니다.
변수명을 의미없게 짓지 않은 이상 car.move(), bmw.move(), oldCar.move()과 같이 이미 move를 하는 주체가 누구인지는 알수 있습니다. 그런의미에서 메소드 명에서 반복하실 필요는 없답니다!
There was a problem hiding this comment.
https://www.wepware.com/web/l/1EW_KUoxQLM
원래 어려운것 맞아요... 😭
안그래도 어제도 동료 분이 변수명 지어줘서 일의 40% 해줬다는 우스갯소리 오갔네요 😄
| private final static String ERR_MESSAGE_FOR_INVALID_NAME = "이름을 잘못 입력하였습니다."; | ||
| private final static String ERR_MESSAGE_FOR_NAME_WITH_BLANK = "공백을 포함한 이름을 입력하였습니다."; | ||
| private final static String ERR_MESSAGE_FOR_NAME_LENGTH_OVER = "5글자 초과의 자동차 이름을 입력하였습니다."; |
There was a problem hiding this comment.
반복하여 사용하지도 않고 작성된 메시지가 매직넘버처럼 메시지만 봐서 무슨 의미인지 알기 어렵지도 않은 점에서 상수로 따로 뺼 필요가 있을지 생각해볼 여지가 있네요. 도리어 31 Line의 (carName.length() > 5) 5를 MAX_NAME_LENGTH와 같이 바꿔주는게 더 좋것 같습니다.
There was a problem hiding this comment.
이번에 상수를 관리하는 부분에서 많은 도움을 받게되었습니다. 감사합니다! 다시 한번 코드를 보면서 상수로 분리해서 가독성을 높여주는 부분이라면 상수로 뺏습니다. 지적해주신 에러 메세지를 상수로 관리할 필요가 있을지에 대해서도 다시 한번 생각을 해봤고 상수로 빼지 않고 직접 작성하였습니다.
| this.cars.stream() | ||
| .forEach(car -> car.moveCar(new RandomNumber().getRandomNumber())); |
There was a problem hiding this comment.
이번과는 조금 다른 내용이지만 Java8 Stream은 loop가 아니다.
단순한 반복문이라면 stream보다는 반복문을 활용해주는게 좋다고 생각합니다. 이왕이면 단순 반복문이라면 향상된 for문으로!
다만 모든 분들한테 반복적으로 말씀드리는 부분인데 stream에 익숙해지는 차원에서 의도적으로 stream을 사용하는 거라면 의도대로 잘 사용하시는 것이기 때문에 사용을 적극 추천드립니다. 사용을 자주 해야지 stream 작성에 익숙해지고 필요할 때 원하는대로 잘 작성 할 수 있다고 생각합니다. 그런 의미에서 지금 다시 바꿀 필요는 없어 보입니다!
There was a problem hiding this comment.
지난 미션에서 enum 을 다룰 때 stream 을 처음 사용했었는데 그때 익숙하지 않아서 어려움이 있었습니다. 이번 미션에서 연습을 해보고자 의도적으로 반복문이 필요한 부분에 적용할 수 있다면 적용했습니다. 링크 해주신 문서를 보고서 새로운 점도 알게되었습니다. 앞으로는 반복문과 stream 의 차이점을 이해하고 적재적소에 사용하는 연습을 하도록 하겠습니다. 감사합니다!
There was a problem hiding this comment.
학습과정을 나눠서 진행하길 추천드려요. 그런 차원에서 아직은 stream을 과도하다 싶게 사용해보시걸 추천드립니다. 일주일 정도 사용해본걸론 다양한 상황들을 겪지 못해서 익숙해지기 쉽지도 않고 과도하게 사용하다 보면 무리해서 사용하다 보니 stream에 어떤 연산들이 있는지 알기 좋답니다. 다만 이건 리뷰어가 코드를 볼 때 그 의도를 모르면 오해 할 수도 있긴하고, 의견이 다를 수는 있다고 봅니다.ㅎㅎ
저도 예전에 stream에 익숙해져보려고 알고리즘 문제들을 stream으로만 무리하게 풀어본 적도 있었네요 😸
There was a problem hiding this comment.
좋은 학습방법 추천 감사합니다. 반복문은 다른 언어나 자바에서도 많이 사용해봐서 익숙한데 stream 에 있는 연산들이 무엇이 있고 익숙하게 다루지를 못하니 과도하게 사용하면서 익혀보는게 좋은 방법인것 같습니다! 그대신 말씀해주신대로 리뷰어분들이 그 의도를 모르실테니 피드백을 받기 전에 연습중이라고 말씀을 드려야겠습니다. 조언 감사합니다! 😃
There was a problem hiding this comment.
그렇지만 어디까지나 stream이 학습의 중심은 아니기 때문에 주객전도 되진 않게 해주세요!
- 테스트 코드를 짜시니까 stream을 이용해서 금방 못 짜겠다 싶은 부분은 반복문으로 먼저 구현을 해서 테스트 통과 후 스트림으로도 만들어 볼지 말지 고민 해 보세요.
아무래도 이미 테스트를 통과하게 구현을 해 놓고나면 심적으로도 편해집니다. 다만 말씀 드렸다시피 스트림 학습은 어디까지나 굉장히 작은 일부분 중 하나니까 객체나 테스트 등 집중 해야 할 다른 것에 소홀해지진 않게 조심해주세요!
| boolean flag = false; | ||
| while (!flag) { | ||
| try { | ||
| racingLab = new RacingLab(InputViewer.getRacingLab()); | ||
| flag = true; | ||
| } catch (IllegalArgumentException e) { | ||
| OutputViewer.printErrorMessage(e.getMessage()); | ||
| } | ||
| } |
There was a problem hiding this comment.
try {
racingLab = new RacingLab(InputViewer.getRacingLab());
} catch (IllegalArgumentException e) {
OutputViewer.printErrorMessage(e.getMessage());
initializeRacingLab();
}이처럼 바꿀 수도 있을 것 같네요.
There was a problem hiding this comment.
이 부분에 대해서는 질문이 있어서 수정하지 않고 리뷰 요청합니다. try ~ catch 구문을 이용하여 재귀함수로 메소드를 만든다면 사용자가 계속해서 비정상적인 입력값을 입력했을 때 콜 스택에 메소드 호출이 계속해서 쌓이게 되어서 메모리가 누적되어서 저는 예외를 출력해주는 반복문으로 구현했습니다. 제가 혹시 잘못 생각하거나 놓치는 부분이 있나요? 그리고 재귀함수로 구현하는 것으로 피드백을 주셨는데 이 때의 장점을 알 수 있을까요? 감사합니다!
There was a problem hiding this comment.
과제로서 콜 스택이 초과 할 상황에 대해서는 상정하지 않고 간단하게 예외 처리를 해보는 것을 의도하고 말씀드렸던 것이랍니다. 예외 처리가 생각보다 간단하지도 않고 버겁게도 느껴지는데 예외 처리로 단순히 에러 메시지를 출력하는 정도가 아니라 프로그램의 흐름도 정상적인(?) 방향으로 다시 돌려 놓을 수 있다는 걸 경험해보게 하고 싶었네요. 😄
재귀로 제안 드린 이유는 지금 형태에서 가장 간단히 재입력 받을 수 있는 형태가 재귀로 바꾸는 걸로 보여서 제안드린거지 재귀의 장점이 있어 말씀드린 건 아닙니다.
… 받아오도록 변경 / 직관성이 높아지도록 각 클래스마다 상수 변경 / Cars 의 addCar() -> initializeCar() 로 변경 / PositionTest 에서 move 를 할 때 예외가 발생하지 않도록 설계했으므로 테스트 삭제 / CarTest 에 자동차의 위치가 잘 변하는지 테스트 케이스 추가
| this.cars.stream() | ||
| .forEach(car -> car.moveCar(new RandomNumber().getRandomNumber())); |
There was a problem hiding this comment.
학습과정을 나눠서 진행하길 추천드려요. 그런 차원에서 아직은 stream을 과도하다 싶게 사용해보시걸 추천드립니다. 일주일 정도 사용해본걸론 다양한 상황들을 겪지 못해서 익숙해지기 쉽지도 않고 과도하게 사용하다 보면 무리해서 사용하다 보니 stream에 어떤 연산들이 있는지 알기 좋답니다. 다만 이건 리뷰어가 코드를 볼 때 그 의도를 모르면 오해 할 수도 있긴하고, 의견이 다를 수는 있다고 봅니다.ㅎㅎ
저도 예전에 stream에 익숙해져보려고 알고리즘 문제들을 stream으로만 무리하게 풀어본 적도 있었네요 😸
[라빈] 자동차 경주 - TDD 미션 리뷰 요청입니다.
감사합니다.