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단계] 카피 미션 제출합니다. #355

Merged
merged 43 commits into from
Mar 4, 2024

Conversation

tkdgur0906
Copy link
Member

안녕하세요 비밥😀
2단계 미션은 구현하기에 급급해서 신경쓰지 못한 부분이 많은 것 같습니다
제가 놓친 부분이 있다면 피드백 주시면 감사하겠습니다!

Copy link

@pci2676 pci2676 left a comment

Choose a reason for hiding this comment

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

조금 더 고민해볼만한 부분에 리뷰를 남겨두었습니다.

확인 후 다시 요청 부탁드립니다.

public static void printResult(Players players, Ladder ladder) {
System.out.println(RESULT);
printPlayers(players.getPlayers());
public static void printResult(Players players, Ladder ladder, Results results) {
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에 전달된 도메인 객체에는 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.

파라미터로 Players를 넘겼을 때 View 사용하지 않는 메서드에 접근할 수 있게 되는 것을 확인하였습니다!
View로 넘기는 dto를 생성하여 필요한 최소 단위의 메서드만 호출하도록 단위를 최소화했습니다!

Copy link

Choose a reason for hiding this comment

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

LadderResultDto가 기존 Players players, Ladder ladder, Results results를 감싼 객체의 역할을 수행하는 것 같아보입니다.

새롭게 생성된 dto는 단순이 객체를 실어나르는 컨테이너의 역할만 하게되는것 같습니다.

dto에 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.

dto에 객체가 여전히 들어가있어 dto의 권한 이상의 것을 수행할 수 있을 것 같습니다!
dto에 저장할 멤버 변수를 원시값으로 수정하여 데이터를 전달하는 역할만 할 수 있도록 수정하였습니다!

Comment on lines 10 to 15
public LadderGameResult(Ladder ladder, Players players, Results results) {
ladderGameResult = new HashMap<>();
for (int i = 0; i < players.getPlayers().size(); i++) {
ladderGameResult.put(players.getPlayers().get(i), results.getResults().get(ladder.climb(i)));
}
}
Copy link

Choose a reason for hiding this comment

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

LadderGameResult의 생성을 위해 필요로하는 객체가 많은것 같습니다.

다른 방식으로 생성해보는걸 고려해보시면 좋을것 같습니다.


i 는 축약된 변수명 같습니다.

줄여 쓰지 않는다(축약 금지).


지난번에 드렸던 리뷰와 비슷합니다.

컬렉션의 인덱스에 의존하는 비즈니스 로직이 안전하다고 생각하시나요?

한번 고민해보시고 답변만 달아주셔도 좋습니다.

Copy link
Member Author

@tkdgur0906 tkdgur0906 Feb 28, 2024

Choose a reason for hiding this comment

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

  1. 생성할 때 필요한 값이 많기 때문에 빌더 패턴을 도입해 보았습니다! 빌더 패턴으로 생성하는데 필요한 값이 명확해진 장점이 있지만 필요한 코드가 늘어났다는 단점이 있는 것 같습니다.
    생성에 필요한 객체가 3개여도 빌더 패턴을 고려하는게 좋은지, 아니면 제가 잘못 접근한 건지 궁금합니다!

  2. 축약하지 않는다는 규칙을 지키지 못한 것 같습니다! 다만 for문의 변수 i는 index라는 것이 개발자들에게 통용되는 사실인데 규칙이 없다고 가정했을 때 i를 쓰시는지 궁금합니다!

  3. 컬렉션을 index로 접근한다면 내부적으로 어떻게 저장되어 있는지 알 수 없기 때문에 불안전한 부분이 있다고 생각합니다. 다만 해당 로직의 경우 컬렉션안에 있는 값을 Player의 index를 사용하여 모두 순회하여야 하는데, 이런 경우 어떻게 처리해야하는지 잘 모르겠습니다. 현재는 stream을 사용하는 방식으로 리팩토링 했는데, stream으로 바꾸는 것이 for문과 비교해서 해당 문제를 해결하지는 못하는 것 같습니다😥
    약간의 인사이트를 얻을 수 있을까요?

Copy link

Choose a reason for hiding this comment

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

생성할 때 필요한 값이 많기 때문에 빌더 패턴을 도입해 보았습니다! 빌더 패턴으로 생성하는데 필요한 값이 명확해진 장점이 있지만 필요한 코드가 늘어났다는 단점이 있는 것 같습니다.
생성에 필요한 객체가 3개여도 빌더 패턴을 고려하는게 좋은지, 아니면 제가 잘못 접근한 건지 궁금합니다!

빌더로 개선해주셨지만 이전과 크게 다른 상황으로 보이지는 않는것 같습니다. 😅
3개의 객체를 모두 하나의 객체로 넘기기 보다 객체간 상호작용을 통해 객체를 만들어내도록 변경해보는것도 하나의 방법이 될 수 있을것 같습니다.

Copy link

Choose a reason for hiding this comment

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

축약하지 않는다는 규칙을 지키지 못한 것 같습니다! 다만 for문의 변수 i는 index라는 것이 개발자들에게 통용되는 사실인데 규칙이 없다고 가정했을 때 i를 쓰시는지 궁금합니다!

강제되지 않으면 자유분방하게 사용합니다 ㅋㅋ..

다만 의미가 분명한 값이라면 i와 같은 이름으로 사용하지 않습니다.

Copy link

Choose a reason for hiding this comment

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

컬렉션을 index로 접근한다면 내부적으로 어떻게 저장되어 있는지 알 수 없기 때문에 불안전한 부분이 있다고 생각합니다. 다만 해당 로직의 경우 컬렉션안에 있는 값을 Player의 index를 사용하여 모두 순회하여야 하는데, 이런 경우 어떻게 처리해야하는지 잘 모르겠습니다. 현재는 stream을 사용하는 방식으로 리팩토링 했는데, stream으로 바꾸는 것이 for문과 비교해서 해당 문제를 해결하지는 못하는 것 같습니다😥
약간의 인사이트를 얻을 수 있을까요?

Player에게 조금 더 많은 책임을 주면 어떨까요?

Player가 자신의 위치를 아는것은 이상하다고 생각하시나요?

Copy link
Member Author

@tkdgur0906 tkdgur0906 Feb 29, 2024

Choose a reason for hiding this comment

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

Player에게 조금 더 많은 책임을 주면 어떨까요?
Player가 자신의 위치를 아는것은 이상하다고 생각하시나요?

Player가 본인의 시작 Position을 가지고 있는다는 것은 이상하지 않다고 생각합니다.
컬렉션의 인덱스에 의존하는 로직보다 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.

빌더로 개선해주셨지만 이전과 크게 다른 상황으로 보이지는 않는것 같습니다. 😅
3개의 객체를 모두 하나의 객체로 넘기기 보다 객체간 상호작용을 통해 객체를 만들어내도록 변경해보는것도 하나의 방법이 될 수 있을것 같습니다.

빌더 패턴을 삭제하고 원상태에서 비밥이 말한 객체간 상호작용에 대해 고민해 보았는데, 어떻게 객체의 상호작용을 통해 객체를 생성한다는 것인지 잘 모르겠습니다😥

생각해본 것은 생성자에 3가지 객체 대신 Map을 미리 만들어서 전달하는 것 입니다. 하지만 LadderGameResult가 생성자가 호출될 때 객체 자체가 생성되는 방식보다 좋다고 생각하지 않았습니다.

저는 생성자의 인자가 3개라는 것이 큰 부담이 된다고 생각하지 않아서, 객체간 상호작용을 통해 객체를 만드는 것이 어떤 의미이고 그에 대한 이점을 아직 잘 모르겠습니다😅

이에 대해 비밥의 의견을 들려주시면 감사하겠습니다!

return newlines;
}

public int climb(int playerIndex) {
Copy link

Choose a reason for hiding this comment

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

climb의 결과인 int가 어떠한 의미를 나타내는지 인터페이스만 보고는 파악하기 쉽지 않은것 같습니다.

값을 포장하면 좋을것 같습니다.

Copy link
Member Author

Choose a reason for hiding this comment

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

int가 반환되는 것을 봤을 때 어떤 것을 의미하는지 알기 어려울 것 같습니다!
int를 Position이라는 클래스로 포장하여 사다리에서의 position을 반환하여 명확한 의미를 가지게 하였습니다!

}

private int movePosition(int playerIndex, Line line) {
if (!isFarLeft(playerIndex) && isLegLeftExist(playerIndex, line)) {
Copy link

Choose a reason for hiding this comment

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

! 와 같은 부정표현을 사용하는 것은 코드를 읽는 사람에게 인지과부하를 일으키는 요소중 하나입니다. 가능하면 읽기 쉬운 메세지로 변경하는 것을 추천드립니다.

Copy link
Member Author

Choose a reason for hiding this comment

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

!가 있다면 보는사람으로 하여금 2번 생각하게 하는 결과를 만들 것 같군요😅
메서드에 Not을 붙여 맨 왼쪽, 맨 오른쪽이 아닌지 판별하도록 수정하였습니다!

Comment on lines 37 to 46
return playerIndex - 1;
}
if (!isFarRight(playerIndex, line) && isLegRightExist(playerIndex, line)) {
return playerIndex + 1;
}
return playerIndex;
}

private boolean isFarLeft(int playerIndex) {
return playerIndex == 0;
Copy link

Choose a reason for hiding this comment

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

매직 넘버가 존재하는 것 같습니다.

Copy link
Member Author

@tkdgur0906 tkdgur0906 Feb 28, 2024

Choose a reason for hiding this comment

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

isFarLeft()에서 0이 의미하는 것이 사다리의 가장 왼쪽의 위치를 의미합니다!
매직넘버를 상수로 분리해주었습니다!

}

private boolean isFarRight(int playerIndex, Line line) {
return playerIndex == line.getLegs().size();
Copy link

Choose a reason for hiding this comment

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

line.getLegs().size()는 특별한 의미를 같는것 같습니다. 객체에게 메세지를 던져보시면 좋을것 같습니다.

Copy link
Member Author

@tkdgur0906 tkdgur0906 Feb 28, 2024

Choose a reason for hiding this comment

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

line에서 Leg의 List의 크기는 사다리의 폭을 의미합니다!
따라서 line내부 메서드로 getWidth()를 생성하여 사용하도록 변경하였습니다!

Copy link

Choose a reason for hiding this comment

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

getWidth()을 다르게 표현해보는것은 어떨까요?

isFarRight에 그 의미가 표현되는것 같아보입니다.

Copy link
Member Author

Choose a reason for hiding this comment

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

isNotFarLeft()에서 0을 FAR_LEFT_POSITION으로 바꿨기 때문에 getWidth()보다 getFarRightPosition() 이 의미가 뚜렷할 것 같습니다!

}

@DisplayName("주어진 너비에 맞게 사다리의 다리의 개수를 생성한다.")
@Test
void makeLinesWithWidth() {
Ladder ladder = Ladder.from(HEIGHT, WIDTH);
Ladder ladder = Ladder.from(HEIGHT, WIDTH, randomLineGenerator);
Copy link

Choose a reason for hiding this comment

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

이 테스트에 for-loop가 활용되는것 같습니다.

Copy link
Member Author

Choose a reason for hiding this comment

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

for-loop로 테스트 하는 대신 범위를 줄여 명확한 값과 비교하는 테스트로 수정하였습니다!

Comment on lines 22 to 23
lineGenerator = new RandomLineGenerator();
trueGenerator = new TrueGenerator();
Copy link

Choose a reason for hiding this comment

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

혹시 이렇게 초기화해야하는 특별한 이유가 있나요?

Copy link
Member Author

@tkdgur0906 tkdgur0906 Feb 28, 2024

Choose a reason for hiding this comment

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

당시에는 별다른 이유 없이 @BeforeAll을 사용해서 생성하였습니다. 하지만 해당 객체는 외부에서 주입받는 것이 아니기 때문에 필드에서 초기화하는게 맞는 것 같습니다😅

}

static class TrueGenerator implements BooleanGenerator {
static class TestLineGenerator implements LineGenerator {
Copy link

Choose a reason for hiding this comment

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

사용되지 않는 클래스인것 같습니다.

@pci2676
Copy link

pci2676 commented Feb 27, 2024

추가로 테스트 코드에서 exception을 검사하는데 message는 검사하지 않는 경우가 있는것 같습니다.

message를 검사하지 않고 타입만 검사하면 어떤 문제가 발생할 수 있는지 고민해보시면 좋을것 같습니다.

@tkdgur0906
Copy link
Member Author

예외를 검증할 때 타입만 검증하고 예외메시지를 검증하지 않는다면, 해당 예외가 원하는 예외가 아닌 경우에도 테스트가 통과하는 거짓음성이 발생할 수 있다고 생각합니다!
타입 뿐만 아니라 예외 메시지를 같이 검증해야 테스트의 신뢰도를 높일 수 있을 것 같습니다😀

Copy link

@pci2676 pci2676 left a comment

Choose a reason for hiding this comment

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

리뷰 반영해주신 내용 확인했습니다 👍

조금 더 고민해볼만한 부분에 코멘트를 남겨두었으니 확인 부탁드립니다.

return;
}
legs.add(Leg.from(booleanGenerator.generate()));
public int getWidth() {
Copy link

Choose a reason for hiding this comment

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

int도 다르게 표현해보면 좋을것 같습니다.

Copy link
Member Author

Choose a reason for hiding this comment

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

getWidth가 의미하는 것이 사다리의 특정 위치이기 때문에 int보다 명확한 의미를 가진 Position 객체를 반환하는게 좋을 것 같습니다!

Comment on lines 26 to 28
HashSet<String> uniquePlayers = new HashSet<>();
players.stream()
.forEach(player -> uniquePlayers.add(player.getName()));
Copy link

Choose a reason for hiding this comment

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

아직 Stream이 익숙치 않으신것인지 컬렉션의 선언과 구성을 채우는 코드가 이원화되어있는 부분이 보이는것 같습니다.

Stream을 이용해서 한번에 정의와 생성을 해보는 연습을 해보시면 좋을것 같습니다.

Copy link
Member Author

Choose a reason for hiding this comment

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

stream의 collect(Collectors.toSet())을 사용해서 추가적인 정의 없이 한번에 set으로 반환할 수 있게 변경하였습니다!

Comment on lines 10 to 14
public class LadderResultDto {

private final List<Player> players;
private final List<Line> lines;
private final Results results;
Copy link

Choose a reason for hiding this comment

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

값을 단순히 담아가는것이 아니라 필요한 정보를 만들어서 전달해보면 더욱 좋을것 같습니다.

}

@DisplayName("높이가 0이하면 예외가 발생한다.")
@ParameterizedTest
@ValueSource(ints = {0, -1})
void createHeightWithUnderZero(int invalidHeight) {
assertThatThrownBy(() -> new Height(invalidHeight))
.isInstanceOf(IllegalArgumentException.class);
.isInstanceOf(IllegalArgumentException.class)
.hasMessageContaining(INVALID_LADDER_HEIGHT_EXCEPTION.getMessage());
Copy link

Choose a reason for hiding this comment

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

👍

@DisplayName("주어진 참여자의 사다리 타기 결과를 인덱스로 반환한다.")
@Test
void climb() {
Ladder ladder = Ladder.from(HEIGHT, WIDTH, new CustomLineGenerator());
Copy link

Choose a reason for hiding this comment

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

HEIGHT, WIDTH가 모든 테스트에서 사용되는데 정확한 값을 파악하기위해 테스트 클래스의 상위로 이동하여 값을 확인해야하는것 같습니다.

테스트 코드에서 동일하게 사용되는 값을 중복으로 생각하시고 상수로 추출하신것이 아닐까 싶습니다.

테스트 코드는 테스트 코드를 벗어나지 않고 모든 값을 확인할수 있는 것이 가장 읽기 좋습니다.

Copy link
Member Author

Choose a reason for hiding this comment

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

상수 처리를 하면 읽는 사람이 상수를 보러 왔다갔다 해야한다는 단점이 있군요!
웬만하면 테스트 내에서 모두 처리할 수 있도록 생성하겠습니다!

Copy link

@pci2676 pci2676 left a comment

Choose a reason for hiding this comment

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

지난번 리뷰드렸던 내용을 비롯해 많이 개선해주신것 같습니다.

질문에 대한 답변과 한번 더 고민해볼만한 부분에 조금 리뷰를 남겨두었으니 확인부탁드립니다.

Comment on lines +7 to +9
private final List<String> playerNames;
private final List<List<Boolean>> lines;
private final List<String> resultNames;
Copy link

Choose a reason for hiding this comment

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

dto에 객체가 여전히 들어가있어 dto의 권한 이상의 것을 수행할 수 있을 것 같습니다!
dto에 저장할 멤버 변수를 원시값으로 수정하여 데이터를 전달하는 역할만 할 수 있도록 수정하였습니다!

잘 반영해주신것 같습니다.

다만, 단일 값 객체는 값 객체 그대로 사용해도 괜찮을것 같습니다.

객체를 어떠한 형태로 어떻게 넘길지는 정답이 정해진것은 아니니 앞으로 미션을 하시면서 많이 고민해보시면 좋을것 같습니다.

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를 넘길 때 필요한 데이터만 포함할 수 있도록 범위를 최소화해야겠네요!
단일 값 객체도 원시값으로 바꿔서 보내야하나 고민했었는데, 이에 관해서는 유연하게 처리해야겠습니다!


private final Map<Player, Result> ladderGameResult;

public LadderGameResult(Ladder ladder, Players players, Results results) {
Copy link

Choose a reason for hiding this comment

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

빌더 패턴을 삭제하고 원상태에서 비밥이 말한 객체간 상호작용에 대해 고민해 보았는데, 어떻게 객체의 상호작용을 통해 객체를 생성한>다는 것인지 잘 모르겠습니다😥

생각해본 것은 생성자에 3가지 객체 대신 Map을 미리 만들어서 전달하는 것 입니다. 하지만 LadderGameResult가 생성자가 호출될 >때 객체 자체가 생성되는 방식보다 좋다고 생각하지 않았습니다.

저는 생성자의 인자가 3개라는 것이 큰 부담이 된다고 생각하지 않아서, 객체간 상호작용을 통해 객체를 만드는 것이 어떤 의미이고 그에 >대한 이점을 아직 잘 모르겠습니다😅

이에 대해 비밥의 의견을 들려주시면 감사하겠습니다!

인자가 작을수록 좋다는데는 이견이 없을것 같습니다.

학습의 의미를 두고 인자가 작은 인터페이스를 생성하는것을 연습해보시면 좋을것 같아 리뷰를 드렸습니다.

지금의 경우 ladder에게 player와 result를 넘겨주고 ladderGameResult를 생성하는 등의 메세지를 던져 ladderGameResult를 생성하는 방법을 염두하였습니다.

Copy link

Choose a reason for hiding this comment

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

또한 Result Map을 이용할 경우 Position의 동등성 비교로 ladderGameResult를 생성하는 로직이 간단해질수 있을것 같아보입니다.

Copy link
Member Author

Choose a reason for hiding this comment

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

LadderGameResult에 여러 객체를 넘겨 생성자 내부에서 로직을 처리하는 대신, ladder에 makeLadderGameResult 메서드를 만들어 Player, Result를 받아 생성할 수 있게 하였습니다.
생성자에서 모두 처리한 것 보다 책임을 분리하니 가독성이 높아진 것 같습니다!

Copy link
Member Author

@tkdgur0906 tkdgur0906 Mar 2, 2024

Choose a reason for hiding this comment

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

  1. Result Map을 이용하라는 것이 Map<Result, Position>을 생성하여 player의 이동 결과와 동등비교 하라는 것일까요? -> 이렇게 해보니 더 복잡해 보이는 것 같습니다.
    Result Map이 어떤 것을 의미하는 건지 잘 모르겠습니다😅

  2. 비밥은 객체의 생성자에 값을 할당해주는 로직, 검증 로직 이외에, LadderGameResult를 고치기 이전처럼 복잡한 생성 로직을 포함하는 것에 대해 어떻게 생각하시나요?
    저는 너무 복잡한 것만 아니라면 포함되어도 괜찮을 것 같다는 생각을 하고 있는데, 비밥의 의견을 듣고 싶습니다!

Copy link

Choose a reason for hiding this comment

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

Result Map을 이용하라는 것이 Map<Result, Position>을 생성하여 player의 이동 결과와 동등비교 하라는 것일까요? -> 이렇게 해보니 더 복잡해 보이는 것 같습니다.
Result Map이 어떤 것을 의미하는 건지 잘 모르겠습니다😅

죄송합니다. 제가 적은 리뷰를 다시 읽어봤는데 저도 이해하기 어렵게 적어두었네요. 😅

Results는 Result 컬렉션을 관리하는 역할이라고 보았을때 getter를 이용하여 컬렉션 자체가 외부로 노출되면 외부에서 Results의 구현세부사항을 알게되는것과 비슷해질 수 있습니다.

그런데 Results가 외부에 제공하는 정보를 보았을 때 Results에서 getter가 노출될 이유가 딱히 없어보였기 때문입니다.

그래서 Results가 Map<Position, Result>형태로 컬렉션을 관리하면 아래와 같이 표현될것 같아 리뷰를 드렸습니다.

public LadderGameResult makeLadderGameResult(Players players, Results results) {
        Map<Player, Result> ladderGameResult = players.getPlayers().stream()
                .collect(Collectors.toMap(player -> player,
                        player -> results.findResult(player)));

Copy link

Choose a reason for hiding this comment

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

비밥은 객체의 생성자에 값을 할당해주는 로직, 검증 로직 이외에, LadderGameResult를 고치기 이전처럼 복잡한 생성 로직을 포함하는 것에 대해 어떻게 생각하시나요?
저는 너무 복잡한 것만 아니라면 포함되어도 괜찮을 것 같다는 생각을 하고 있는데, 비밥의 의견을 듣고 싶습니다!

개인마다 생각하는 '복잡하다'의 정도는 다를것 같아 말씀드리기 조금 어려운 부분이 있는것 같습니다.

저도 당연히 복잡하면 안넣는게 좋을것 같고, 단순하다면 넣어도 된다고 생각합니다. 😄

여기부터는 제 경험을 토대로 말씀드려보겠습니다.

가능하면 생성자에 객체를 생성하는 로직은 안넣는 편입니다. 모든 경우에 그렇지는 않지만 개발자가 제어할 수 있는 영역을 벗어나고 상호작용하는 객체에 대해 자세히 파악해야하는 경우가 많기 때문입니다.

제가 가장 기피하는 상황은
특정 객체를 생성하는 협력 객체들의 소통 과정이 객체의 생성자 내에 전부 노출이되어있는 경우입니다.

이러한 경우 만들고자하는 객체를 생성하기 위해 알아야 하는 지식이 협력객체의 수만큼 늘어나게 됩니다.
A만을 위한 비즈니스 로직을 만들고 싶은 상황에서 A를 만들기 위해 B를 알아야하고, B를 알아보려하다보니 C를 또 알아야하는 상황이 생길수도 있습니다.
A입장에서는 자신이 어떻게 생성되는지 보다 자신이 가지고 있는 값들이 일관성과 규칙을 잘 준수하는지가 중요하다. 라고 생각합니다.

또한 원하는 형태의 객체를 만드는데 많은 제약사항을 가지게 되는 경우도 있습니다.

지금의 상황에서 테스트를 예로 들었을때 LadderGameResult의 생성자에 3개의 파라미터를 필요로하는 경우 public Result get(Player player) 테스트하기 위해 Ladder, Players, Results까지 모든 객체가 필요하지만

LadderGameResult가 관심있는 컬렉션만을 입력받아 테스트를 할 수 있다면 아래의 테스트 코드와 같이 노출되는 정보도 적어지고 한결 편리한 테스트가 작성될 수� 있어 선호하는 편 입니다.

        Player player = new Player("pobi", new Position(0));
        Result result = new Result("꽝");
        LadderGameResult ladderGameResult = new LadderGameResult(Map.of(
                player, result
        ));

        Result actual = ladderGameResult.get(player);
        
        assertThat(actual).isEqualTo(result);

Copy link
Member Author

Choose a reason for hiding this comment

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

Results가 Position과 Result를 같이 가지고 있는 방식으로 수정하면 좀 더 로직을 작성하는데 있어서 간단하게 할 수 있겠네요! 일급 컬렉션을 무작정 List로 묶는 것이 아닌, 도메인이 포함하면 좋은 정보에 관해서도 생각해봐야겠습니다!

Copy link
Member Author

@tkdgur0906 tkdgur0906 Mar 4, 2024

Choose a reason for hiding this comment

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

생성자가 복잡해지지 않아야 한다는 것에 대해 저도 동의합니다!
필요한 컬렉션만 받아 다른 객체를 체인으로 서로 아는 것을 지양하기 위해 구조에 대해서 많이 고민해봐야겠습니다.

Comment on lines 14 to 19
private Ladder(int height, int width, LineGenerator lineGenerator) {
lines = makeLines(new Height(height), new Width(width), lineGenerator);
}

public static Ladder from(int height, int width) {
Ladder ladder = new Ladder(height, width);
ladder.makeLines(width);
return ladder;
public static Ladder from(int height, int width, LineGenerator lineGenerator) {
return new Ladder(height, width, lineGenerator);
Copy link

Choose a reason for hiding this comment

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

정적 메서드를 활용해서 생성하는걸 선호하시는것 같은데 혹시 특별한 이유가 있을까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ladder같은 경우에는 내부적으로 특별한 로직이 없어 사용하는 것에 대해 큰 이점이 있다고 생각하지는 않지만, 개인적으로 객체를 생성할 때 new 대신 이름을 부여하는 것을 선호하기 때문에 사용하고 있습니다!

Copy link

@pci2676 pci2676 left a comment

Choose a reason for hiding this comment

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

질문해주신 부분에 대한 답변을 달아두었으니 확인해주시고 다시 요청을 주시면 merge하도록 하겠습니다.

Copy link

@pci2676 pci2676 left a comment

Choose a reason for hiding this comment

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

사다리 미션 수행하시느라 고생 많으셨습니다.

머지하겠습니다. 🚀

@pci2676 pci2676 merged commit 872901e into woowacourse:tkdgur0906 Mar 4, 2024
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