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단계 - 사다리(생성) #1708

Merged
merged 21 commits into from May 12, 2023
Merged

2단계 - 사다리(생성) #1708

merged 21 commits into from May 12, 2023

Conversation

dkswnkk
Copy link

@dkswnkk dkswnkk commented May 10, 2023

뭔가 급격하게 난이도가 올라간 느낌이네요..!

저는 이번 2단계를 진행하면서 한 가지 궁금점이 계속 들었습니다.
다음과 같은 Line.Class가 있을때

public class Line {
    private final List<Boolean> points;
}
public class Lines {
    private final List<Line> lines;

이렇게 Lines.class로 묶어서 다른 로직에서 사용하는 방식과 그냥 List<Line> lines로 사용하는 것 중 고민을 했습니다.
한 번 더 일급 컬렉션으로 묶으면 장점은 분명하나 단점으로 도메인 클래스가 너무나도 많아지고, 한눈에 로직을 파악하기 어렵다는 느낌이 들었는데, 멘토님께서는 어떤 생각을 가지고 계신가용?

Copy link

@brainbackdoor brainbackdoor left a comment

Choose a reason for hiding this comment

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

2단계 미션 진행하느라 고생하셨어요~ 👍🏻
사다리 미션부터 난이도가 많이 올라가지요 😅
이번 미션에서 달성하고자한 목표는 이룬듯하여 여기서 머지해요~
Comment 남겨둔 부분들은 다음 단계 진행하면서 한번 참고해보세요~

Comment on lines +12 to +14
public List<ParticipantName> getNames() {
return names;
}

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 +23
private static final Random RANDOM = new Random();
private boolean wasPrevious;

@Override
public List<Boolean> generate(int size) {
return IntStream.range(0, size - 1)
.mapToObj(i -> generatePoint())
.collect(Collectors.toList());
}

private boolean generatePoint() {
boolean point = !wasPrevious && RANDOM.nextBoolean();
this.wasPrevious = point;

Choose a reason for hiding this comment

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

👍🏻

Comment on lines +21 to +28
public static void printParticipantNamesAndLadders(LadderGame ladderGame) {
System.out.println("실행 결과" + NEWLINE);

String output = formatParticipantNames(ladderGame.getParticipantNames()) +
formatLines(ladderGame.getParticipantNames().getFirstParticipantNameLength(), ladderGame.getLines());

System.out.println(output);
}

Choose a reason for hiding this comment

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

Suggested change
public static void printParticipantNamesAndLadders(LadderGame ladderGame) {
System.out.println("실행 결과" + NEWLINE);
String output = formatParticipantNames(ladderGame.getParticipantNames()) +
formatLines(ladderGame.getParticipantNames().getFirstParticipantNameLength(), ladderGame.getLines());
System.out.println(output);
}
public static void printParticipantNames(ParticipantNames participantNames) {
System.out.println("실행 결과" + NEWLINE);
System.out.println(formatParticipantNames(participantNames));
}
public static void printLadders(Lines lines) {
System.out.println(formatLines(lines));
}
  • 기능별로 메소드를 나누고 출력에 필요한 VO만 넘기는건 어떨까요?
  • 기능 요구사항에 "사람 이름을 5자 기준으로 출력"한다고 했으므로, 첫번째 사람 이름에 따라 변경되기 보다는 5자리로 공백을 둬도 되지 않을까요

Comment on lines +11 to +12
@DisplayName("사다리타기 테스트")
public class LadderTest {

Choose a reason for hiding this comment

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

  • 사다리 생성관련 테스트도 작성하는건 어떨까요? 가령, "사다리 타기가 정상적으로 동작하려면 라인이 겹치지 않도록 해야 한다" 등의 조건이 준수되는지 테스트해볼 수 있지 않을까요? 룰에 대한 테스트도 가능하고 Line 생성 테스트도 가능할듯합니다.
  • 랜덤 규칙을 DI하도록 구성하셨던데요. Fake 객체를 넣어서 테스트해보는건 어떨까요? 가령, 높이만큼 Lines가 생성되는지, Line이 사람수 만큼 생성되는지 등등도 테스트해볼 수 있을거 같아요.

}

@Test
@DisplayName("참가자 이름이 영문이 아닌 문자가 포함되어 있다면 예외를 던진다.")

Choose a reason for hiding this comment

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

참가자 이름에 길이 조건 외에 다른 조건도 있었나요?

Comment on lines +5 to +14
public class Lines {
private final List<Line> lines;

public Lines(List<Line> lines) {
this.lines = lines;
}

public List<Line> getLines() {
return lines;
}

Choose a reason for hiding this comment

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

public Lines(int countOfParticipant, int height, GenerateLadderPointStrategy strategy) {
        this(IntStream.range(0, height)
                .mapToObj(i -> new Line(countOfParticipant, strategy))
                .collect(Collectors.toList()));
    }

지금과 같이 data holder로서만 존재한다면 질문주셨던 것처럼 일급콜렉션을 따로 구성하지는 않을거 같아요.
다만, LadderGame 의 setLines의 로직은 Lines의 생성자에서 진행되는건 어떨까 싶네요.

@brainbackdoor brainbackdoor merged commit bdd504d into next-step:dkswnkk May 12, 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.

None yet

2 participants