-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Step5 - 리펙토링 #6073
Step5 - 리펙토링 #6073
Conversation
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.
수고 많으셨습니다. 👍
사실 지금 구현 사항으로도 현 스텝에선 충분하나, 진도가 빠른 편이시기도 하고,
앞으로의 스텝 구현에 도움이 될 수 있는 학습 과제 제시 차원에서 몇 가지 개선 사항 남겨드렸습니다.
확인 및 반영 부탁드릴게요.
src/main/java/race/CarList.java
Outdated
public class CarList { | ||
private final Random random = new Random(); |
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.
이 클래스의 역할과 책임이 너무 비대한 것 같습니다. 우선 CarList
는 일급 콜렉션임을 염두에 두고 개발 하셨나요?
그렇다면 random
변수는 조금 과하다는 생각이 듭니다. 아울러, 제가 지난 스텝에서도 한번 말씀드린 부분이기는 한데, 인스턴스 변수에 직접 new
를 호출해 값을 할당함으로써 테스트 가능성이 극적으로 줄어든 것 같습니다.
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.
다음과 같이 moveWithRandom 에서 random을 주입 받도록 수정했습니다.
public void moveWithRandom(Random random) {
for (RacingCar car : cars) {
car.moveWithSeed(random.nextInt());
}
}
src/main/java/race/CarList.java
Outdated
Position maxPosition = new Position(); | ||
for (RacingCar car : cars) { | ||
maxPosition = car.getMaxPosition(maxPosition); | ||
} |
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.
List
의 stream API로 개선 시도도 해보시면 좋겠습니다.Position maxPosition = new Position();
은 없앨 수 있을 것 같습니다. 이것을 없애는 것을 목표로 stream API 사용을 시도해봐 주세요.
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.
comparable 을 구현하여 아래와 같이 sort 를 적용해보았습니다.
코멘트 주신 것 처럼 CarList에서는 우승한 RacingCar 객체만 신경쓰는게 더 적절한 것 같습니다.
cars.sort(Collections.reverseOrder());
RacingCar winner = cars.get(0);
src/main/java/race/CarList.java
Outdated
List<RacingCar> winners = new ArrayList<>(); | ||
|
||
for (RacingCar car : cars) { | ||
if (car.isSamePosition(maxPosition)) winners.add(car); | ||
} | ||
return winners; |
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.
여기도 stream API 로 개선 시도해주세요.
src/main/java/race/Position.java
Outdated
public Position getMax(Position comparePosition) { | ||
if (this.value > comparePosition.getValue()) { | ||
return this; | ||
} | ||
return comparePosition; | ||
} |
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.
이런 역할을 할 수 있는 java.lang.Object
의 좋은 메소드 가 있습니다.
이것을 오버라이드 하시는 것이 좋겠습니다.
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.
아래와 같이 compareTo 오버라이드 하였습니다
@Override
public int compareTo(Position o) {
return Integer.compare(this.value, o.value);
}
아래 기준에 맞춰서 코드 리팩토링을 진행했습니다.
모든 원시값과 문자열을 포장한다
: RacingCar에서 사용하던 String name, int position 수정
3개 이상의 인스턴스 변수를 가진 클래스를 쓰지 않는다.
: RacingCar의 name, position, randomGenerator, positionPrinter 총 4개의 변수를 name, position 2개로 수정
한 줄에 점을 하나만 찍는다
:
일급 콜렉션을 쓴다
: RacingTrack 에 있던 List cars 을 CarList 객체로 추출
도메인 객체를 테스트가 가능하도록 수정
: RacingCar, RacingTrack 테스트가 수월하도록 RacingCar의 생성자에 position 추가 및 CarList에 getWinners 추가