[2단계 - 자동차 경주 리팩토링] 검프(김태정) 미션 제출합니다. #217

Livenow14 · 2021-02-10T06:42:00Z

안녕하세요 제이! 또 만나뵙게 되었습니다. ㅎㅎ 검프입니다. 😁
우선, 저번에 리뷰 주셨던 부분들을 리펙토링 했습니다.

리펙토링 부분

중점적으로 한 부분은 ,

첫 번째,`할당 -> 값 검증`의 로직을 `복사(방어본) -> 값 검증 -> 할당`의 과정으로 변경하였습니다.

여기서 질문은!

참조 타입을 매개변수로 받을 때 의식적으로 복사를 하는 것이 좋은 것인가 하는 것입니다.

현재 제 코드에서는, 참조 변수로 들어오는 값들을 전부 복사 후 검증, 할당의 과정을 거치고 있는데, 이를 위해 아래와 같이 깊은 복사를 위한 구현 로직을 추가햇습니다.

public class Cars implements Cloneable {
    private static final int START_NUMBER = 0;
    private static final int END_NUMBER = 9;
    private static final int MINIMUM_CAR_SIZE = 2;
'''
    @Override
    protected Cars clone() {
        Cars copy = null;
        try {
            copy = (Cars) super.clone();
        } catch (CloneNotSupportedException e) {
            e.printStackTrace();
        }
        return copy;
    }
'''
}

사용의 예

public RacingGameMachine(final Cars cars, final TryCount tryCount) {
        Cars copy = cars.clone();
        validateCars(copy);
        this.cars = copy;
        this.tryCount = tryCount;
    }

제 생각은, 복사를 통해 객체의 참조를 끊어주지 않으면, 위험하지 않을까 합니다.

하지만, 복사를 위한 구현의 추가는 무엇인가 무거워 보입니다. 현재와 같이 사용하는게 괜찮은 방법인지 궁금합니다!

두 번째, 원시 값을 전부 객체로 포장했습니다.

tryCount의 예를 들겠습니다.

public class TryCount implements Number {
    private static final int ZERO = 0;

    private int value;

    public TryCount(final int value) {
        validateTryCounts(value);
        this.value = value;
    }

    private void validateTryCounts(final int value) {
        if (value <= ZERO) {
            throw new IllegalArgumentException(String.format("시도횟수는 1회 이상이어야 합니다. 현재 입력값: %d", value));
        }
    }

    public boolean reduce() {
        if (value <= ZERO) {
            return false;
        }
        this.value--;
        return true;
    }

    @Override
    public int getValue() {
        return value;
    }
}

TryCount에서 value를 관리하게 하였는데요,

여기서 질문은!

TryCount를 이렇게 쓰는게 더 좋은가, 아니면 인스턴스를 불변으로 유지(final 화)해서 관리하는게 더 좋은가 입니다.

제 생각은, TryCount의 주솟값을 사용하는 곳은 불변을, TryCount자체는 불변이 아니게해도 괜찮지 않을까? 입니다.

public class RacingGameMachine {
    private static final int MINIMUM_CAR_COUNTS = 2;

    private final Cars cars;
    private final TryCount tryCount;

현재 RacingGameMachine에서 TryCount를 불변으로 사용하고 있습니다. 비록 TryCount 자체가 불변이 아니지만, 그래도 주솟값 자체는 바뀌지 않으니 불변이라 볼 수 있다는게 제 의견입니다.!

저번에 알려 주신 내용으로 더 많은 것을 볼 수 있게 되었습니다. 정말 감사하다는말 드리고싶습니다! 😊

이번에도 많은 질문이 있네요..!

긴 글 읽어주셔서 감사합니다. ㅎㅎㅎ 많은 피드백 바랍니다.

새해복 많이 받으세요 제이🙇🏻‍♂️🙇🏻‍♂️🙇🏻‍♂️

Screen클래스가 body가 있는 공통의 메서드를 추출할 수 있거나, 공통의 멤버변수를 활용할 수 있을 때 사용하는 추상클래스의 성질과 맞지않은 것 같아, 인터페이스로 변경

getCarDtos()에서 getCars()로의 변경을 통해 domain에서 view의 결합도를 낮춤

domain -> view dto들이 view의 성향이 강하기 때문에 이동함

Dto 반환 -> names 반환 (dto 때문에 view 에 강하게 의존되있기 때문)

테스트를 위한 메소드를 굳이 정의할 필요하기 때문에, tryCounts로 테스트할 수 있는 테스트로 변경함

중괄호 convention이 맞지 않아 수정하였음

new Winners(new ArrayList<>(Arrays.asList(pobi)))) -> new Winners(Collections.singletonList(pobi)))

@DisplayName

전체 테스트에 @DisplayName 애노테이션을 적용함.

유효성 검사가 통과하지 않을때 에러만 뱉는 것이 아닌, 메세지도 출력하게 변경하였음

검증시 에러메세지 출력하게 변경함

검증 실패시 에러메시지도 함께 출력하게 변경함.

Winner클래스 검증실패시 에러메세지 출력

복사본(방어본)을 만들고 -> 검증 -> 할등의 과정을 가지게 하엿음.

검증 순서를 복사(방어) -> 검증 -> 할당의 순서로 바꿨고, Cars 객체를 검증하는 로직을 추가하였음

복사본(방어본)을 만들고 -> 검증 -> 할등의 과정을 가지게 하엿음.

입력값을 알 수 있게 변경하였음.

매개변수로 넘어온 값이 불변함을 보장하기 위해 final을 붙임

RacingGameMachine의 인스턴스 변수가 변경됨. int tryCount -> TryCount

Car의 인스턴스 변수가 변경됨. int position -> Position

step1의 리뷰를 바탕으로 리펙토링 목록을 정리하였음

int 값으로 비교하는 것이 아닌 Position 객체로 로직을 수정하게 변경하였습니다.

CarName을 통해 자동차이름에 대한 책임을 지니게함

인텔리제이 자동 정렬 기능을 사용하여 리팩토링함

dto로 감싸서 보여주기로함.

중복된 이름에 대해서 검증하는 로직을 추가함

JunHoPark93

안녕하세요 검프! 리뷰어 제이입니다.
mvc로 패키지 구조를 잘 나누어주셨습니다. 이번 단계의 핵심을 잘 구현해주셨네요~
미션의 내용을 충실히 수행한 후에 추가적으로 공부하고 적용을 시도하는 모습이 정말 멋집니다 👍
미션 요구사항은 만족하신거같아 머지하도록 하겠습니다 🙏
그리고 미션 외적으로 적용하신것들에 대해서는 따로 코멘트를 많이 남기지는 않았습니다. 하지만 미션을 빨리 마무리 해주셨기 때문에 해당 내용에 대한 피드백은 dm으로라도 전달드리도록 할게요~

JunHoPark93 · 2021-02-10T13:38:35Z

src/main/java/racing/docs/요구사항.md

 - [x] 가장 멀리 이동한 1명 이상의 우승자 명단을 탐색해 출력한다.
+
+## 리펙토링 목록 정리


피드백에 관련한 리팩토링 목록도 정리하셨네요 👍
멋집니다!

문서화를 습관화 하겠습니다. 😊

JunHoPark93 · 2021-02-10T13:45:52Z

src/main/java/racing/domain/name/Name.java

@@ -0,0 +1,5 @@
+package racing.domain.name;
+
+public interface Name {


그냥 참고차 남깁니다!

Name이라는 인터페이스를 추출하셨네요.
우리가 인터페이스가 필요하다고 생각될 때는 주로 공통된 행동들을 정의할 때 사용합니다. 현재는 이름 이라는 것은 자동차의 이름 이라는 것 하나만 사용되고 있을텐데요, 현재 시점에서 굳이 인터페이스가 사용되어야 할 필요는 없어보입니다.

인터페이스가 나쁘다는건 아니에요.
항상 무언가 필요로 할 때 등장을 하는게 좋습니다. 닭잡는데 소잡는 칼을 쓸 필요는 없으니까요.

확장성과 가독성의 부분에서 고민했습니다. Name 인터페이스가 현재 CarName을 위해 사용되고 있지만, 이후 다른 UserName등의 객체가 추가될 수 있다는 생각으로 인터페이스로 추출했던거 같아요.
하지만 제이 말씀처럼 현재 사용되지 않을 것을 "나중에 사용하겠지"라는 생각으로 추가하는 것은 불필요한 작업인 것 같습니다.
인터페이스를 제거후 가독성이 더 좋아졌습니다. ㅎㅎ

JunHoPark93 · 2021-02-11T02:17:10Z

src/main/java/racing/domain/Cars.java

-        if (cars.size() < MINIMUM_CAR_COUNTS) {
-            throw new IllegalArgumentException();
+    @Override
+    protected Cars clone() {


이부분은 질문을 하셨기에 남깁니다!
어떤 이유로 clone()을 오버라이드 하신건가요?
아마 리스트 반환에서 불변으로 반환을 하려고 하신거같은데 하단의 getCars()메서드에서 이미 그 기능을 하고있다고 생각해요!

그냥 참고로만 말씀드리면 객체를 생성자를 통해서 만드는것이 아니고 필드를 복사하며 만들기 때문에 불변을 보장하기가 어렵습니다. 이것으로 인한 버그들이 많다고알고있는데요, shallow copy를 통한 작업으로 인해 정말 의도한 대로 복사가 이루어지지 않을 수 있습니다.
추후 학습하시면서 아시게 될거에요!

현재 RacingController에서 Cars를 생성하고, 생성한 객체를 RacingGameMachine의 생성자 인자로 넘겨주고 있습니다. (Cars객체의 생성의 책임울 RacingController에서 지니고 있음) 그렇기 때문에, 생성자 인자로 넘어온 Cars객체를 복사후 → 검증 → 할당의 과정을 거치게 하기위해 Cloneable 인터페이스를 구현한 것입니다.

다시 한번 확인해보니, 깊은 복사인척을 하는 얇은 복사였던 것 같습니다. clone을 없애고, 새롭게 객체를 만들어 복사하는 형식으로 변경하였습니다. 객체는 생성자를 통해 만들어야한다 를 정확히 이해하게 되었습니다. 감사합니다.

JunHoPark93 · 2021-02-11T02:25:14Z

src/main/java/racing/view/GameScreen.java

@@ -1,11 +1,13 @@
 package racing.view;

-import racing.domain.dto.CarDto;
-import racing.domain.dto.WinnersDto;
+import racing.view.dto.CarMoveStatusDto;


dto 패키지도 domain에서 분리해주셨네요~ 👍

JunHoPark93 · 2021-02-11T02:27:12Z

src/main/java/racing/domain/name/CarName.java

+    private final String name;
+
+    public CarName(final String name) {
+        String copy = name;


이작업도 불필요한거같아요 🙏

네 확인했습니다!!

JunHoPark93 · 2021-02-11T02:31:20Z

src/main/java/racing/domain/RacingGameMachine.java

-        return cars.getCarDtos();
+    private List<CarMoveStatusDto> getCarDtos() {
+        return cars.getCars().stream()
+                .map(car -> CarMoveStatusDto.of(car))


Suggested change

.map(car -> CarMoveStatusDto.of(car))

.map(CarMoveStatusDto::of)

🙏

확인했습니다! 감사합니다.

JunHoPark93 · 2021-02-11T02:50:52Z

첫번째 질문에 대한 내용은 코멘트에도 남겨놓았지만 써야되는이유가 무엇인지 다시 한번 생각해보셨으면 좋겠습니다! 그리고 현재 코드가 정말 깊은 복사가 일어나는것인가 살펴보세요. 혹여나 이해가 되지 않는다면 dm보내주세요!

두번째 질문에 대한 것입니다. 원시값을 포장하는 시도는 정말 잘하셨습니다! 질문주신 내용은 관점의 차이인거같아요. "시도횟수"라는 것을 생각해보았을 때 이것이 비지니스상으로 굉장히 중요하고 연산또한 의도치 않은 값들이 나오지 않게 차단하는것이 중요하다고 하면 값객체로써 활용해도 될것같다는 생각입니다.
이 부분 또한 추후 미션을 진행하시면서 접하게 될 내용입니다!

혹시 질문에 대한 답이 되었을까요?
추가적으로 궁금한 것이 있다면 휴일 주말 밤늦게도 상관없으니 언제든 코멘트나 dm 보내주세요~~
검프도 새해 복 많이 받으세요 🙇‍♂️

Livenow14 · 2021-02-15T06:40:19Z

질문에 대한 답이 완벽히 된거 같습니다!! 수고많으셨습니다 감사합니다 ㅎㅎㅎ

Livenow14 added 27 commits February 6, 2021 17:30

refactor: Screen 추상클래스 인터페이스로 변경

19d9f6f

Screen클래스가 body가 있는 공통의 메서드를 추출할 수 있거나, 공통의 멤버변수를 활용할 수 있을 때 사용하는 추상클래스의 성질과 맞지않은 것 같아, 인터페이스로 변경

refactor: Cars 클래스에서 필요한 값만 반환한다

3a3c1e3

getCarDtos()에서 getCars()로의 변경을 통해 domain에서 view의 결합도를 낮춤

refactor: domain에 있던 dto 클래스 view패키지르 이동

880f835

domain -> view dto들이 view의 성향이 강하기 때문에 이동함

refactor:Winners 메소드 변경

8cafb73

Dto 반환 -> names 반환 (dto 때문에 view 에 강하게 의존되있기 때문)

refactor: 테스트를 위한 메소드 삭제

e2980c4

테스트를 위한 메소드를 굳이 정의할 필요하기 때문에, tryCounts로 테스트할 수 있는 테스트로 변경함

refactor: code convention

4bfd56e

중괄호 convention이 맞지 않아 수정하였음

refactor: Collections.singletonList() 사용

919443e

new Winners(new ArrayList<>(Arrays.asList(pobi)))) -> new Winners(Collections.singletonList(pobi)))

refactor: 테스트클래스 @DisplayName 애노테이션으로 표현

7e74c4f

전체 테스트에 @DisplayName 애노테이션을 적용함.

refactor: Cars클래스 검증 실패시, 에러메세지 출력

778c305

유효성 검사가 통과하지 않을때 에러만 뱉는 것이 아닌, 메세지도 출력하게 변경하였음

refactor: Car 클래스 검증 실패시, 에러메세지 출력

86aca54

검증시 에러메세지 출력하게 변경함

refactor: RacingGameMachine 클래스 검증 실패시, 에러메세지 출력

c4f2b1f

검증 실패시 에러메시지도 함께 출력하게 변경함.

refactor: Winner검증 실패시, 에러메세지 출력

dd2a453

Winner클래스 검증실패시 에러메세지 출력

refactor: Car 클래스 검증 순서 변경

d66a7e7

복사본(방어본)을 만들고 -> 검증 -> 할등의 과정을 가지게 하엿음.

refactor: Cars 클래스 검증 순서 변경

2a42110

복사본(방어본)을 만들고 -> 검증 -> 할등의 과정을 가지게 하엿음.

refactor: RacingGameMachine 클래스 검증 순서 변경 및 검증 로직 추가

071620f

검증 순서를 복사(방어) -> 검증 -> 할당의 순서로 바꿨고, Cars 객체를 검증하는 로직을 추가하였음

refactor: Winner클래스 검증 순서 변경

cd9d349

복사본(방어본)을 만들고 -> 검증 -> 할등의 과정을 가지게 하엿음.

refactor: RacingGameMachine 에러 출력 메세지 변경

08c318f

입력값을 알 수 있게 변경하였음.

refactor: 파라미터 부분 전체 final 처리

bc3613d

매개변수로 넘어온 값이 불변함을 보장하기 위해 final을 붙임

feat: TryCount 객체를 만들어 시도 횟수에 대한 책임을 지정함

54caae2

RacingGameMachine의 인스턴스 변수가 변경됨. int tryCount -> TryCount

refactor: Position을 만들어 위치에 대한 책임을 지정함

7d30beb

Car의 인스턴스 변수가 변경됨. int position -> Position

docs: 리펙토링 목록 정리

426921d

step1의 리뷰를 바탕으로 리펙토링 목록을 정리하였음

refacotr: dto 클래스명 변경

22370bb

refactor: int 값을 사용하던 로직을 Position 객레로 변경

c0a24aa

int 값으로 비교하는 것이 아닌 Position 객체로 로직을 수정하게 변경하였습니다.

refactor: String으로 지정된 name Name 인터페이스로 변환

5560c58

CarName을 통해 자동차이름에 대한 책임을 지니게함

refactor: code convention

2608a07

인텔리제이 자동 정렬 기능을 사용하여 리팩토링함

refactor: CarMoveStatusDto에서 사용하던 원시값 dto로 변경

15fe424

dto로 감싸서 보여주기로함.

feat: 중복된 이름 검증 로직 구현

782086b

중복된 이름에 대해서 검증하는 로직을 추가함

JunHoPark93 approved these changes Feb 11, 2021

View reviewed changes

JunHoPark93 merged commit cdce92a into woowacourse:livenow14 Feb 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[2단계 - 자동차 경주 리팩토링] 검프(김태정) 미션 제출합니다. #217

[2단계 - 자동차 경주 리팩토링] 검프(김태정) 미션 제출합니다. #217

Livenow14 commented Feb 10, 2021 •

edited by JunHoPark93

Loading

JunHoPark93 left a comment

JunHoPark93 Feb 10, 2021

Livenow14 Feb 15, 2021

JunHoPark93 Feb 10, 2021

Livenow14 Feb 15, 2021 •

edited

Loading

JunHoPark93 Feb 11, 2021 •

edited

Loading

Livenow14 Feb 15, 2021

JunHoPark93 Feb 11, 2021

JunHoPark93 Feb 11, 2021

Livenow14 Feb 15, 2021

JunHoPark93 Feb 11, 2021

Livenow14 Feb 15, 2021

JunHoPark93 commented Feb 11, 2021

Livenow14 commented Feb 15, 2021

		- [x] 가장 멀리 이동한 1명 이상의 우승자 명단을 탐색해 출력한다.

		## 리펙토링 목록 정리

		@@ -0,0 +1,5 @@
		package racing.domain.name;

		public interface Name {

	.map(car -> CarMoveStatusDto.of(car))
	.map(CarMoveStatusDto::of)

[2단계 - 자동차 경주 리팩토링] 검프(김태정) 미션 제출합니다. #217

[2단계 - 자동차 경주 리팩토링] 검프(김태정) 미션 제출합니다. #217

Conversation

Livenow14 commented Feb 10, 2021 • edited by JunHoPark93 Loading

리펙토링 부분

중점적으로 한 부분은 ,

첫 번째,할당 -> 값 검증의 로직을 복사(방어본) -> 값 검증 -> 할당의 과정으로 변경하였습니다.

여기서 질문은!

두 번째, 원시 값을 전부 객체로 포장했습니다.

여기서 질문은!

JunHoPark93 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Livenow14 Feb 15, 2021 • edited Loading

Choose a reason for hiding this comment

JunHoPark93 Feb 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JunHoPark93 commented Feb 11, 2021

Livenow14 commented Feb 15, 2021

Livenow14 commented Feb 10, 2021 •

edited by JunHoPark93

Loading

첫 번째,`할당 -> 값 검증`의 로직을 `복사(방어본) -> 값 검증 -> 할당`의 과정으로 변경하였습니다.

Livenow14 Feb 15, 2021 •

edited

Loading

JunHoPark93 Feb 11, 2021 •

edited

Loading