Skip to content

Step2 #2378

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 11 commits into
base: kjy2844
Choose a base branch
from
Open

Step2 #2378

wants to merge 11 commits into from

Conversation

kjy2844
Copy link

@kjy2844 kjy2844 commented Apr 14, 2025

질문

  1. Ladder, LadderRow의 constructor가 객체를 초기화할 때 하는 일이 많은데, 이렇게 둬도 괜찮은지, 팩토리 메서드 같은 것으로 분리하는게 좋은지에 대한 의견이 궁금합니다.
  2. getter를 제거하기 위해 forEach를 만들었는데, 이런 방법도 괜찮을지 궁금합니다.

사다리 게임 기능 목록

  1. 사용자 입력 처리
  • 참여자 이름 입력 받기 (쉼표로 구분)
  • 이름 유효성 검사 (최대 5글자)
  • 사다리 높이 입력 받기 (숫자)
  1. 사다리 생성
  • 사다리 높이만큼 행 생성
  • 참여자 수에 맞게 열 생성
  • 랜덤으로 가로 연결선 생성
  • 가로 연결선 겹치면 안됨
  1. 사다리 출력
  • 참여자 이름 출력 (5자 기준 정렬)
  • 사다리 모양 출력 ("|", "-" 사용)

Comment on lines +13 to +17
public Ladder(int height, int width) {
for (int i = 0; i < height; i++) {
ladderRows.add(new LadderRow(width, () -> RANDOM.nextBoolean()));
}
}

Choose a reason for hiding this comment

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

이러면 여기에 대한 테스트가 불가할것으로 보입니다.
인터페이스를 통한 generator를 만들어서 인자로 넣는 방식을 이용하면 어떨까여?

@ksy90101
Copy link

Ladder, LadderRow의 constructor가 객체를 초기화할 때 하는 일이 많은데, 이렇게 둬도 괜찮은지, 팩토리 메서드 같은 것으로 분리하는게 좋은지에 대한 의견이 궁금합니다.

팩터리 메서드가 나은거 같습니다!

getter를 제거하기 위해 forEach를 만들었는데, 이런 방법도 괜찮을지 궁금합니다.

당연히 collection이라면 forEach가 있는게 어색하지 않습닝다. getter를 만들지 않는 방법이라면 나쁘지 않을것으로 판단됩니다.

Copy link

@ksy90101 ksy90101 left a comment

Choose a reason for hiding this comment

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

간단한 구조에 대한 리뷰 남겼습니다! 확인해보시면 좋을거 같습니다.
언제든지 궁금한게 있다면 DM주세요!

throw new IllegalArgumentException("이름은 필수입니다.");
}

if (name.length() > 5) {

Choose a reason for hiding this comment

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

상수를 통해 의미 전달을 해보시면 좋겠습니다.

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;

public class PlayerTest {

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 +18
public Players(List<String> playerNames) {
if (playerNames == null) {
throw new IllegalArgumentException("플레이어가 존재하지 않습니다.");
}
this.players = playerNames.stream()
.map(Player::new)
.collect(Collectors.toList());
}

Choose a reason for hiding this comment

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

여기 테스트가 미흡합니다.

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