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

[사다리 타기] 김나윤 미션 제출합니다. #11

Open
wants to merge 20 commits into
base: bbggr1209
Choose a base branch
from

Conversation

bbggr1209
Copy link

어려웠던 점

  • TDD 방식으로 코드를 작성하는 것이 익숙치 않아 시간이 오래 걸렸습니다.
  • 일급컬렉션, 들여쓰기 등 프로그래밍 요구사항을 충족시키면서 코드를 작성하는 것이 생각보다 까다로웠습니다.
  • 테스트 작성할 범위를 어디까지 구현해야할지 정하는 것이 어려웠습니다.

궁금한 점

  • 테스트 단위를 어느 정도 범위까지 작성하는 것이 적절할까요? 가능한 모든 경우에 대해 테스트하는 것이 최선인가요?

Copy link

@hangillee hangillee left a comment

Choose a reason for hiding this comment

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

안녕하세요, 나윤! 👋
중간고사 끝나고 미션하느라 고생 많았습니다!

테스트 단위를 어느 정도 범위까지 작성하는 것이 적절할까요?
결론부터 말씀드리자면 테스트 범위는 개발자의 선택에 달렸습니다. 즉, 정답은 없습니다! 내가 무엇을 테스트하고 싶은지, 내가 어디까지 테스트해야 내 코드를 신뢰할 수 있는지 같은 기준을 세우고 테스트를 작성하면 자연스럽게 나만의 테스트 범위가 생깁니다. 저는 주로, 도메인과 비즈니스 로직, 예외 발생은 무조건 테스트를 작성합니다.

현재 저는 우테코 레벨 2에서는 Spring을 학습하고 있습니다. Spring에는 도메인 뿐만 아니라 Controller, Service, Repository 같은 Layered Architecture 객체들도 있습니다.(나윤도 잘 알다시피) 이때는 더욱 어디까지 테스트해야 할지 감이 잘 잡히지 않을 수 있습니다. 그래서 나만의 테스트 기준을 세우고 기준에 맞게 테스트가 잘 작성 되었는지 여러번 검토해야 합니다.

물론, 테스트 범위에 정답은 없으나 오답은 있습니다. 아예 테스트를 작성하지 않거나, 테스트하기 어렵다는 이유로 도메인 생성 로직이나 서비스의 핵심 비즈니스 로직을 테스트하지 않으면 결코 좋은 테스트라고 할 수 없습니다. 기본적으로 나윤이 구현하고자 하는 기능은 모두 테스트를 하는 것이 좋고, Java가 구현 해준 표준 입출력과 랜덤 값 생성 같은 기능은 따로 테스트하지 않아도 됩니다.

쉽게 말해서 언어에 대한 도전(테스트 작성)은 무의미하다는 말입니다. 나윤이 짠 코드를 테스트하는 것만으로 충분합니다. 이번 미션에서는 주어진 기능 요구 사항을 모두 만족했는가를 기준으로 테스트 범위 기준을 세워보면 좋겠네요. 리팩토링 파이팅!

p.s. 테스트 커버리지를 맹신하는 것은 좋지 않지만 참고 지표는 될 수 있습니다! 전 도메인 패키지의 클래스들은 메소드 커버리지가 합당한 사유가 없는 한, 100% 달성을 목표로 놓친 테스트가 없는지 검토합니다.

Comment on lines +1 to +46
## 구현할 기능 목록

- [x] 플레이어 이름 입력
- [x] 이름을 입력 받을 메세지 출력
- [x] 이름을 입력 받음
- [x] 이름을 쉼표(,)를 기준으로 분리
- [x] 이름의 유효성 검사
- [x] 이름이 5글자 이하여야 한다.
- [x] 이름에는 공백이 들어갈 수 없다.
- [x] 이름이 중복되어서는 안된다.

- [x] 사다리 높이 입력
- [x] 사다리 높이 입력 메세지 출력
- [x] 사다리 높이 입력 받음
- [x] 사다리 높이의 유효성 검사
- [x] 사다리 높이는 1 이상이어야 한다.
- [x] 사다리 높이는 0~9 사이의 숫자여야 한다.

- [x] 결과 출력
- [x] 플레이어 이름 출력
- [x] 사다리 출력

---

## 필요한 객체

### Player
- 이름을 입력받고 유효성 검사를 수행하는 객체

### Players
- Player 객체를 관리하는 객체

### Line
- 사다리의 선을 관리하는 객체

### Point
- 선의 시작점과 끝점을 관리하는 객체

### Line Generator
- Line을 랜덤으로 생성하는 객체

### Ladder
- 여러 개의 선 객체를 관리하는 객체

### 입출력
- 입력을 받고 출력하는 객체

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 +22
public class Ladder {

private final List<Line> lines;

public Ladder(int numberOfPlayers, int height) {
lines = new ArrayList<>();
for (int i = 0; i < height; i++) {
lines.add(LineGenerator.generateLine(numberOfPlayers));
}
}

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 List<String> getPlayerNames() {
return players.stream()
.map(Player::getName)
.collect(Collectors.toList());

Choose a reason for hiding this comment

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

toList()만 사용해도 List 전환 가능합니다!

public static Line generateLine(int numberOfPlayers) {
List<Point> points = new ArrayList<>();
for (int i = 0; i < numberOfPlayers - 1; i++) {
boolean next = random.nextBoolean() && !previous;

Choose a reason for hiding this comment

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

부정 연산자 !은 가독성을 해친다는 의견이 있습니다.

조건문으로 리팩토링 하거나 다른 방법이 있다면 해당 방법으로 구현하는 편이 나을 수 있습니다.
다만, 현재 코드는 그렇게까지 가독성을 해치진 않는 것 같습니다. 참고만 해주시고, 리팩토링은 나윤의 선택입니다!

src/main/java/util/LineGenerator.java Show resolved Hide resolved
Comment on lines +11 to +17
public static void printLadder(List<Line> ladder, List<String> playerNames) {
System.out.println("실행 결과");
printPlayerNames(playerNames);
for (Line line : ladder) {
printLine(line);
}
}

Choose a reason for hiding this comment

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

Ladder라는 일급 컬렉션을 구현했음에도 List<Line>으로 매개변수를 전달한 이유가 있을까요?
또한, View에 직접 Domain을 전달했을 때는 어떤 문제가 일어날 수 있나요?

package model;

public class Player {
private final static int MAX_NAME_LENGTH = 5;

Choose a reason for hiding this comment

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

Java Language Specification에 따르면 private static final이 더 올바른 순서입니다!

Comment on lines +26 to +39
@Test
@DisplayName("사다리의 높이를 확인합니다.")
void ladderHeight() {
// given
int numberOfPlayers = 4;
int height = 5;

// when
Ladder ladder = new Ladder(numberOfPlayers, height);
List<Line> lines = ladder.getLines();

// then
assertEquals(height, lines.size());
}

Choose a reason for hiding this comment

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

해당 테스트는 "사다리의 높이를 확인합니다."라기보단 "주어진 높이에 따라 사다리를 생성한다."가 더 어울릴 것 같습니다.
테스트 코드가 일종의 기능 명세의 역할도 한다는 것을 생각하면 @DisplayName은 코드 이해를 위한 중요한 단서가 됩니다.

따라서, 모호한 표현보다는 좀 더 확실하게 해당 테스트가 어떤 기능을 테스트하고 있는지 명시해줘야 합니다.

Comment on lines +12 to +19
@Test
@DisplayName("라인을 생성합니다.")
void createLine() {
List<Point> points = List.of(new Point(true), new Point(false), new Point(true));
Line line = new Line(points);

assertThat(line.getPoints()).isEqualTo(points);
}

Choose a reason for hiding this comment

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

사다리 생성 시, 랜덤한 값의 영향을 받는 부분을 외부에서 주입하도록 해결했네요! 👍
랜덤 때문에 테스트하기 어려웠을 것 같은데 잘 하셨습니다!

Comment on lines +24 to +33
@Test
@DisplayName("플레이어의 이름이 5글자를 초과하면 예외가 발생합니다.")
void validateNameLength() {
// given
String name = "pobibibibi";

// when & then
assertThatThrownBy(() -> new Player(name))
.isInstanceOf(IllegalArgumentException.class);
}

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.

3 participants