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단계 - 사다리 게임 실행] 허브(방대의) 미션 제출합니다. #234

Merged
merged 71 commits into from
Feb 27, 2023

Conversation

greeng00se
Copy link
Member

@greeng00se greeng00se commented Feb 24, 2023

안녕하세요 터틀! 🐢 오랜만입니다~ 잘 지내셨나요?
이전 코드를 꼼꼼하게 봐주셔서 감사합니다!

2단계에서는 크루들과 어떻게 하면 좋은 방법으로 작성할 수 있을지 고민하는 시간을 오랫동안 가져서 제출이 조금 늦어졌습니다. 😢

사다리 게임을 잘 구현하기 위해 다양한 방법을 생각을 해보았는데요~
크게 3가지 방법을 생각해 보았습니다.

  1. 인덱스를 활용한 방법
  2. LadderGame에서 Position(사실상 인덱스)을 기준으로 사다리를 타는 방법
  3. Player에게 Ladder를 넘겨서 ladder에게 position을 넘겨주며 메시지를 보내는 방법

이중 2, 3번을 직접 구현하여 어떤 점이 안 좋았는지 직접 체감을 해보았는데요~

LadderGame에서 Position을 기준으로 사다리를 타는 방법

링크

graph TD

    LadderGameController --> LadderGame
    LadderGame --> Ladder
    LadderGame --> Players
    LadderGame --> Items

    Ladder --> Line
    Line --> LineStatus

    LadderGame --> Position
    Ladder --> Position
    Items --> Position
    Line --> Position
    Players --> Position

    LadderGame --> LadderGameResult

    Items --> Item
    Players --> Player

    LadderGameController --> InputView
    LadderGameController --> OutputView
Loading
public LadderGameResult play() {
    final Map<Player, Item> result = new LinkedHashMap<>();
    for (Position position : Position.range(players.count())) {
        final Position resultPosition = ladder.play(position);
        result.put(players.get(position), items.get(resultPosition));
    }
    return new LadderGameResult(result);
}
  1. Position에 대한 의존도가 너무 높다고 느껴졌습니다.
  2. 인덱스를 사용하는 방법과 별반 다르지 않은 것 같다고 느꼈습니다.
  3. Players의 책임이 너무 없는 느낌이 들었습니다.(협력하는 느낌이 들지 않았습니다.)

따라서 초기에 생각했던 아래와 같은 구조로 변경해 보았습니다.

Player에게 Ladder를 넘겨서 ladder에게 Position을 넘겨주며 메시지를 보내는 방법

graph TD

    LadderGameController --> LadderGame
    LadderGame --> Ladder
    LadderGame --> Players
    LadderGame --> Items

    Ladder --> Line
    Line --> LineStatus
    Line --> Position

    Players --> Ladder
    Player --> Ladder

    Item --> Position
    Player --> Position


    LadderGame --> LadderGameResult

    Items --> Item --> ItemName
    Players --> Player --> PlayerName

    LadderGameController --> InputView
    LadderGameController --> OutputView

    OutputView --> LadderMessageGenerator

Loading
public LadderGameResult play() {
    final Map<Player, Position> playResult = players.play(ladder);

    final Map<Player, Item> result = new LinkedHashMap<>();
    for (Player player : playResult.keySet()) {
        result.put(player, toItem(playResult.get(player)));
    }
    return new LadderGameResult(result);
}

Ladder에게 Player를 넘겨 Player를 움직이는 방법도 있지만 이 방법은

  1. Ladder에 대한 책임이 너무 많다고 느꼈습니다.
  2. 위에 2번 방법과 마찬가지로 Player가 긴밀하게 협력하는 느낌이 들지 않았습니다.

따라서 Player에게 Ladder를 넘기는 방법으로 구현하였습니다.

이 방법이 긴밀하게 협력하고, 사다리게임에 대한 적절한 책임을 나눠가졌다고 생각이 들었습니다.

터틀은 2단계를 진행하셨다면 어떤 구조로 사다리 게임을 구현하셨을 것 같나요? 그리고 이유도 들어보고 싶습니다!

추가로 기술적인 부분이나, 부족한 부분 말씀해주시면 바로 반영하겠습니다!

감사합니다 😄

Copy link
Member

@begaonnuri begaonnuri left a comment

Choose a reason for hiding this comment

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

안녕하세요 허브 🌱
1단계때도 요구사항을 만족하면서 코드가 아주 잘 정돈되어있었는데 2단계 역시 잘 구현해주셨네요 👍🏻
짧은 시간이었을텐데 2가지 방법으로 진행해보신점이 대단하게 느껴집니다!

몇가지 리뷰 남겼으니 확인해주세요! 질문 주신 부분은 코멘트로 남기겠습니다.

README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
src/main/java/ladder/Application.java Show resolved Hide resolved
src/main/java/ladder/Application.java Show resolved Hide resolved
src/main/java/ladder/controller/LadderGameController.java Outdated Show resolved Hide resolved
src/main/java/ladder/domain/ladder/BooleanGenerator.java Outdated Show resolved Hide resolved
src/main/java/ladder/domain/ladder/LadderGameResult.java Outdated Show resolved Hide resolved
src/main/java/ladder/domain/ladder/Position.java Outdated Show resolved Hide resolved
src/test/java/ladder/domain/item/ItemNameTest.java Outdated Show resolved Hide resolved
src/test/java/ladder/domain/item/ItemNameTest.java Outdated Show resolved Hide resolved
@begaonnuri
Copy link
Member

begaonnuri commented Feb 25, 2023

2가지 방법으로 진행해보시고 체감하신게 대단합니다 👍🏻
저 역시도 허브가 선택한 3번 방법이 좋다고 생각해요. 이야기해준것처럼 Position에 대한 의존이 너무 큰 점을 포함해 Players, Player가 Position에 대해 적게 알수록 좋다고 생각해서 이 부분의 결합도를 줄인 3번이 더 좋은 설계라고 느껴지네요!

@greeng00se
Copy link
Member Author

안녕하세요 터틀~ 🐢
주말인데도 정성스럽게 리뷰 남겨주셔서 감사합니다.

깊게 고민하고, 혼자 생각할 수 있는 리뷰들이 많아서 너무 좋았습니다!
명확한 근거 없이 코딩했던 부분이나, 부족했던 부분을 채워나갈 수 있는 소중한 시간 이었던 것 같습니다.

추가로 제가 생각했던것들이 터틀이 남겨주신 코멘트의 의도나 방향과 맞는지 확인해주실 수 있을까요?

Copy link
Member

@begaonnuri begaonnuri left a comment

Choose a reason for hiding this comment

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

안녕하세요 허브! 리뷰 잘 반영해주셨네요 👍🏻
이 정도면 머지하기에도 충분하다고 생각이 드네요!
질문해주신 부분에 대해 코멘트 답변만 한번 더 확인해주세요😊

src/main/java/ladder/Application.java Show resolved Hide resolved
src/main/java/ladder/controller/LadderGameController.java Outdated Show resolved Hide resolved
src/main/java/ladder/controller/LadderGameController.java Outdated Show resolved Hide resolved
src/main/java/ladder/domain/ladder/BooleanGenerator.java Outdated Show resolved Hide resolved
src/main/java/ladder/domain/ladder/LadderGameResult.java Outdated Show resolved Hide resolved
src/main/java/ladder/domain/ladder/Position.java Outdated Show resolved Hide resolved
src/test/java/ladder/domain/item/ItemNameTest.java Outdated Show resolved Hide resolved
@greeng00se
Copy link
Member Author

터틀 🐢 안녕하세요~ 좋은 코멘트 많이 달아주셔서 정말 감사합니다! 👍

해당 클래스를 사용하는 입장에서 고민을 많이 할 수 있는 시간이었습니다.
추가로 부족한 부분이 있다면 코멘트 달아주시면 바로 반영하겠습니다!

Copy link
Member

@begaonnuri begaonnuri left a comment

Choose a reason for hiding this comment

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

마지막까지 테스트를 꼼꼼하게 작성해주셨군요 👍🏻
생각할거리가 많은 리뷰를 드렸는데 반영하시느냐 고생하셨습니다! 이번 미션은 여기서 머지할게요! 🚀

@begaonnuri begaonnuri merged commit 40b9a10 into woowacourse:greeng00se Feb 27, 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