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단계 - 사다리 게임 실행] 테오(최우성) 미션 제출합니다. #194

Merged
merged 27 commits into from
Mar 1, 2023

Conversation

woosung1223
Copy link
Member

안녕하세요 서브웨이 ~ 🥙

2단계 기능 구현 완료했습니다!

다만 몇 가지 고민사항이 있었는데요, 이 부분은 서브웨이의 의견을 듣고 싶어요.

  1. 설계에 대한 고민입니다. 좋은 구조에 기능을 의존시키는 것이 객체지향의 핵심이라고 알고 있습니다.
    다만, 기능을 구현하다 보니 구조가 기능을 따르게 된 것 같습니다. Players는 일급 컬렉션의 위치에서 벗어났고, Player는 역할이 거의 없습니다. 하지만 이를 쉽게 바꾸지 못했는데요, 기능을 바꾸는 것이 아니라 구조를 바꾸는 것은 처음부터 끝까지 대규모 변경이 필요겠더라구요 ㅠ
    어떻게 보면 초반 설계 문제일까? 싶기도 합니다. 서브웨이는 어떻게 설계하시나요? 설계는 경험의 영역일까요?

  2. 1단계에서 저희가 이야기 나눴던 예외처리 문제를 해결하기 위한 방법을 코드로 작성해봤습니다.
    개발자에게 보여져야 하는 예외말고, 사용자에게 보여져야 하는 예외는 이렇게 다루면 좋을까 싶은데, 서브웨이는 어떻게 생각하는지 궁금해요!


다른 고칠 부분도 보이는대로 말씀해주시면 감사하겠습니다! ☺️
좋은 설계를 하고 싶은데, 아쉬운 부분이 참 많은 것 같아요.. 서브웨이의 따끔한 일침 기다릴게요!!

Copy link

@joseph415 joseph415 left a comment

Choose a reason for hiding this comment

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

안녕하세요 테오! 전체적으로 재설계해서 구현 잘해주셨네요ㅎㅎ!💯
리뷰가 많이 늦었습니다!..🥲 답변은 추가로 코멘트로 달아두겠습니다!
리뷰 확인 부탁드려요!

src/test/java/ladder/domain/player/PlayersTest.java Outdated Show resolved Hide resolved
src/test/java/ladder/domain/ladder/LineTest.java Outdated Show resolved Hide resolved
import ladder.domain.ladder.Ladder;
import ladder.domain.ladder.Line;
import ladder.domain.player.Player;
Copy link

@joseph415 joseph415 Feb 24, 2023

Choose a reason for hiding this comment

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

현재 Domain를 의존해서 View를 생성하고 있는데요. 도메인을 파라미터로 받아서 바로 view에 보여질 데이터로 바로 정제해서 보여주기에 코드적으로 깔끔하게 유지 할 수는 있을 것 같습니다.
지금같은 간단한 코드에서는 도메인을 넘겨줘서 view에서 처리해주는것이 코드가 깔끔해지고, 불필요한 작업이 없어서 좋아보일 수 있습니다.

하지만, view로 도메인이 객체가 나가게 되면 이전에 사용하던 데이터 때문에, 상위호환성을 지켜야해서 비지니스 요구사항이 변경되어도 도메인영역을 변경해야할 때, 쉽게 할 수가 없게됩니다. 저희가 계속 이야기 나눴던 비지니스영역이 외부로 노출되지 않게 해야하는 이유입니다!

만약에, 도기가 이런 내용을 충분히 잘 이해했다면, 따로 변경을 요청드리지는 않을게요~ 하지만, 잘 이 말이 와닿지 않으면, 도메인이 View로 노출되지 않도록 해보시고, 느껴보시는 것도 좋을 것 같아요!


다른 크루에게도 같은 리뷰를 남겼는데 잘 와닿지 않는다하여서 쫌 더 추가적으로 예시를 들어볼게요!

만약에 View가 실제 클라이언트에게 노출되는 영역이라고 할 때, 저희가 만드는 서비스가 굉장히 인기가 많아서 100만명의 클라이언트가 생겼다고 가정해볼게요!

근데 마침, 복잡한 비지니스 기획이 추가돼서 도메인을 변경해야할 상황이 생기면, 쉽게 변경이 가능할까요? 도메인 필드를 하나 삭제해야한다면 100만영의 클라이언트 모두가 영향을 받을 수 있습니다. 따라서 해당 필드는 영원히 지울 수 없게 될 수도 있어요.
즉, 하위호환성을 지켜야 해서 쉽게 변경이 불가능해집니다.

그래서 도메인영역이 외부로 노출되지 않도록 경계를 나누는 것입니다. 도메인은 비지니스 영역이기 때문에 변경에 유연하게 대응이 가능해야합니다. 그래야 기능을 추가하고 수정하는데 제약이 없게될거에요!

제가 저번에 1단계때, 익셉션의 메시지가 바뀐다고 도메인이 영향받는다는 이야기는 다른차원인것 같다고 한것도 위의 이유와 비슷합니다. 보통 에러가 발생하면, 에러 전용 데이터모델을 만들어서 클라이언트에 전송하게 됩니다!

Copy link
Member Author

Choose a reason for hiding this comment

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

(저는 도기가 아니라 테오에요 😭)

그렇다면 서브웨이는 DTO 사용을 권장하시는걸까요?
확실히 컴포넌트 간 통신규약을 정의한다는 점에서는 좋아보입니다.

하지만 DTO를 사용하더라도, 결국 DTO 안에 존재하는 것은 도메인 객체이며 뷰는 도메인에 의존적일 수 밖에 없지 않나요?!

이 문제를 해결하려면 결국 도메인 - 뷰 간의 의존성을 최소로 끌어내리고,

  1. 도메인의 변경이 뷰에 영향을 주지 않으며
  2. 뷰의 변경이 도메인에 영향을 주지 않는다.

이 두 가지 조건을 만족시키는 방법이 잘 떠오르지 않는 것 같습니다.
그나마 좋은 대안이 DTO인 것 같은데, DTO를 사용한다고 해서 하위호환성을 지키게 될 것 같다는 느낌이 들지는 않네요 😢

도메인 정보를 뷰에게 필요한 데이터 모델로 변환해주는새로운 Layer의 도입이 필요한걸까요?

Copy link

@joseph415 joseph415 Feb 25, 2023

Choose a reason for hiding this comment

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

앗!.. 제 현재 다른 리뷰이가 도기인데, 둘이 같은 내용의 리뷰를 남기느라 내용참고를 하다 도기라고 써버렸네요😮

dto를 사용하지 않더라도, controller에서 화면에 맞게 값을 정제해서 view로 전달 가능하다고 생각해요. dto 사용을 권장한다는 리뷰는 아니였습니다.

view --> controller --> domain 이다보니, view는 controller에서 내려주는 값에 의존을 하게 되는것이 아닐까요? 그렇기 떄문에, 이 둘간의 강결합이 없어진다고 생각합니다.

view ---> controller --> domain
(컨트롤러에서 전달해준 값 노출 --> string으로 정제 --> Ladder)

Copy link
Member Author

@woosung1223 woosung1223 Feb 27, 2023

Choose a reason for hiding this comment

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

서브웨이 바쁘실텐데 좋은 코멘트 감사해요!

제가 추가적으로 궁금한 것은,

컨트롤러가 도메인을 통해 String을 View에게 반환하게 되면 View의 책임은 거의 없어지는 것이 아닐까 생각이 듭니다.

저는 View의 책임이

  1. 도메인 객체를 통해 원하는 정보를 뽑아내서 가공
  2. 가공된 정보를 출력

두 가지라고 생각했는데, 컨트롤러가 1번 책임을 가지게 된다면 View의 존재의의가 희미해지는 것이 아닌가.. 생각이 듭니다.

그리고 컨트롤러가 도메인 객체들을 통해 어떤 정보를 String으로 보낼지를 결정하게 되려면 View를 알아야 하는 것이 아닌가요?!

이에 대해 어떻게 생각하시는지 궁금해요!

Copy link

@joseph415 joseph415 Feb 27, 2023

Choose a reason for hiding this comment

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

앗 요거 답변이 늦었네요. 메일 알람을 놓쳤습니다!..

View의 책임은 서버에서 내려온 데이터를 사용해 사용자에게 보여주는 것이 View의 책임 아닌가요~?
위에서 제가 보여드린 의존성 방향은 예시를 보여드리려고 한방향으로만 그렸는데,

view ---> controller --> domain
     <---

입니다!

따라서,

  1. 컨트롤러에서 1번처럼 데이터를 완전하게 가공하여서 그대로 View로 전달해서 보여주는 방향
  2. 컨트롤러에서 도메인 객체 속성을 getter로 꺼내 View로 전달하여 View에서 정제를 하는 방향
    으로 가야합니다.

도메인 객체를 View로 그대로 넘겨서 처리하는 것은, 맨 위에서 말씀드린 예시와 같은 상황 때문에 피해야합니다.
그리고, 컨트롤러에서 전부 정제하고 View에서 그대로 보여주는 형태도 나쁜 형태는 아닙니다!ㅎㅎ 어떤 상황에서는 이렇게 개발해야할때도 있습니다!

Copy link
Member Author

@woosung1223 woosung1223 Feb 28, 2023

Choose a reason for hiding this comment

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

감사합니다 서브웨이!!

서브웨이 말씀대로 제 방식대로라면 다음과 같은 의존성 흐름이라고 생각되네요.

view ---> controller ---> domain

    |                          ↑ 
    ---------------------------

말씀 들어보니 뷰에서는 도메인을 다뤄서는 안 된다고 생각하게 되었어요!
그것이 MVC의 기본 모델이라고 생각하기도 하구요!

그렇다면 View에서 도메인 모델을 직접 다뤄야 하는 상황도 있을까요?

실제 서비스를 생각해보면 그런 경우는 없을 것 같긴 한데, 제가 웹 분야는 거의 몰라서 그런 경우도 있는지가 궁금해요!!!

Copy link

@joseph415 joseph415 Mar 1, 2023

Choose a reason for hiding this comment

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

웹서비스 뿐아니라 앱서비스든 도메인을 그대로 내려서 하는 경우는 없을 것 같아요! 그렇게 하게됐을 때, 생기는 단점이 너무 많은것같아요~! 아직 교육 초반이라 이 말이 잘 와닿지 않을 수 있겠지만, 계속 학습하고 경험하시다보면 이해하실거라 생각합니다!

@@ -0,0 +1,34 @@
package ladder.view;
Copy link

@joseph415 joseph415 Feb 24, 2023

Choose a reason for hiding this comment

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

MessageSelector가 Exception을 담당하는 Controller 역할을 하도록 하는게 어떤가요?!

Exception을 받아서 어떤 메시지를 노출할지 고르는 역할을 하게 되는데, 익셉션을 담당하는 컨트롤러의 역할이 맞지 않을까해서요ㅎ

Copy link
Member Author

Choose a reason for hiding this comment

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

저는 MessageSelector가 특정 View에게 종속적이어야 한다고 판단해서 위처럼 분류했습니다.

역할 측면에서는 컨트롤러의 성향이 강하긴 한데, 뷰마다 컨트롤러를 생성하는 것이 좋은 방법인지 고민이 되네요.

저는 특정 View + MessageSelector가 한 묶음이라고 생각했는데, 서브웨이 생각은 어떠신가요!
컨트롤러를 적용할 수 있는 범위를 어디까지라고 생각하시는지 궁금해요

Choose a reason for hiding this comment

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

테오가 생각처럼 간다면, 그것도 맞는것 같은데 제가 추구했던 방향이 아래의 방향이였습니다.

MessageSelector 가 ExceptionController 역할을 하고 에러 응답을 담당하는 컨트롤러가 되는 것이요.

그러면, view에서는 에러 응답을 받아서 처리하면 되고, 서버에서 view로 적절한 값을 내려줄 수 있고요. 그러면, 사용자에게 나가는 응답이 변경될 때, ExceptionController에서 변경해주면 되니 더 좋은 형태로 가지 않을까 했습니다.

아래 제가 답변을 달아둔 것처럼 외부로 나가는 ExceptionController과 내부 디버깅용 ExceptionController 을 만들면 뷰마다 컨트롤러가 만들어지는 것은 아닌것 같은 생각이 듭니다!

Copy link
Member Author

Choose a reason for hiding this comment

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

감사합니다.

사실 예외처리 메세지 출력도 공통적인 부분으로 뽑아낼 수 있고,
이것이 서브웨이가 말씀하신 ExceptionController가 될 것 같네요.

일단 말씀해주신 방식이 더 좋은 방식이라 생각되어, 그 방향으로 리팩토링을 한번 해볼게요!
이후에 코드 봐주시고 짚어주시면 감사하겠습니다 :)

src/main/java/ladder/view/MessageSelector.java Outdated Show resolved Hide resolved
@@ -1,6 +1,6 @@
package ladder.view;

import ladder.exception.NotIntegerException;
import ladder.domain.exception.NotIntegerException;
Copy link

@joseph415 joseph415 Feb 24, 2023

Choose a reason for hiding this comment

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

NotIntegerException 도 도메인익셉션 인가요?! View에서 담당하는 validate가 도메인과 관련되어야 할까요?
그리고 이미 자바에서 만들어준 IllegalArgumentException, NumberFormatException등으로 해당 Exception을 표현할 수 있을 것 같습니다!

Copy link
Member Author

Choose a reason for hiding this comment

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

감사합니다!

제가 예외마다 메세지를 매핑하기 위해 + 도메인 집약적인 표현을 하기 위해 커스텀 예외를 사용하고 있는데
어떤 예외는 커스템 예외, 어떤 예외는 표준예외를 사용한다는 점이 조금 부자연스럽다고 느꼈었네요.

혹시 실무에서도 커스텀 예외 + 표준 예외를 같이 사용하는지, 아니면 하나만 사용하는지 궁금해요!!

Choose a reason for hiding this comment

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

실무에서는 통용해서 사용하는 곳도 있을 거고, 아예 커스텀 exception을 사용하지 않는 곳도 있을 거에요ㅎㅎ 그런데 아마 우린 커스텀만 써야한다! 라는 곳은 많이 없지 않을까 생각합니다~

보통 예외전환은 내부에서 발생한 예외를 그대로 던지는 것이 그 예외상황에 대한 적절한 의미를 부여해주지 못하는 경우에, 의미를 분명하게 해줄 수 있는 예외로 바꿔주기 위해서 사용합니다~

Copy link
Member Author

Choose a reason for hiding this comment

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

감사합니다 서브웨이~

제가 커스텀 예외로 모든 예외를 집합시킨 이유는, catch 블록이 늘어나는 것이 두려웠기(?) 때문인데요.

사실 현재 요구사항에서 제가 정의한 모든 예외는(NotIntegerException 포함) 사용자에게 재입력을 받기 위해 존재합니다.

예외마다 처리가 달라지는 경우는 조금 세분화했을 것 같은데, 그게 아니어서 모두 CustomException의 자식 클래스로 바꿔
catch를 한번에 하게 한 것 같아요.

그리고 보통 예외전환은 내부에서 발생한 예외를 그대로 던지는 것이 그 예외상황에 대한 적절한 의미를 부여해주지 못하는 경우에, 의미를 분명하게 해줄 수 있는 예외로 바꿔주기 위해서 사용합니다 부분이 참 와닿네요.

도메인적인 예외는 CustomException 산하로, 뷰적인 예외는 RuntimeException 산하로 한번 처리해볼게요!

}

public List<Block> getBlocks() {
return List.copyOf(blocks);
public int goDownAndGetLocation(final int currentLocation) {

Choose a reason for hiding this comment

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

함수 이름에서 2개의 역할을 하도록 네이밍이 되어있는데, 하나의 역할을 하도록 만드는 것이 좋지 않을까요~?

gameRecords.put(players.get(i), gameRecord.get(i));
}
}

Copy link
Member Author

Choose a reason for hiding this comment

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

recordGameResult 메소드가 일종의 setter 느낌이 아닌가 싶어서 우려가 되는데, 서브웨이 생각은 어떤지 궁금해요!

Copy link

@joseph415 joseph415 Feb 25, 2023

Choose a reason for hiding this comment

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

저도 한때 결론적으로 setter 역할을 하는 메서드들은 뭐지?.. 분명 세터는 죄악이라고 들었는데?! 라는 고민을 많이했는데요.

제가 깨닫고 내린 결론은 무의미한 setter를 사용하지 않는 것이였어요. 외부에 setter를 그냥 개방해버리면, 객체의 프로퍼티를 객체가 아니라 외부에서 아무 의미 없이 변경하게 되버리는 상황이 발생할 가능성이 있습니다. 따라서 객체 내부에 비지니스 로직이 생기는게 아닌, Controller, Service에서 비지니스 로직이 생기고, 중복되는 코드가 생겨나게 됩니다.

그런데 테오가 네이밍 한것처럼 recordGameResult 는 어떤 도메인적 행위를 갖고 있습니다. 따라서 결국에는 setter이지만, 도메인 내부에서 비지니스적인 의미를 나타낸다고 말 할 수 있을것 같아요

나중에, db를 연결하고 엔티티의 속성 하나를 업데이트 하는 상황이 생기면 updatePlayer(Player player) 처럼 정말 세터랑 다름없는 메서드를 만들어야하는 상황이 생깁니다.
하지만, 이도 마찬가지로 결과적으로는 세터지만, 도메인적 표현, 행위를 넣는다는 의미가 있기에 이렇게 사용한다고 말 할 수 있을 것 같아요.

Copy link
Member Author

Choose a reason for hiding this comment

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

답변 감사합니다 서브웨이 ~!

setter 사용을 해서는 안된다. 라는 원칙에 매몰되어서 비즈니스 로직의 흐름을 잊어버렸었네요.

setter 사용이 죄악인 것은, 캡슐화를 저해시키고 자율적이지 않은 객체들을 만들기 때문임을 잊고 있었네요

좋은 인사이트 감사합니다!!

import java.util.List;
import java.util.Map;
import java.util.stream.Collectors;

public class Players {

Copy link
Member Author

Choose a reason for hiding this comment

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

객체 분리가 잘 되었는지도 궁금합니다! Player에 관한 정보는 Players가 담고 있기에, 게임 결과를 기록하는 책임을 Players에게 부여했는데, 올바르게 분리된걸까요?

상태와 행위를 한 곳에서 관리한다는 관점에서는 옳지만, 과연 Players라는 이름의 객체가 게임 결과 기록의 책임이 있어야 하는지를 따져보면 아닌 것 같기도 합니다 😢

Choose a reason for hiding this comment

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

저는 리뷰를 하면서 전혀 이상한 점을 못느꼈습니다.

Players 를 이름객체가 아닌 player들을 종합적으로 관리하는 객체라고 생각했어요ㅎㅎ 그렇게 생각하니 플레이어들의 기록들을 가지고 있을 수 있겠다 생각헀습니다. 합리적이라고 생각했어요.

추가적으로, 일급컬렉션을 사용하는것이 요구사항이니 Player들이 자신의 결과를 갖고 Players 에서 메서드로 결과를 내려주는것도 좋을 것 같네요. (가볍게 생각한거라, 실제 구현이 가능한 상황인지는 파악하지 않았습니다!)

@joseph415
Copy link

joseph415 commented Feb 25, 2023

1번질문

저도 설계하는 부분에 있어서는 항상 어려운 것 같아요. 처음부터 완벽한 설계는 할 수 없다고 생각해요. 완벽한 설계를 하려한다면 많은 정신적 고통이 찾아오더라고요ㅋㅋㅋ 좋은 설계를 하기 위해서는 경험적인 요소, 도메인지식등이 많이 필요하다고 생각합니다.

테오가 지금 겪는 부분이 당연한 과정이라고 생각합니다. 그리고 저는 그런 고민하는 과정들이 중요하다고 생각해요! 지금 했던 고민들이 추후에 설계나 사고하는데 있어서 좋은 밑거름이 될거에요! 이런 이유에서 꾸준한 리펙토링을 해야하고, 코드가 죽지 않도록 개선 해나가야 한다고 생각해요.
완전하고 완벽한 설계는 할 수 없기에 개발자들은 변화하는 상황에 계속 대처해나가야 하죠ㅎㅎ (현재 저도 사내에서 이런 작업을 하고있어요ㅎㅎ)

한번 이번 과제가 끝나고서나, 방학때 테오가 더 좋은 설계로 리펙토링 해보는 과정을 해보는것도 좋은 경험이라고 생각합니다. 과제를 진행하면서 도메인적 지식이 많이 올라갔다고 생각합니다. 따라서 어떤 기능을 추상화 해야 좋을지, 어떤 객체끼리 응집시켜야할지 등,, 더 좋은 설계가 나올 수 있을 거에요! 하지만, 달리는 기차의 바퀴를 갈아끼우는 일이 쉬운일이 아니니기에 리펙토링을 하는 것이 쉽지는 않을 거에요ㅎㅎ


2번질문

저는 테오가 MessageSelector 를 만든 부분 정말 인상깊게 봤습니다!.. 저는 저렇게 생각 못했을 것 같은데ㅎㅎ 저도 배워가는것 같네요!

테오가 만든 MessageSelector를 view로 전용 controller, 디버깅용 controller로 분리해서, 사용자에게 보여질 에러 메시지와, 내부에서 디버깅을 위해 보여지는 에러메시지를 분리해서 관리하면 좋을 것 같아요.

추가적으로, 저번에 1단계때 언어별 메시지를 어떻게 관리하나? 에 대한 주제로 조금 이야기를 나눴잖아요~ 보통 다국어 지원을 위해서 자바의 Property를 이용할 수 있습니다. Property에 key, value 값을 선언하고, 코드상에서 사용할 때는 프로퍼티를 가져와 키 값만 적용해주면 국가별 언어를 다르게 지원할 수 있을 거에요! 요거는 참고만해주세요ㅎㅎ

Copy link

@joseph415 joseph415 left a comment

Choose a reason for hiding this comment

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

안녕하세요. 테오!

2단계 미션 잘 구현해주셨네요ㅎㅎ
지금까지 주고받은 피드백 내용들이 잘 녹아든거같습니다! 💯

사다리미션은 이만 merge하겠습니다! 고생 많으셨어요.
블랙잭 미션도 화이팅입니다~!

@joseph415 joseph415 merged commit 076721b into woowacourse:woosung1223 Mar 1, 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