Skip to content

[사다리미션] 강민혁 제출합니다. #56

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

Open
wants to merge 5 commits into
base: kangminhyuk1111
Choose a base branch
from

Conversation

kangminhyuk1111
Copy link

@kangminhyuk1111 kangminhyuk1111 commented May 28, 2025

3단계 까지 구현 완료하였고 여기서 고민이 조금 있어서 먼저 PR올려봅니다!
4단계 까지 구현 완료 했습니다!

테스트 구현

저 같은 경우 random하게 사다리의 중간 다리를 생성하기 위해 nextBoolean을 구현하는 함수형 인터페이스를 Ladder 객체를 생성할 때 내부에 넣어줬습니다.

그렇게 하니 Ladder가 내부에 List을 가지고 이 Line에 nextBoolean을 통해 들어가는 값이 모두 동일한 값이 들어가게 되어서 테스트를 다양하게 작성을 못하고 한정적으로 작성해야되는데 이런경우를 해결하기 위해서는 어떻게 해야할까요?

지금드는 생각은 Ladder에 randomgenerator를 주입시키지말고 한 뎁스 더 깊게 Line에서 randomgenerator를 넣어주면 되나 라는 생각이 들기는 하는데 혹시 어떻게 보시나요..?

미션 진행 과정

이번 미션 진행중에 가장 어려웠던 부분은 첫번째 도메인 분석부터 사다리 그리기까지 였던 것 같습니다.
하지만 이후에 사다리를 그리고 사다리의 결과를 반환하는 부분까지 작성하니 이후 3단계와 4단계는 그리 어렵지 않았던 것 같습니다.
저번에 알려주셨던 부분중에 @FuntionalInterface 어노테이션에 대해서 찾아봤습니다.

@FuntionalInterface
인터페이스 내부에 하나의 메서드만 선언해주면 FuntionalInterface인데 굳이 명시적으로 선언해주는 이유는 컴파일 단계에서 강제성을 갖게 해주기 위함 입니다. 함수형으로 쓰겠다고 하고 막상 코드에 두개이상의 메서드가 있는경우는 규칙을 위배합니다. FuntionalInterface 어노테이션으로 명시적으로 이 함수는 함수형 인터페이스야 라고 선언해주게 되면 컴파일 단계에서 에러를 발견할 수 있게됩니다. 그리고 다른 개발자들도 이것은 람다를 사용하기 위한 함수형 인터페이스라고 명확하게 설명도 된다고 합니다.

그리고 Ladder 클래스는 생성자를 private하게 가지고 팩토리 메서드를 통해 객체를 생성함으로써, 객체 내부에 생성자의 복잡한 로직은 보여주지 않고 .of를 통해 간결한 생성 방식을 사용했습니다.

그리고 예측 가능한 테스트 범위를 만들기 위해 of와 testOf를 만들었는데, 이같은경우 수정이 필요하다고 판단합니다! 왜냐하면, 저번에 설명해주신 부분중에 테스트 만을 위한 코드는 좋지 못하다. 라고 했던 것이 기억이 납니다. 테스트 만을 위한 코드는 비즈니스 로직에 굳이 필요없는 부분이고 결국에는 테스트 자체도 비즈니스 로직을 테스트하기 위한 수단인데 주객 전도가 되어버리는 상황이기 때문에 변경되어야 하는 부분이라고 생각합니다.

나머지 부분은 비슷한 부분이 많았다고 생각하고 이번 경험을 통해 도메인에 대한 고민을 조금 더 깊게한 것 같습니다.

Copy link

@RIANAEH RIANAEH 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 LadderController {

public void run() {
// 사용자, 결과 입력받기
Copy link

Choose a reason for hiding this comment

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

코드만으로도 상대방을 이해시킬 수 있다면 주석은 없어도 되지 않을까요?

Copy link

Choose a reason for hiding this comment

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

메서드가 10줄을 넘어가지 않게 리팩터링 부탁드려요!

final String rewardString = InputView.inputReward();

// 도메인으로 변환
final Players players = InputParser.parsePlayers(playersString);
Copy link

Choose a reason for hiding this comment

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

InputView와 InputParser를 나누어주신 이유가 궁금해요!

Copy link
Author

Choose a reason for hiding this comment

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

InputView는 입력만을 받도록 설계를 해야될 것 같아서 했습니다.
InputView를 통해 입력받은 String을 파싱하는 객체를 따로 만들어 주도록 했습니다.
후에 InputView는 동일하게 String을 입력받도록 유지되고 파싱하는 방법만 변경하게 된다면 조금 더 객체지향적이지 않을까? 라고 생각하고 작성한 것 같습니다.

Copy link

Choose a reason for hiding this comment

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

저는 관련 역할을 InputView에서 모두 처리해도 되지 않을까 생각했어서 민혁님의 생각이 궁금했어요!
객체를 나누는데에는 방법이 다양해서 생각해주신 방향대로 진행해주셔도 좋을 것 같네요👍

Comment on lines 6 to 26
public class Ladder {

private final List<Line> lines;

private Ladder(final int width, final int height, final BooleanGenerator booleanGenerator) {
validateWidth(width);
validateHeight(height);

this.lines = new ArrayList<>();
for (int i = 0; i < height; i++) {
lines.add(new Line(width, booleanGenerator));
}
}

public static Ladder of(final int width, final int height) {
return new Ladder(width, height, new RandomBooleanGenerator());
}

static Ladder testOf(final int width, final int height, final BooleanGenerator generator) {
return new Ladder(width, height, generator);
}
Copy link

Choose a reason for hiding this comment

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

저 같은 경우 random하게 사다리의 중간 다리를 생성하기 위해 nextBoolean을 구현하는 함수형 인터페이스를 Ladder 객체를 생성할 때 내부에 넣어줬습니다.
그렇게 하니 Ladder가 내부에 List을 가지고 이 Line에 nextBoolean을 통해 들어가는 값이 모두 동일한 값이 들어가게 되어서 테스트를 다양하게 작성을 못하고 한정적으로 작성해야되는데 이런경우를 해결하기 위해서는 어떻게 해야할까요?
지금드는 생각은 Ladder에 randomgenerator를 주입시키지말고 한 뎁스 더 깊게 Line에서 randomgenerator를 넣어주면 되나 라는 생각이 들기는 하는데 혹시 어떻게 보시나요..?

Ladder에 List를 넣어주는 생성자, Line에 List를 넣어주는 생성자를 만들어주면 해결되지 않을까요?
추가적으로 각 생성자에서는 각 객체의 규칙에 맞는 검증을 넣어줘야겠죠?
(ex. Line에서는 List 내의 동일 값이 continuous 할 수 없다.)

Copy link

Choose a reason for hiding this comment

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

this.lines에 list를 먼저 할당하고 값을 add 해주는것이 아니라, 다 만들어진 list를 넣어주면 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

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

실제로 적용은 Line을 생성할 때, booleangenerator를 넣어주니 테스트 용이한 코드로 변경 가능했습니다!

Comment on lines 40 to 50
private void validateWidth(int width) {
if (width < 2) {
throw new RuntimeException("사다리 폭은 최소 2 이상이어야 합니다.");
}
}

private void validateHeight(int height) {
if (height < 1) {
throw new RuntimeException("사다리 높이는 최소 1 이상이어야 합니다.");
}
}
Copy link

Choose a reason for hiding this comment

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

👍

Comment on lines 13 to 22
public static Players parsePlayers(String players) {
return new Players(Arrays.stream(players.split(","))
.map(Player::new)
.toList());
}

public static Rewards parseRewards(String rewards) {
return new Rewards(Arrays.stream(rewards.split(","))
.toList());
}
Copy link

Choose a reason for hiding this comment

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

사용성 측면에서 trim을 진행해주면 좋을 것 같아요.

image

Comment on lines 30 to 31
final Ladder ladder = Ladder.of(width, height);
final LadderGame game = new LadderGame(ladder);
Copy link

Choose a reason for hiding this comment

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

만들어진 사다리를 이용해 게임을 진행하는 LadderGame를 만들어주셨군요! 좋습니다😎

Copy link
Author

Choose a reason for hiding this comment

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

👍

Comment on lines 11 to 15
private void validateName(final String name) {
if (name.length() >= 5) {
throw new RuntimeException("이름은 5글자를 초과할 수 없습니다.");
}
}
Copy link

Choose a reason for hiding this comment

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

이상일 수 없습니다?

Copy link
Author

Choose a reason for hiding this comment

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

앗 잘못 작성되었네요! 수정하겠습니다.

Comment on lines 17 to 24
@Override
public boolean equals(final Object o) {
if (o == null || getClass() != o.getClass()) {
return false;
}
final Player player = (Player) o;
return Objects.equals(name, player.name);
}
Copy link

Choose a reason for hiding this comment

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

왜 equals만 재정의해주셨나요?

Copy link
Author

Choose a reason for hiding this comment

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

제가 찾아봤을 때, hashcode를 record 클래스에서는 자동으로 재정의 해준다고 알고있었는데, 잘못된 정보였습니다.
record는 equals와 hashcode를 자동으로 재정의 해준다고 하네요! 둘다 굳이 작성할 필요가 없는 부분인 것같습니다.

Copy link

Choose a reason for hiding this comment

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

record에 대해 학습해주셨군요👍
(테스트로도 동등성 체크해주신 부분 좋네요!)

printRewards(rewards);
}

public static void printResults(Players players, Rewards rewards, LadderResult results) {
Copy link

Choose a reason for hiding this comment

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

Players, Rewards, LadderResult 를 엮어주는 일을 OutputView에서 담당하고 있는걸로 보이는데요.
이 친구가 담당해주는게 맞을까요?

import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.ValueSource;

class LadderTest {
Copy link

Choose a reason for hiding this comment

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

다양한 테스트를 작성해주셨군요! 크!!🔥

Copy link
Author

Choose a reason for hiding this comment

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

👍👍👍

Comment on lines +9 to +23
private Ladder(final List<Line> lines) {
validateLines(lines);

this.lines = lines;
}

private void validateLines(final List<Line> lines) {
validateEmpty(lines);
validateWidth(lines);
validateHeight(lines);
}

public static Ladder of(final List<Line> lines) {
return new Ladder(lines);
}
Copy link

Choose a reason for hiding this comment

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

Ladder 생성자를 만들고 검증을 진행해주셨군요! 정적 팩토리메서드에서도 생성자를 사용하도록 잘 변경해주셨네요!
하지만 지금은 생성자와 정적 팩토리메서드의 용도가 같아보여요.
넗이, 높이, 커넥션제너레이터 등을 파라미터로 받는 generate, initialize 등의 정적 팩토리메서드로 바꿔보면 어떨까요?
의도해서 말씀드린 방향은 이거였어요!

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