Skip to content
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

레이싱게임 #19

Open
wants to merge 23 commits into
base: master
Choose a base branch
from
Open

레이싱게임 #19

wants to merge 23 commits into from

Conversation

csbsjy
Copy link

@csbsjy csbsjy commented May 2, 2020

equals랑 hashcode 때문에 자코코 실패하는 것입니다.

csbsjy and others added 22 commits March 18, 2020 11:51
- Scanner Test TestCoverage 높이게 수정
- void 타입 메서드..
1. RacingGameServiceGenerator interface 리턴하도록
2. InputViewTest는 진짜 의미없어보인다.
3. Scanner가 들어가니 테스트를 못하겠다.
1. Car 생성 로직 RacingCars로 이동
2. 구현체 생성 조금 더 고민 ,,
- Scanner Test TestCoverage 높이게 수정
- void 타입 메서드..
1. RacingGameServiceGenerator interface 리턴하도록
2. InputViewTest는 진짜 의미없어보인다.
3. Scanner가 들어가니 테스트를 못하겠다.
1. Car 생성 로직 RacingCars로 이동
2. 구현체 생성 조금 더 고민 ,,
2. 일급컬렉션을 Service가 물고있으니 getRacingCars().getCars() 이런것들이 너무 지저분해보인다
3. DefaultEngine 테스트의 필요성 ?
…eyeon

# Conflicts:
#	src/main/java/racing/RacingController.java
#	src/main/java/racing/domain/Car.java
#	src/main/java/racing/domain/RacingCars.java
#	src/main/java/racing/service/RacingGameService.java
#	src/main/java/racing/view/InputView.java
#	src/main/java/racing/view/OutputView.java
#	src/test/java/racing/domain/CarTest.java
#	src/test/java/racing/view/InputViewTest.java
TODO: 리팩토링, 테스트코드
@csbsjy csbsjy requested a review from chldbtjd2272 May 2, 2020 12:13
@@ -0,0 +1,5 @@
package racing.support;

public interface Engine {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Engine과 관련된 클래스들은 다 도메인 영역으로 옮기는게 좋아보여
도메인 비지니스에 핵심로직이니 util,support 패키지 보단 도메인 영역이 맞아

@@ -0,0 +1,30 @@
package racing.dto;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

지금 나눈 dto와 vo의 차이가 뭐야?
또 RacingGameResult라는 dto를 도메인 클래스가 멤버변수로 들고있는 구조가 이상해보이고, 지금 정의한 dto,vo전부 도메인 영역으로 표현하는게 좋아보여

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dto 는 입력, 출력할 때 사용되는 객체고 vo 는 도메인(경기가 끝난 레이싱카)을 그대로 반환하는 것을 막기 위해 Snapshot 클래스를 만든건데 이상한가 ,,? Dto를 도메인이 물고있는 구조가 이상한건 동의해! 이건 더 생각해서 고쳐볼겝 !

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

경기 결과를 받는 클라이언트한테 도메인을 노출시키고 싶지 않은데 vo를 도메인 영역으로 표현하면 그게 안될거같거든 ㅠ^ㅠ 머르게따 ,,

private final RacingGameResult racingGameResult;

public RacingGame(RacingGameInfo racingGameInfo) {
this(new RacingCars(racingGameInfo), racingGameInfo, new RacingGameResult(racingGameInfo));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이렇게 코드를 짜는 장점이뭐야?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아 이거 디디디 강의때 모든 생성을 주생성자에게 위임하는방식을 봐서 한번 적용해봤던거얌

import java.util.List;

public class RacingGameResult {
private final int totalGameRound;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

게임결과 클래스에 총 게임 라운드가 필요없어보여,
RacingGame클래스에 게임정보클래스로 게임판수를 들고있는데 굳이 여기에 값을 복사해서 valid를 체크하는 것 보단 RacingGame에서 체크하는게 좋을거같아

public String getName() {
return name;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

사용하지 않는 toString,equals,hashCode는 재정의하지 않아도돼

@csbsjy csbsjy changed the title 문자열계산기 레이싱게임 May 11, 2020
}

@Override
public boolean equals(Object o) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

테스트에서 equals로 비교하기 위해 실코드의 equals를 재정의하는건 너무이상한거같아
실 코드에서 equals를 다른 방식으로 사용하기위해 구현하게 된다면, 밑에 테스트코드는 원하는 대로 동작할까?
equals,hasCode가 실코드에서 중요한 코드로 사용될 가능성이 높은데 테스트 코드용으로 재구현하는건 너무 위험한 생각인거같아


private final RacingCars racingCars;
private final RacingGameInfo racingGameInfo;
private final RacingGameResult racingGameResult;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 RacingGameResult라는 객체는 record 라는 메서드 때문에 가변일 수 밖에 없는 객체인데, RacingGameResult를 멤버 필드로 강한 참조를 해버려서 RacingGame도 가변객체가 된다는 느낌이 있네..!

나라면 raceWith를 void가 아닌 RacingGameResult로 낮추어서 조금 관계를 약하게 만들 것 같아

this.carSnapshots = carSnapshots;
}

public List<CarSnapshot> getCarSnapshots() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CarSnapshot이 불변이긴 하지만, 이 list가 밖으로 꺼내져서 remove 등의 가능성이 있으니, 나라면 테스트코드나 범용적으로 필요한 메서드들은 getCarSnapshotsSize() 와 같이 만들어서 막을 것 같아.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아래 테스트 코드에 getCarSnapshots().size() 이런게 있길래..!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아근데 outputView에서 iterator로 풀어서 print하는구나 흐음...고민이군

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

범용적으로 필요할 것 같은 메서드(Collection 인터페이스에서 제공하는 느낌의 너가 말한대로 size같은 ,,?)는 현재는 테스트코드에서만 써도 만들어도 괜찮다고 생각행? 이부분 늘 고민이얌!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TDD에서도 물어봐야겠다,,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants