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

[2단계 - 자동차 경주 리팩터링] 테오(최우성) 미션 제출합니다. #590

Merged
merged 16 commits into from
Feb 16, 2023

Conversation

woosung1223
Copy link
Member

@woosung1223 woosung1223 commented Feb 12, 2023

안녕하세요 코즈!
바쁘신데 꼼꼼히 봐주셔서 감사합니다.

이미 MVC 모델을 차용하고 있어서, 저번에 코즈가 코멘트 남겨준 것을 토대로 리팩토링 해보았는데요, 이번에도 잘 부탁드립니다!

아래에는 변경사항 및 고민사항을 정리했어요.

2단계 변경사항

  1. 상수 필드를 모두 static final로 변경
  2. 우승자 검출을 위한 자동차 비교 시 getter를 전혀 사용하지 않도록 수정
  3. 도메인과 컨트롤러 영역에서 View 관련 정보에 대해 모르도록 예외 메세지를 출력하는 부분까지 OutputView에게 위임
  4. 자동차 이름, 위치 정보를 다루는 VO 생성
  5. 시도 횟수를 다루는 객체를 생성해 프로덕션 및 테스트 코드를 이해하기 쉽도록 변경

2단계 고민사항

  1. 어느정도까지 확장성있게 설계하는게 옳을까? 얼마나 확장해야 오버프로그래밍이 되지 않을까 고민입니다.
    예를 들어서, OutputView가 지금은 한국어로 콘솔에 출력하는 것이지만, 영어로 콘솔에 출력 된다거나, 콘솔이 아닌 다른 곳에 출력이 될 수도 있을 것인데요,
    그러면 OutputView 위에 부모 클래스나 Interface를 정의하는 것이 낫지 않을까? 라는 생각이 들지만 현재 문제 해결 상황에서는 전혀 필요하지 않습니다. 그래서 궁금한 것이, 오버 프로그래밍과 유연함 사이의 적정선은 어디인지 입니다.
    즉, 다형성을 얼마나 이용하는 것이 맞을까요?

  2. VO의 경우 메소드 및 프로퍼티의 네이밍은 어떻게 해야할까도 고민입니다.
    CarName 객체에서 프로퍼티 이름을 carName으로 했는데, 클래스명과 중복이 된다는 점에서 애매하다는 느낌을 받았습니다.
    또한 비슷한 이유에서 getter 이름을 getCarName이라고 하지 않고 get 이라고 정의했는데(carName.get() 처럼 사용될 수 있도록)
    괜찮은 방법인지도 궁금합니다.

그리고 저번에 남겨주셨던 도메인 영역에서 View에 어떤 메세지를 노출시켜야할지 알아야 하는 구조인 것 같습니다. 부분을 오래
고민해봤는데, 쉽게 해답을 못찾고 있어서 조금 더 고민해보고 나중에 정 안되면 질문드리겠습니다! 🙇‍♂️

Copy link

@kouz95 kouz95 left a comment

Choose a reason for hiding this comment

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

안녕하세요 테오 :)
리뷰 확인해주세요 🙏

  1. 질문 주신 것에 정답은 무엇이다 말씀드리기 어렵습니다. 하지만 좋은 고민이라고 생각합니다.
    설계라는 것은, 같은 동작을 하는 코드를 어떻게 배치할 것이냐에 대한 이야기라 생각합니다. 좋은 설계라는 것은 변화하는 것에 유연하게 대처할 수 있는, 유지보수성이 좋은 구조를 잡아나가는 것일 텐데요. 이 말인즉슨, 변하지 않는 환경이라면 설계가 좋다 나쁘다를 가름하기 어렵습니다.
    학습을 하면서 변해도 되는 것과 변하면 안 되는 것에 대해 고민해보시길 바랍니다. 변할 수도 있는 부분, 혹은 Testablity를 위해 다형성을 적용하는 것은 오버 엔지니어링이 아닐 수 있지만, 변하면 안 되는, 혹은 변하면 전체에 영향을 주는 것이 마땅한 부분에 다형성을 적용하는 것은 잘못된 설계입니다.
  2. 취향의 영역인 것 같습니다. 개인적으로 get()은 Optional의 get()이 떠올라서 오해의 소지가 있는 것 같네요.
    getter는 getMemberVariable() 로 작성하는 것이 일반적이라고 생각합니다 놀람 최소화 원칙

this.remaining = remaining;
}

public void use() {
Copy link

Choose a reason for hiding this comment

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

use() 이전에 isLeft() 검사를 해주는 것도 좋을것 같네요.
도메인 모델의 메서드엔 도메인 규칙을 어기지 않도록 신경써서 만들어주는 것이 좋습니다.


public class CarName {

private static final String CAR_NAME_LENGTH_EXCEED = "[ERROR] 자동차 이름의 길이는 1자 이상, 5자 이하여야 합니다.";
Copy link

Choose a reason for hiding this comment

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

View에 노출되는 메세지를 도메인 모델이 결정하고 있지 않나요?
다국어 View를 지원해야한다면, 그것을 오롯이 View의 책임이 되도록 할 수 있지 않을까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

해당 내용에 대해 계속 고민해보았는데, 제가 생각해본 대안은 다음과 같습니다.

  1. 예외 메세지를 다루는 enum 객체를 생성하기
  2. 커스텀 예외 생성하기

그러나 1번의 경우 응집도가 높아지고 다국어 View가 생겨도 프로퍼티만 하나 만들면 되지만,
결국 도메인 영역에서 도메인 외부를 의존하고 있는 것이 아닌가 생각이 듭니다.
private static final ExceptionMessage CAR_NAME_LENGTH_EXCEED = ExceptionMessage.CAR_NAME_LENGTH_EXCEED 처럼요.

2번의 경우 문제가 해결되기는 하지만 각각의 예외사항마다 클래스를 만들어줘야 한다는 단점이 있는 것 같습니다.
제가 보기에 두 방식 모두 문제가 있는 것 같은데, 혹시 조금만 더 방향을 짚어주실 수 있을까요?

Copy link

Choose a reason for hiding this comment

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

제가 비지니스 로직과 뷰 로직 분리의 방법으로 생각했던 방식은 말씀해주신 2번 방식에 가까울 것 같습니다. 1번은 말씀 주신 것처럼 도메인에서의 뷰 로직 의존을 끊어내는 방법이 아닙니다.
전 단점이라고 생각하지 않았는데, 매번 클래스가 생겨난다는 것을 충분히 단점으로 여길수 있다고 생각합니다 :)
클래스를 줄이는 방법을 생각해보자면, 새로 정의한 Exception에 어떤 Exception인지를 나타내는 값을 멤버 변수로 잡아주는 방법과, 예외를 발생시키는 구조가 아닌, boolean 혹은 다른 객체로 결과 값을 정의하여, 결과 값에 맞는 View를 호출하는 방법이 있을것 같습니다.

Copy link

Choose a reason for hiding this comment

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

참고로, 응집도(Cohesion)는 보통 모듈 단위로 평가되는 개념입니다. 일반적으로 응집도는 높고 결합도는 낮을 수록 좋은 구조라고 봅니다.
https://madplay.github.io/post/coupling-and-cohesion-in-software-engineering

@@ -0,0 +1,18 @@
package domain;

public class Coin {
Copy link

Choose a reason for hiding this comment

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

VO라는 용어를 사용하셔서 질문드려요 :)
Coin은 VO라고 생각하고 모델링 하신걸까요?
VO (Value Object) 이외엔 어떠한 유형의 객체 모델이 존재하나요?

Copy link
Member Author

@woosung1223 woosung1223 Feb 13, 2023

Choose a reason for hiding this comment

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

CarName이나 Position은 VO라고 생각하지만, Coin객체는 VO라고 생각하지 않았습니다!

제가 생각하는 VO의 정의는 다음과 같습니다.
본래 객체로 나타내지 않아도 될 값들을 제약조건이나 동등성 비교를 위해 객체로 다루는 것

즉, 어떤 값들을(돈, 나이, 개수 등) 원시형으로 다룰 수도 있겠지만,
일부러 객체로 구성해 의미를 부여하는 것이 Value Object의 목적이라고 생각했습니다.

Coin 객체는 remaining 이 같다고 해도 같은 객체라고 보장할 수 없고,
값을 나타내기 위해 탄생한 객체가 아니기 때문에 VO라고 생각하지 않았습니다.

또한 조사해 본 결과, VO 이외에 DTO, DAO, Entity, PO, BO, SO 등의 객체 모델이 존재하는 것 같습니다..!

Copy link

Choose a reason for hiding this comment

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

값을 나타내기 위해 탄생한 객체가 아니기 때문에 VO라고 생각하지 않았습니다.

(전 VO같아요 🙃)
이 부분에 대해선 따로 더 의견 드리진 않겠습니다 :)
용어에 대한 해석이 제 각각이고, 각자의 해석에 따라 여러 자료들이 재생산되다보니 여러 개념이 뒤섞이게 되는것 같은데요.
이 부분은 고민해보면서 테오 나름의 개념을 만들어 가셨으면 좋겠습니다.
그리고 말씀주신 유형중, VO와 같은 선상에 놓고 비교해야하는 객체는 Entity라고 생각합니다. (PO, BO, SO는 처음 들어보네요. 저도 공부해볼게요)

Copy link

@kouz95 kouz95 left a comment

Choose a reason for hiding this comment

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

안녕하세요 테오 :)
질문에 답변드렸으니 확인 부탁드리겠습니다~!

@woosung1223
Copy link
Member Author

코즈 꼼꼼한 리뷰 감사합니다~

커스텀 예외 처음 사용해봤는데 확실히 말씀하신대로 장점이 뚜렷한 것 같네요.

지금은 ConsoleView 만을 위한 한국어 메세지를 제공하고 있지만, 만약 말씀주신 것처럼 다국어를 사용하게 된다면
프로퍼티에 메세지를 정의하고, getKoreanMessage, getEnglishMessage 처럼 뷰에서 메세지를 선택하게 할 수 있을 것 같아요.

사실 예외처리에 관한 고민도 많았는데, 큰 깨달음 하나 주셔서 감사합니다! 🥺

Copy link

@kouz95 kouz95 left a comment

Choose a reason for hiding this comment

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

이번 미션에서 요구하는 수준은 충분히 만족하신 것으로 판단하여 머지합니다 :)
고생 많으셨습니다.


public class CarNameBlankException extends RuntimeException {

private static final String MESSAGE = "[ERROR] 자동차의 이름은 공백이면 안됩니다.";
Copy link

Choose a reason for hiding this comment

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

이 부분도 결국엔 도메인이 어떤 메세지를 노출시켜야할지 정하는 구조이지 않을까요?
의존의 방향으로 생각하면 현재,
도메인 모델 -> 예외 -> 메세지
인것 같은데요, 예외에 따라 View가 메세지를 처리하도록 변경이 가능해 보입니다 :)

고민해보시고 마땅한 방법이 떠오르지 않으신다면 DM 주세요

@kouz95 kouz95 merged commit 7c282e4 into woowacourse:woosung1223 Feb 16, 2023
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.

2 participants