-
Notifications
You must be signed in to change notification settings - Fork 452
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[2단계 - 자동차 경주 리팩터링] 도이(유도영) 미션 제출합니다. #568
Conversation
- service 패키지 삭제, 하위 클래스 domain 패키지로 이동 - ui 패키지명 view로 변경 - controller 패키지 생성, 클래스 이동
- 경주에 참여하는 자동차들을 관리하는 RacingCars 클래스를 만들어 RacingGame 클래스의 책임 분리 - RacingCars에서 자동차 개수 및 중복 이름 검증, 1회 경주 실행, 우승자 산출 책임 담당 - 전달받은 추진력 값(power)에 따라 Car 클래스가 직접 전진 또는 유지할지 결정하도록 변경 - 이에 따라 Car 클래스의 move() 메소드를 moveOrStayByPower(int power)로 변경 - 위 리팩터링 사항으로 인한 테스트 코드 재작성
- RacingGameController의 레이싱게임을 실행하는 메소드명 race를 play로 변경 - RacingGame의 race 메소드 실행 횟수인 파라미터명 trialCount를 racingCount로 변경
- 이전 커밋에서 변경한 trialCount -> racingCount 파라미터명과 맞게 메소드명 수정
- RacingGame에서 지정된 추진력 숫자 생성기를 만들어 경주참여 자동차들에게 전달하도록 함. - 파라미터명을 도메인 의미에 가깝게 PowerValueGenerator 로 변경함.
public class KeyboardReader { | ||
|
||
// TODO 지금과 같은 환경이 아니라 멀티스레드(?) 등의 환경이면 스캐너를 static 변수로 저장해두고 쓰는 게 어떤 영향을 끼치는지? | ||
private static final Scanner scanner = new Scanner(System.in); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
InputView
에서 Scanner 객체를 직접 쓰고, 데이터 타입 변환까지 담당하는 대신
키보드 입력값을 받아서 문자열, 정수, 자연수와 같은 형식으로 변환해주는 유틸리티성 클래스를 별도로 만들었습니다.
그런데 이와 같은 경우 여러 곳에서 하나의 Scanner 객체에 접근하게 되는 상황이 되기 때문에 문제가 있을 것 같은데 아직 멀티스레드 등의 환경에 대한 지식이 없어서 이를 어떻게 생각해야 할지 고민됩니다..!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
동시성을 고민해야하는 환경이라도 단순히 사용자 입력을 받는 것이라면 동시성 이슈로 인해 데이터 무결성이 깨지지는 않을 것 같아요~
if (name.isEmpty()) { | ||
throw new IllegalArgumentException("자동차 이름은 빈 문자열일 수 없습니다."); | ||
} | ||
if (name.length() > MAX_LENGTH) { | ||
if (name.length() > NAME_LENGTH_MAX) { | ||
throw new IllegalArgumentException("자동차 이름은 5자 이하여야 합니다."); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
예외 메시지는 별도 상수로 분리하는 것보다 예외처리 로직과 같이 있는 것이 파악하기 용이하다고 생각해 분리하지 않았습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저도 예외메시지를 바로 확인하는 것도 좋은 방법이라고 생각해요 👍
도이가 명확한 의도를 가지고 구현해 주셨으면 그것이 정답입니다~
validateName(name); | ||
this.name = name; | ||
this.position = position; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CarTest
에서 자동차 위치를 비교할 때, 자동차를 전진시키는 다른 메소드를 사용할 필요가 없도록 하고 싶어서
position
을 바로 설정할 수 있는 생성자를 정의했습니다.
테스트를 위한 생성자 사용에 대해서는 견해가 다양한 것 같은데 괜찮은 방법인지 고민이 됩니다..!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
도이 말씀대로 테스트만을 위한 코드에 대해서 견해가 많은데요, 저는 개인적으로 생성자 같은 경우는 많아도 된다고 생각해요.
이 객체를 생성할 수 있는 다양한 방법을 열어놓은 것이라고 생각하기 때문이에요. (물론 반대하는 의견도 있습니다😊)
도이 질문에 답변 남겨둡니다 😊
도이 말씀대로 리팩터링을 진행할 때는 어쩔 수 없이 하나의 커밋에 많은 변경사항이 있을 수밖에 없는 것 같아요 😭 도이가 해주신대로 자세히 변경내용을 적어주시는 것으로 충분하다고 생각해요 ㅎㅎ 현업이거나 함께 진행하는 프로젝트라고 하더라도, 리팩터링 내용에 대한 사전에 충분한 소통(문서화나 회의) 등이 되었다면 문제 없을 것이라고 생각합니다.
코멘트에 남겨두었어요~😊
👍 좋은 생각입니다 ㅎㅎ 명확히 분리되어서 저도 좋은 것 같아요.
예외 메시지가 조금만 바뀌어도 그 때마다 테스트 코드를 수정해주어야 한다는 단점 도이 말씀이 맞아요! 사소한 변경내용 때문에 테스트코드가 깨진다면 그 뜻은 테스트 코드가 내부구현에 의존하고 있다는 뜻이죠. 그래서 테스트는 내부 구현 변경에 취약하면 안된다는 특징을 가지고 있답니다. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요 도이 🙇♀️ 코다 입니다.
이미 1단계부터 잘 구현해주셔서 2단계에 추가적으로 학습할 수 있는 것들을 많이 고민하셨군요 ㅎㅎ
1, 2단계를 통해서 미션을 통해서 배울 수 있는 것을 다 잘 해주셨어요 👍
도이가 고민하고 적용하고 싶다고 하신 부분이 있었지만, 그 부분은 다음 미션에서도 똑같이 고민하고 적용할 수 있는 부분이라서 바로 머지해요 ~
첫 번째 미션 완료하신 것 축하드립니다 🎉
궁금한 것은 언제든지 DM 주시고 앞으로도 화이팅 입니다 🙌
- [x] 자동차의 정보를 전달받아 이름과 위치를 뷰에 넘겨주는 기능 | ||
- [x] 최종 결과를 넘겨받아 뷰에 넘겨주는 기능 | ||
|
||
## ♻️ 2단계 리팩터링 검토 목록 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
readme를 자세하게 써주셔서 덕분에 저도 편했네요 👍 감사합니다 ㅎㅎ
public class RacingGameApplication { | ||
|
||
public static void main(String[] args) { | ||
RacingGameController racingGameController = new RacingGameController(); | ||
final RacingGameController racingGameController = new RacingGameController(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
final 선언 👍
final Map<String, Integer> converted = new LinkedHashMap<>(); | ||
carsStatus.forEach(car -> converted.put(car.getName(), car.getPosition())); | ||
return converted; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
converted
map을 따로 선언하지 않고 바로 return 할 수 있는 방법이 있는데 무엇일까요? 😊 (힌트: stream)
중요한 코멘트는 아니고, 잘 구현해주셔서 추가질문 차 남겨요 ㅎㅎ
if (name.isEmpty()) { | ||
throw new IllegalArgumentException("자동차 이름은 빈 문자열일 수 없습니다."); | ||
} | ||
if (name.length() > MAX_LENGTH) { | ||
if (name.length() > NAME_LENGTH_MAX) { | ||
throw new IllegalArgumentException("자동차 이름은 5자 이하여야 합니다."); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저도 예외메시지를 바로 확인하는 것도 좋은 방법이라고 생각해요 👍
도이가 명확한 의도를 가지고 구현해 주셨으면 그것이 정답입니다~
public boolean moveOrStayByPower(int power) { | ||
if (isMovable(power)) { | ||
++this.position; | ||
return true; | ||
} | ||
return false; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 부분 추가 질문 주셨던 부분이군요.
도이가 말씀해주신 것처럼 move가 전진인지 정지인지 모르는 것이 좋지 않다고 느낀 것 매우 훌륭하다고 생각해요 👍
movable
한지 아닌지도 Car
에서 가지고 있는 것이 명확하다고 생각하셨다면 그것이 좋은 방법이라고 생각해요.
그런데 boolean을 꼭 리턴해야 할 필요가 있는지 고민해보면 좋을 것 같아요.
질문을 바꿔보자면..
자동차가 움직였는지 아닌지를 알려주는게 자동차 경주 게임 세계에서 Car
가 알려줘야하는 책임이 있나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
남겨주신 코멘트를 보고 생각해보니 자동차가 움직였는지 아닌지를 알려주는
것은 도메인에 필요한 기능이라기보다는
개발하는 입장에서 편의적으로 필요하다고 판단한 부분인 것 같습니다..!! Car
에게 필요한 책임은 아니었다는 생각이 드네요!
막연하게 넘어갈 수도 있던 부분 짚어주셔서 감사합니다 !!
} | ||
|
||
public List<Car> racingCars() { | ||
return Collections.unmodifiableList(racingCars); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
불변리스트 👍
return scanner.nextLine(); | ||
} | ||
|
||
public static int readInt() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
요 함수는 private로 변경하면 좋겠네요 ~
import java.util.Collections; | ||
import java.util.List; | ||
|
||
public class RacingGame { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RacingGame
이 RacingCars
을 그대로 중개해주는 역할인 것 같네요 ㅎㅎ
RacingGame이라는 객체를 도메인으로 볼 수 있을까요?
정답이 있는 것은 아니고, 이번 미션에서 크게 중요한 것도 아니지만, 바로 전달해주기만 하는 도메인이 어떤 의미가 있을지 고민해보는 것도 좋을 것 같아요😊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
중개해주는 서비스와 같은 역할을 하는 클래스라고 생각했는데, 도메인이라고 보기는 어려웠던 것 같아요!
앞으로도 이 부분을 더 고민해보겠습니다 !!
validateName(name); | ||
this.name = name; | ||
this.position = position; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
도이 말씀대로 테스트만을 위한 코드에 대해서 견해가 많은데요, 저는 개인적으로 생성자 같은 경우는 많아도 된다고 생각해요.
이 객체를 생성할 수 있는 다양한 방법을 열어놓은 것이라고 생각하기 때문이에요. (물론 반대하는 의견도 있습니다😊)
public class KeyboardReader { | ||
|
||
// TODO 지금과 같은 환경이 아니라 멀티스레드(?) 등의 환경이면 스캐너를 static 변수로 저장해두고 쓰는 게 어떤 영향을 끼치는지? | ||
private static final Scanner scanner = new Scanner(System.in); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
동시성을 고민해야하는 환경이라도 단순히 사용자 입력을 받는 것이라면 동시성 이슈로 인해 데이터 무결성이 깨지지는 않을 것 같아요~
안녕하세요 코다 ! |
안녕하세요 코다! 남겨주신 피드백을 기반으로 2단계 리뷰 요청드립니다.
2단계 리팩터링에서 주로 고민하고 검토한 내용들을
README
의2단계 리팩터링 검토 목록
에 작성하였습니다.아직 더 고민 중인 부분들이 많은데, 한 번에 모든 걸 수정하려고 하기보다는
코다에게 질문드리고 저도 스스로 더 생각해보면서 진행하고 싶어서 일단 리뷰 요청드리게 되었습니다.
2단계 리팩터링을 진행하며 고민했던 부분들은 아래와 같습니다.
1. 리팩터링 시 커밋 단위
객체 간의 책임을 분리시키기 위해 클래스를 분리하고 책임지는 메소드를 이동하는 등 전면 리팩터링을 진행하다 보니,
테스트코드를 포함해 서로 연관 있는 클래스들을 모두 한 번에 수정해주어야 했습니다.
그러다 보니 너무 많은 변경을 하나의 커밋
3f308d1
에 한 번에 올리게 되었습니다. 대신 커밋 바디에 변경 내용을 자세히 적었지만, 지금보다 훨씬 큰 규모 또는 협업 프로젝트였다면 충분히 문제가 될 만한 상황이었다고 생각합니다.리팩터링 과정에서의 문제보다는 기존의 클래스 간 결합도가 높아서 불가피한 상황이었다고 보고 넘겨야 할지, 더 좋은 방법이 있었을지 고민이 됩니다.
조금 막연하지만 리팩터링 할 때 이런 일을 피할 수 있는 방법이 있을지, 코다의 팁(?)이 있으신지 질문 드려보고 싶었습니다.
2. 기존
move()
메소드와 setter 메소드의 차이, 결과를 알 수 있는 메소드로 만들기1단계에서 저는 자동차의
move()
메소드의 수행 결과가 전진/정지 두 가지 중 어느 것인지 외부에서 알 수 없는 메소드라는 것이 싫었습니다. 그래서move()
메소드는 무조건 전진하는(position에 값을 1 더하는) 메소드로 만들고, 외부에서 이를 호출하거나 호출하지 않는 방식으로 구현했습니다.하지만, 외부에서 언제나
Car
의 position을 1 증가시킬 수 있는move()
메소드는 setter 메소드와 비슷한 단점을 가진다는 생각을 하게 되었습니다. 그리고 자동차 스스로 우승자를 비교할 수 있게 구현하라는 피드백을 주신 것과 마찬가지로, 전진 여부 또한 자동차 스스로 판단하는 것이 객체지향적 관점에서 더 자연스럽다는 생각이 들었습니다. 전진 기준인 상수값 또한Car
에서 직접 관리할 수 있기 때문입니다.그래서 기존에 구현한
void move()
메소드를boolean moveOrStay(int power)
로 변경하였습니다.boolean 반환값은, 그럼에도 앞서 말씀드린 전진/정지 결과를 알 수 없다는 점은 좋지 않다고 생각하여
전진했을 때는 true, 정지해있을 때는 false를 반환하도록 만들었습니다.
그런데 이는 불필요한 반환값인지 고민이 됩니다.
3. 테스트 코드 내부 클래스 분리
Car
클래스의 테스트 중 생성자 테스트/ 기능테스트에 따라@BeforeEach
로 중복을 제거할 수 있는 테스트와, 그렇지 않은 테스트가 나뉘는 상황이었기에 내부 클래스로 테스트를 나누고, 생성 외 기능 테스트에서만@BeforeEach
를 적용하였습니다.모두 같은
Car
클래스에 대한 테스트이지만 그 안에서도 테스트 유형이 달라지고, 위와 같이 중복을 줄일 수 있는 상황이기에 테스트 코드를 더 관리하기 좋은 방식이라고 생각했습니다.4. 예외 테스트에서 메시지 비교를 통한 정확한 원인 파악
지난 1단계 마지막 피드백으로 주셨던 내용인데요, 이 부분은 아직 더 고민하면서 진행하고 싶어서 커밋에는 반영이 되지 않은 상태입니다.
예외테스트에서
Exception
의message
를 비교하여 어떤 원인에 대한 예외가 발생한 것인지 알면 더 정확한 테스트가 될 것이라는 피드백을 주셔서 생각해봤는데요!hasMessage()
메소드를 이용해 메시지를 비교하면 코다가 말씀해주신 대로 정말로 의도한 예외가 발생한 것인지 정확히 파악할 수 있을 것 같아요.하지만 제가 고민이 되는 부분은,
Exception
객체나 단순 값을 비교 할 수 있는 다른 결과값들과 달리, 예외 메시지는 같은 의미를 가져도 문자열의 정확한 내용은 달라질 수 있기 때문입니다. (ex
”자동차 이름은 공백일 수 없습니다.” → “공백이 아닌 이름을 입력해야 합니다.”)그리고 테스트 코드에 위와 같은 예외 메시지를 직접 적어두면, 예외 메시지가 조금만 바뀌어도 그 때마다 테스트 코드를 수정해주어야 한다는 단점이 걸려서 더 고민해보고 싶습니다.
예외 메시지를 상수 클래스로 관리한다고 해도, 해당 상수를 테스트 코드에서 import하는 것 또한 프로덕션 코드에 너무 의존적인 테스트가 되는 것이 아닐까 고민됩니다. 그래서 이 부분은 반영하기 전에 코다는 어떻게 생각하시는지 먼저 여쭤보고 싶었습니다!!
2단계 리뷰도 잘 부탁드립니다! 더 질문드리고 싶은 부분이 있다면 커밋 내역에 코멘트로 남겨두겠습니다.
감사합니다 :)