Skip to content

[사다리 미션] 정상희 미션 제출합니다. #19

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 54 commits into
base: sangheejeong
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 33 commits
Commits
Show all changes
54 commits
Select commit Hold shift + click to select a range
1196afa
[FEAT] Ladder & Line 클래스 생성
SANGHEEJEONG Oct 24, 2024
8b96693
[FEAT] Line 생성 메서드 작성 (1단계)
SANGHEEJEONG Oct 24, 2024
f20be9f
[FEAT] LineView Enum 클래스 생성 (1단계)
SANGHEEJEONG Oct 24, 2024
19995ef
[REFACTOR] domain과 view 분리 (1단계)
SANGHEEJEONG Oct 24, 2024
49d105c
[FEAT] Ladder 생성 메서드 작성 (1단계)
SANGHEEJEONG Oct 24, 2024
090cb0f
[FEAT] OutputView 메서드 작성 및 Main 구현 (1단계)
SANGHEEJEONG Oct 24, 2024
06a00a7
[FEAT] InputView 메서드 작성 및 Main 구현 (2단계)
SANGHEEJEONG Oct 25, 2024
3e47a2a
[FEAT] 사다리 방향 확인 메서드 작성 (3단계)
SANGHEEJEONG Oct 26, 2024
07fe975
[FEAT] 사다리 게임 실행 메서드 작성 (3단계)
SANGHEEJEONG Oct 26, 2024
2265787
[FEAT] 사다리 게임 실행 메서드 수정 (3단계)
SANGHEEJEONG Oct 26, 2024
2fef17c
[FIX] 사다리 방향 결정 인덱스 오류 수정(3단계)
SANGHEEJEONG Oct 26, 2024
f7ab094
[FEAT] 4단계 입력 메서드 작성 (4단계)
SANGHEEJEONG Oct 26, 2024
dbbb328
[FEAT] Players 클래스 생성 (4단계)
SANGHEEJEONG Oct 26, 2024
65d58d2
[FEAT] GameResult 클래스 생성 및 OutputView 메서드 작성 (4단계)
SANGHEEJEONG Oct 26, 2024
56b093f
[FEAT] main 작성 (4단계)
SANGHEEJEONG Oct 26, 2024
5c6b21e
[REFACTOR] decideWhereToGo 메서드 분리 (5단계)
SANGHEEJEONG Oct 26, 2024
67fcc5a
[REFACTOR] Player 객체 분리 (5단계)
SANGHEEJEONG Oct 29, 2024
74f098d
[REFACTOR] LadderGenerator 메서드 수정 (5단계)
SANGHEEJEONG Oct 29, 2024
9763754
[REFACTOR] LadderGame forEach 람다 형식으로 바꿔보기(5단계)
SANGHEEJEONG Oct 29, 2024
a16726a
[REFACTOR] OutputView 메서드 순서 수정(5단계)
SANGHEEJEONG Oct 29, 2024
69871ca
[REFACTOR] findPlayerByName 메서드를 Players로 책임 분리(5단계)
SANGHEEJEONG Oct 30, 2024
169ec00
[REFACTOR] OutputView 주석 및 수정 (5단계)
SANGHEEJEONG Oct 30, 2024
ea543eb
[REFACTOR] Name, Position 원시값 포장 (5단계)
SANGHEEJEONG Oct 30, 2024
e81fa34
[REFACTOR] Name 예외처리 (5단계)
SANGHEEJEONG Oct 30, 2024
047d483
[REFACTOR] ResultTypes 예외처리 (5단계)
SANGHEEJEONG Oct 30, 2024
2dfe464
[REFACTOR] Players 예외처리 (5단계)
SANGHEEJEONG Oct 30, 2024
42cdbd6
[REFACTOR] 입력 예외처리 (5단계)
SANGHEEJEONG Oct 30, 2024
62f451d
[REFACTOR] 불변객체 적용 (5단계)
SANGHEEJEONG Oct 30, 2024
422af42
[REFACTOR] 단위테스트 작성 (5단계)
SANGHEEJEONG Oct 30, 2024
494e2d9
[REFACTOR] 코드 정렬 (5단계)
SANGHEEJEONG Oct 30, 2024
b2b1220
Create README.md
SANGHEEJEONG Nov 2, 2024
7dbb8cd
[REFACTOR] 리뷰 반영
SANGHEEJEONG Nov 2, 2024
67b09e4
[REFACTOR] README 작성
SANGHEEJEONG Nov 2, 2024
5c280c4
[REFACTOR] README 수정
SANGHEEJEONG Nov 6, 2024
8eac61b
[REFACTOR] Controller 메서드 분리
SANGHEEJEONG Nov 6, 2024
beeb626
[REFACTOR] 미사용 import 정리
SANGHEEJEONG Nov 6, 2024
570c4d7
[REFACTOR] LadderGame 메서드 책임에 따른 클래스 분리
SANGHEEJEONG Nov 7, 2024
6f6b09a
[REFACTOR] 접근지정자 수정 public -> private
SANGHEEJEONG Nov 7, 2024
95f8ec1
[REFACTOR] Boolaen -> Point ENUM 사용
SANGHEEJEONG Nov 8, 2024
f31cacc
[REFACTOR] position 메서드 수정
SANGHEEJEONG Nov 8, 2024
e43ee2d
[REFACTOR] 입력 메서드 수정
SANGHEEJEONG Nov 8, 2024
7e4cd52
[REFACTOR] LadderController 수정
SANGHEEJEONG Nov 9, 2024
f8dce70
[REFACTOR] LadderGenerator 삭제 및 Controller 메서드 분리
SANGHEEJEONG Nov 16, 2024
ac02d9c
[REFACTOR] moveLeft & moveRight 예외 처리
SANGHEEJEONG Nov 16, 2024
d48bce2
[REFACTOR] Controller 와 Application 책임 분리
SANGHEEJEONG Nov 18, 2024
ea12de3
[REFACTOR] Ladder 클래스 책임 부여
SANGHEEJEONG Nov 18, 2024
01b8efe
[REFACTOR] decideWhereToGo 메서드 line 클래스 이동
SANGHEEJEONG Nov 18, 2024
7cf78a6
[REFACTOR] Point -> LadderStep 이름 변경 및 학습테스트(익명 클래스 & 인터페이스) 적용
SANGHEEJEONG Nov 18, 2024
174ff32
[REFACTOR] nextStep 미사용 인자 삭제
SANGHEEJEONG Nov 19, 2024
6b26030
[REFACTOR] 동명이인 예외 처리
SANGHEEJEONG Nov 19, 2024
1d96fb1
[REFACTOR] PlayerName VO로 만들기
SANGHEEJEONG Nov 20, 2024
0bd1928
[REFACTOR] test 수정
SANGHEEJEONG Nov 20, 2024
76e1d31
[REFACTOR] test 수정
SANGHEEJEONG Nov 20, 2024
6473cca
[REFACTOR] 코드 정렬
SANGHEEJEONG Nov 20, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 33 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
# 기능 구현

사다리의 연결은 Boolean을 통해 표현
+ true면 "-----" 연결되었다는 뜻
+ false면 연결되지 않았다는 뜻

## **controller**
+ LadderController

## **domain**
+ Ladder
* List<Line> 사다리 전체를 표현하는 객체
+ LadderGame
* 사다리 게임 실행과 관련된 메서드를 담은 클래스
+ LadderGenerator
* 사다리 생성과 관련된 메서드를 담은 클래스
+ Line
* List<Boolean> 사다리 한 줄을 표현하는 객체
+ PlayerName
* 플레이어 이름을 원시값 포장한 객체
+ Player
* 플레이어 객체
+ Players
* 플레이어 리스트 일급 컬렉션
+ Positon
* 플레이어 위치를 원시값 포장한 객체
+ ResultTypes
* 결과 리스트 일급 컬렉션

## **View**
+ InputException
+ InputView
+ OutputView

Choose a reason for hiding this comment

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

[Request Change]

README를 작성하신 이유가 있을까요?

저는 상희님이 작성하신 패키지 목록만 봐도 위의 내용은 예상 가능합니다.
image

만약, Position 클래스의 이름을 Location으로 변경한다하면, README도 변경해줘야겠죠?
오히려 관리 포인트가 늘어나는 일인데요.

고민포인트

이렇게 번거러움에도 불구하고 사람들이 README를 작성하는 이유가 무엇일까요?

개선점

README가 읽는 대상이 누구일지 생각하고 더 이해하기 쉽게 작성하시면 좋을 것 같습니다

Copy link
Author

Choose a reason for hiding this comment

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

앗 리뷰어님이 말씀하신대로 제가 '읽는 대상'과 '쓰는 목적'에 대한 고민을 덜 한 채로 README를 작성한 것 같네요

많은 사람들의 README를 참고해서 고민한 결과로

  1. 구현할 기능을 스스로 작성함으로써 전체적인 틀을 잡는다
  2. 자신의 코드를 볼 사람들의 이해를 돕는다

이 두 가지가 가장 큰 목적인 것 같습니다.
물론 저는 README를 기능 구현 후에 작성했지만 다음 프로젝트에서는 구현 전에 작성해보는 게 좋겠네요! 수정해서 올렸는데 더 수정 및 보완할 부분이 있다면 알려주시면 감사하겠습니다.

18 changes: 18 additions & 0 deletions src/main/java/Application.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import controller.LadderController;
import domain.Ladder;
import domain.LadderGame;
import domain.LadderGenerator;
import domain.Players;
import domain.ResultTypes;
import view.OutputView;
import view.InputView;

Choose a reason for hiding this comment

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

[Request Change]

사용하지 않는 코드에 대한 import는 꼭 제거해주세요.

import는 클래스간 의존성을 한눈에 볼 수 있는 부분입니다.

IDE를 통해 commit을 할 때, import를 최적화 하는 방법도 있으니 검색해보시면 좋겠네요!!


import java.util.List;


public class Application {
public static void main(String[] args) {
LadderController ladderController = new LadderController();
ladderController.run();
}
}
43 changes: 43 additions & 0 deletions src/main/java/controller/LadderController.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
package controller;

import domain.Ladder;
import domain.LadderGame;
import domain.LadderGenerator;
import domain.Players;
import domain.ResultTypes;
import view.InputView;
import view.OutputView;

import java.util.List;

public class LadderController {
public void run() {
// 게임 로직 시작
LadderGame ladderGame = new LadderGame();

// 플레이어 생성
List<String> playerNames = InputView.splitString(InputView.inputNames());
Players players = new Players(playerNames);

// 결과 생성
List<String> kindOfResults = InputView.splitString(InputView.inputLadderResults());
ResultTypes resultTypes = new ResultTypes(kindOfResults, players.getPlayersSize());

// 사다리 생성
int width = players.getPlayersSize();
int height = InputView.inputHeight();

LadderGenerator ladderGenerator = new LadderGenerator();
Ladder ladder = ladderGenerator.createLadder(width, height);

// 사다리 출력
OutputView.printPlayers(playerNames);
OutputView.drawLadder(ladder);
OutputView.printResultTypes(resultTypes.getResultTypes());

// 게임 시작 및 결과 출력
ladderGame.runGame(ladder, players);
OutputView.printResult(players, resultTypes.getResultTypes());
}

Choose a reason for hiding this comment

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

[�Comment]

image
로또 미션 요구사항에서 위와같이 10줄이 넘어가지 말라는 요구사항이 있었는데요.

이 run 함수는 매우 기네요.
길어진 이유는, 주석을 달아주신 것 처럼 많은일을 하고 있어서인 것 같네요.

다른 분들도 Controller의 함수가 길어지는 현상을 간혹 봤는데요.

상희님만의 새로운 요구사항이 생길 수 있다 가정해볼까요?
image

네이버 사다리타기는 이렇게, 한명 한명 결과를 확인하거나, 전체 결과를 한번에 해결하거나 할 수 있죠.
지금 이 Controller 함수는 유저가 할 수 있는 시나리오 1개밖에 실행을 못하네요.
더 유연한 방법은 뭐가있을까요?

Copy link
Author

Choose a reason for hiding this comment

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

헉 규칙을 까먹어 버렸네요ㅜ
메서드를 분리함으로써 다양한 시나리오에 유동적으로 대처가 가능할 것 같습니다!

이거는 함수 분리랑 다른 내용이긴 한데 전부터 궁금했었어서 질문 드립니다
Application 클래스가 있는데 많은 사람들이 Application에 직접 메서드를 작성하지 않고 굳이 Controller를 따로 만드는 이유가 무엇인지 잘 모르겠습니다,,

Choose a reason for hiding this comment

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

글쎄요... 저도 잘모르겠어요!!

controller에 동작을 1개(run)만 정의하는 이유를 모르겠어요!!

controller는 게임의 흐름(run)을 결정하는 주체라고 생각하지 않아요.
입력(요청)을 모델에게 전달하거나 혹은 모델의 상태를 뷰가 출력하도록 전달(응답)하는 역할이라고 생각해요.

이에 대해서는 밑에서 더 다뤄보죠!! 아주 좋은 고민이네요


}
17 changes: 17 additions & 0 deletions src/main/java/domain/Ladder.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
package domain;

import java.util.Collections;
import java.util.List;

public class Ladder {

private final List<Line> lines;

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

public List<Line> getLadder() {
return Collections.unmodifiableList(lines);
}
}

Choose a reason for hiding this comment

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

[Comment]

getter 만 존재하는 일급컬렉션을 선언하신 이유가 있을까요?

Copy link
Author

Choose a reason for hiding this comment

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

List<Line>을 그냥 사용하는 것보다 일급 컬렉션으로 선언했을 때

  1. 관련된 로직을 직접 처리한다
  2. 각 컬렉션에 의미부여가 명확해진다

이 두 가지의 큰 장점이 생기는 것 같습니다.
근데 리뷰어님이 말씀하신대로 getter만 존재한다는 것은 장점1이 적용되지 않은 클래스라고 생각되네요! 다시 한 번 코드를 보면서 해당 클래스에서 처리할 메서드가 있다면 옮기도록 하겠습니다

Choose a reason for hiding this comment

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

[Comment]

여전히 허전하네요. 일급컬랙션으로 래핑할 이유가 없어보여요.

근데, 너무 중요한 도메인이죠? 이 미션이름이 **'사다리'**미션인데 어떻게 객체로 안만들어요.😭😭😭😭😭😭😭😭😭

그래도 아쉬운 마음에 조금 더 영업을 해보겠습니다.
저는 사다리 게임의 어떤 모양?을 상상하면
image
(출처: 네이버 사다리 게임)
image
(출처: 위키피디아)

사다리 꼭대기에 놓여진 players들이 너무 input 같지 않나요?🧐

Copy link
Author

Choose a reason for hiding this comment

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

저는 moveAllPlayers 메서드가 '플레이어들이 사다리 위를 움직인다'는 관점으로 봐서 1차 리뷰 수정 때 Players 클래스에 넣었는데 생각해보니까 '사다리' 게임인 만큼 리뷰어 님이 말해주신 저 관점으로 보면 Ladder 클래스에 넣는 게 더 적합할 수도 있겠네요

23 changes: 23 additions & 0 deletions src/main/java/domain/LadderGame.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
package domain;

public class LadderGame {

public void movePlayer(Line line, Player player) {
if (line.canMoveRight(player.getPosition())) {
player.moveRight();
return;
}

if (line.canMoveLeft(player.getPosition())) {
player.moveLeft();
}
}

public void moveEachPlayer(Ladder ladder, Player player) {
ladder.getLadder().forEach(line -> movePlayer(line, player));
}

Choose a reason for hiding this comment

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

[Request Change]

이 두 메서드는 테스트를 제외하곤 LadderGame 내부에서만 사용되네요.
private으로 변경해야할 것 같아요.

만약 이 로직이 test 대상처럼 느껴진다면, 고민해볼 필요가 있겠네요.

  1. 단위 테스트 대상은 어디까지?
  2. 이것만으로 테스트할 가치가 있는 내용이라면, 이것들을 결국 호출하는 runGame()이란 메서드가 너무 많은 책임을 갖고 있는 것 아닐까?

Copy link
Author

Choose a reason for hiding this comment

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

자료를 찾아보니 private 함수는 단위 테스트를 진행하지 않는 방향이 맞을 것 같아요

이 질문에 대한 답은 “어떻게든 private 메서드를 테스트하고 싶다면, 그 메서드는 private이면 안된다"다. 메서드를 public으로 바꿔도 될지 마음에 걸린다면, 이 메서드는 별도의 책임의 일부로서 원래는 다른 클래스에 들어있어야 했던 것이다.


public void runGame(Ladder ladder, Players players) {
players.getPlayers().forEach(player -> moveEachPlayer(ladder, player));
}

Choose a reason for hiding this comment

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

[Request Change]

디미터의 법칙을 들어보신 적이 있을까요?

Players 객체 생성 -> Players 객체에서 List get -> List를 순회하며 move를 한다
이런 과정이라면 그냥 쉽게 List나 Player를 파라미터로 넘겨주는 것이 좀 더 심플하지 않을까요?

디미터의 법칙에 대해서 한번 스터디 해보고, getter를 사용하는 것보다 객체에게 메시지를 던져서 동작하도록 하면 좋겠어요.

}
38 changes: 38 additions & 0 deletions src/main/java/domain/LadderGenerator.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
package domain;

import java.util.ArrayList;
import java.util.List;

import static java.lang.Boolean.TRUE;

public class LadderGenerator {

public boolean randomTrueOrFalse() {
return Math.random() < 0.5; // 0.0(포함) - 1.0(미포함) 사이의 랜덤한 실수 반환
}

public boolean createValue(List<Boolean> line, int lineIndex) {
if (lineIndex != 0 && line.get(lineIndex - 1) == TRUE) // 이전 값이 true 면 false 반환
return false;

Choose a reason for hiding this comment

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

[Comment]

Suggested change
if (lineIndex != 0 && line.get(lineIndex - 1) == TRUE) // 이전 값이 true 면 false 반환
return false;
if (lineIndex != 0 && line.get(lineIndex - 1)) {
return false;
}
  1. Boolean.TRUE와 비교하는 것은 없어도 되는 영역아닐까요?
  2. if문 등에 코드 블럭은 넣어보면 어떨까요? 많은 java 컨벤션에서 그것을 규칙으로 하고 있습니다.

Choose a reason for hiding this comment

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

[Comment]

사실 이 코드가 랜덤 사다리 생성 시, 매우매우 중요한 로직으로 보여지는데요.

false가 무엇이고, true가 무슨 의미인지 알기 어렵네요...
더 좋은 방법이 있을 것 같은데요.
이는 Line 클래스에서 조금 더 살펴보겠습니다.

Copy link
Author

Choose a reason for hiding this comment

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

  1. 없어도 상관없지만 가독성을 위해 Boolean.TRUE를 적었습니다 && 연산자로 엮여 있기도 하고 line이 Boolean 값으로 이뤄진 List라는 것을 바로 보여주기에 편한 것 같아서요!
  2. 넵 실수로 지워진 채로 냅둔 것 같네요 수정했습니다


Choose a reason for hiding this comment

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

[Request Change]

내부에서만 사용하는 코드는 접근지정자를 private으로 변경해주세요

return randomTrueOrFalse();
}

public Line createLine(int width) {
List<Boolean> points = new ArrayList<>();
for (int i = 0; i < width - 1; i++) {
points.add(createValue(points, i));
}

return new Line(points);
}

public Ladder createLadder(int width, int height) {
List<Line> lines = new ArrayList<>();
for (int i = 0; i < height; i++) {
lines.add(createLine(width));
}

return new Ladder(lines);
}

Choose a reason for hiding this comment

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

[Comment]

상희님은 왜 Ladder를 생성하는 로직을 Ladder 스스로에게 부여하지 않고 Factory로 빼셨을까요?

어떤 장단이 있어서 빼신것인지 생각을 들려주세요.

Factory로 빼는 것은 Ladder에 대한 생성 로직을 Ladder 클래스만 봐선 알기 힘들다는 단점이 있을 것 같은데요.

Copy link
Author

Choose a reason for hiding this comment

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

Boolean값이 모여 Line을 만들고 Line이 모여 Ladder클래스를 만들어 결국 사다리가 완성되는 것이기 때문에 가시성의 측면에서 이를 한 곳에 모음으로써 사다리 생성의 전체 과정을 쉽게 파악하고자 했습니다.

또한 사다리 생성 방식에 변화가 있을 경우에 대처하기가 쉽고 각 클래스의 생성 로직과 기능을 분리함으로써 책임 분리를 할 수 있다는 것이 장점이라고 생각합니다.

Choose a reason for hiding this comment

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

[Comment]

확실히 생성 로직이 복잡하고 그 변경사항이 다른 코드들과 협력이 많이 필요하거나 외부 인프라 등을 활용해야할 때 외부에 팩터리를 두는 것이 유리할때가 있죠.

하지만, 저는 여전히 해당 createLadder(29번-36번)은 그리 복잡해보이지 않은 로직이라, Ladder안에 정적팩터리매서드로 존재해도 좋다고 생각해요.

그렇게 한다면, 위에 Line과 Point를 만드는 로직 또한 각각의 클래스 내부로 넣어야겠죠.

사다리 생성 방식에 변화가 있을 경우에 대처하기가 쉽고

라는 의견이 틀렸다는 말은 아니지만, 저는 꼭 그렇진 않다고 생각해요. 만약 제가 상희님 후임자로 대신 이 프로그램을 유지보수한다고 생각했을 때, 사다리 생성 요구사항에 변경이 생기면

  1. 무조건 class Ladder 부터 찾아본다.
  2. Ladder 생성자가 쓰이는 곳을 찾아본다.
    이 순서로 생성 로직을 볼 것 같아요.

속는 셈치고 루카 훈수 따라보기

  • 현재 상황의 고민 점
    • 이렇게 팩터리 패턴을 유지한다 했을 때, 해당 클래스는 유틸 클래스여야하는가?
  • 훈수 들어보기
    • 각 Ladder, Line, Point의 생성 로직을 해당 클래스로 넣어보기
    • Point에 대해서는 해당 코드에서 좀 더 자세히 말씀드리겠습니다

}
26 changes: 26 additions & 0 deletions src/main/java/domain/Line.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
package domain;

import java.util.Collections;
import java.util.List;

import static java.lang.Boolean.TRUE;

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

Choose a reason for hiding this comment

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

[Request Change]

Boolean은 어떤 의미를 갖는 값인가요.
Boolean은 boolean을 감싸는 래퍼클래스이긴 한지만, true / false를 나타내는 원시타입으로서 많이 사용하는 것 같아요.

과연 Boolean을 100명에게 보여줬을 때, 100명 모두에게

true는 다리를 건널 수 있는 값이고 false는 다리를 건너지 못하는 값이야!!

이를 납득시킬 수 있으신가요?
납득시킨다 해도 제가 이렇게 리뷰를 달고 있는 것 처럼 상희님의 동료 개발자가 이것이 무슨 의미인지 물어볼려고 상희님을 찾아올 수 있겠네요.

더 의미있는 값으로 변경하기에 좋은 방법이 뭐가 있을까요?
이는 사다리미션 - 학습 테스트(함수형 프로그래밍)를 스터디해보고 개선할 점을 찾으셨으면 좋겠요. (꼭 이 방법이 아니라도 바꿀 방법은 많겠죠?)


public Line(List<Boolean> points) {
this.points = points;
}

public boolean canMoveRight(int ladderOrder) {
return (ladderOrder < points.size()) && (points.get(ladderOrder) == TRUE);
}

public boolean canMoveLeft(int ladderOrder) {
return (ladderOrder != 0) && (points.get(ladderOrder - 1) == TRUE);
}

public List<Boolean> getLine() {
return Collections.unmodifiableList(points);
}
}
28 changes: 28 additions & 0 deletions src/main/java/domain/Player.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
package domain;

public class Player {

private final PlayerName playerName;
private final Position position;

public Player(PlayerName name, Position position) {
this.playerName = name;
this.position = position;
}

public void moveLeft() {
position.movePositionLeft();
}

public void moveRight() {
position.movePositionRight();
}

public String getPlayerName() {
return playerName.getName();
}

public int getPosition() {
return position.getPosition();
}
}
41 changes: 41 additions & 0 deletions src/main/java/domain/PlayerName.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
package domain;

public class PlayerName {

private final String name;
private final static int MAX_LENGTH = 5;
private final static String INVALID_NAME = "all";
Comment on lines +6 to +7

Choose a reason for hiding this comment

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

[Approve]

객체의 규칙을 한눈에 볼 수 있는 상수 정말 좋은데요? 👍


public PlayerName(String name) {
validateName(name);
this.name = name;
}

private void validateName(String name) {
validateMaxLength(name);
validateNotBlank(name);
validateNotEqualAll(name);
}

private void validateNotBlank(String name) {
if (name.isBlank()) {
throw new IllegalArgumentException("이름은 공백일 수 없습니다.");
}
}

private void validateMaxLength(String name) {
if (name.length() > MAX_LENGTH) {
throw new IllegalArgumentException("이름은 5자를 초과할 수 없습니다.");
}
}

private void validateNotEqualAll(String name) {
if (name.equals(INVALID_NAME)) {
throw new IllegalArgumentException("이름은 all일 수 없습니다.");
}
}

Choose a reason for hiding this comment

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

[Approve]

Suggested change
private void validateMaxLength(String name) {
if (name.length() > MAX_LENGTH) {
throw new IllegalArgumentException("이름은 5자를 초과할 수 없습니다.");
}
}
private void validateNotEqualAll(String name) {
if (name.equals(INVALID_NAME)) {
throw new IllegalArgumentException("이름은 all일 수 없습니다.");
}
}
private void validateMaxLength(String name) {
if (name.length() > MAX_LENGTH) {
throw new IllegalArgumentException("이름은 " + MAX_LENGTH + "자를 초과할 수 없습니다.");
}
}
private void validateNotEqualAll(String name) {
if (name.equals(INVALID_NAME)) {
throw new IllegalArgumentException("이름은 " + INVALID_NAME + "일 수 없습니다.");
}
}


public String getName() {
return name;
}
}
47 changes: 47 additions & 0 deletions src/main/java/domain/Players.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
package domain;

import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.NoSuchElementException;

public class Players {

private final List<Player> players;
private final static int MIN_PLAYER_SIZE = 2;

public Players(List<String> playerNames) {
validateSize(playerNames);
this.players = createPlayer(playerNames);
}

private void validateSize(List<String> playerNames) {
if (playerNames.size() < MIN_PLAYER_SIZE) {
throw new IllegalArgumentException("플레이어 수는 2명 이상이어야 합니다.");
}
}

private List<Player> createPlayer(List<String> playerNames) {
List<Player> players = new ArrayList<>();
for (int i = 0; i < playerNames.size(); i++) {
players.add(new Player(new PlayerName(playerNames.get(i)), new Position(i)));
}

return players;
}

public Player findByName(String viewerName) {
return players.stream()
.filter(player -> player.getPlayerName().equals(viewerName))

Choose a reason for hiding this comment

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

[Request Chnage]

PlayerName을 VO로서 사용해보면 어떨까요? (실제로도 거의 그렇게 사용되고 있음)
그렇다면 Player는?

고민점

  1. 값 객체(VO)는 어떨때 같은 객체일까? 엔티티는 언제 같은 대상일까? 를 고민해보고 playerName과 player의 equals hascode를 재정의해주세요
  2. 이름이 같다고 같은 사람이라고 할 때 이 게임의 문제는 안생길까?

Copy link
Author

Choose a reason for hiding this comment

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

값 객체라는 키워드를 처음 접해봤는데요, 찾아본 결과
엔티티와 값 객체의 차이점은
불변성과 목적 측면에서 차이가 큰 것 같네요

PlayerName은 불변성의 측면에서 한 번 생성되면 내부 값이 변경되지 않는다는 특징이 있고 고유 식별자가 없어 객체 자체 보다는 내부 속성에 의미가 있기 때문에 VO로 사용하면 좋을 것 같습니다.

하지만 Player는 요구사항에 따라 내부 속성이 변경될 가능성이 크고 객체 자체가 도메인에서 의미가 있는 대상이니까 값 객체 보다는 엔티티의 역할을 하는 것이 맞는 것 같습니다.

  1. playerName에 재정의 추가했습니다
  2. 이름 중복 예외 처리 추가했습니다!

.findFirst()
.orElseThrow(() -> new NoSuchElementException("플레이어 이름 '" + viewerName + "' 이 존재하지 않습니다."));
}

public List<Player> getPlayers() {
return Collections.unmodifiableList(players);
}
Comment on lines +53 to +55

Choose a reason for hiding this comment

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

[Approve]

방어적 복사 👍👍


public int getPlayersSize() {
return players.size();
}
}
22 changes: 22 additions & 0 deletions src/main/java/domain/Position.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
package domain;

public class Position {

private int position;

public Position(int position) {
this.position = position;
}

public void movePositionLeft() {
position--;
}

Choose a reason for hiding this comment

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

[Request Change]

position이 0인 객체의 이 메서드가 호출되지 않을 것이란 보장이 있을까요? 🧐🧐🧐🧐
그렇게된다면 심각한 버그가 발생할텐데요.

일단 저는 저를 포함한 동료개발자들을 그리 믿진 않아요.


public void movePositionRight() {
position++;
}

public int getPosition() {
return position;
}
Comment on lines +25 to +27

Choose a reason for hiding this comment

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

[Comment]

position을 getter로 꺼내는 방식 보다 조금 더 이 값에 대해서 판단을 이 Position이란 객체에게 맡겨보는 것은 어떨까요?

위의 리뷰들을 참고해서 한번 변경해보면 좋을 것 같아요

}
34 changes: 34 additions & 0 deletions src/main/java/domain/ResultTypes.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
package domain;

import java.util.List;

public class ResultTypes {

private final List<String> resultTypes;

public ResultTypes(List<String> resultTypes, int width) {
validate(resultTypes, width);
this.resultTypes = resultTypes;
}

private void validate(List<String> resultTypes, int width) {
resultTypes.forEach(this::validateNotBlank);
validateSize(resultTypes, width);
}

private void validateNotBlank(String resultType) {
if (resultType.isBlank()) {
throw new IllegalArgumentException("실행 결과는 공백일 수 없습니다.");
}
}

private void validateSize(List<String> resultTypes, int width) {
if (resultTypes.size() != width) {
throw new IllegalArgumentException("실행 결과는 사다리의 개수와 일치해야 합니다.");
}
}

public List<String> getResultTypes() {
return resultTypes;
}
}
Comment on lines +5 to +34

Choose a reason for hiding this comment

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

[�Comment]

이 List은 List 같이 의도가 분명한 Wrapper클래스로 감싸지 않은 이유가 있나요?

Copy link
Author

Choose a reason for hiding this comment

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

Result에 대한 속성이 단순히 문자형 결과이고 이에 따른 책임이 유효성 검증밖에 없다고 생각해서 굳이 나누지 않았습니다

Choose a reason for hiding this comment

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

[Comment]

사실 위에서 한 유효성 검증은 ResultType 하나의 요소에 대한 검증이지, List 자체에 대한 검증은 아닌 것 같기도 하네요.

그치만, 저도 상희님이 말씀하신 것 처럼 꼭 래핑할 이유는 없다고 생각해요 해당 클래스만으로도 충분히 의미전달이 된다고 생각합니다

16 changes: 16 additions & 0 deletions src/main/java/view/InputException.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
package view;

public class InputException {

Choose a reason for hiding this comment

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

[Comment]

Input에 대한 Validate를 따로 분리시킨 이유가 있을까요?

Copy link
Author

Choose a reason for hiding this comment

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

원래는 Input에 대한 예외가 많을 줄 알고 나눴었는데 2개 밖에 찾아내지 못해서 그냥 합치는 게 나을 것 같네요 수정했습니다!


public static void validateHeightSize(int height) {
if (height < 1) {
throw new IllegalArgumentException("사다리 높이는 1 이상이어야 합니다.");
}
}

public static void validateViewerNameNotBlank(String viewerName) {
if (viewerName.isBlank()) {
throw new IllegalArgumentException("이름은 공백일 수 없습니다.");
}
}
}
Loading