-
Notifications
You must be signed in to change notification settings - Fork 39
[사다리 미션] 정상희 미션 제출합니다. #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
base: sangheejeong
Are you sure you want to change the base?
Changes from 33 commits
1196afa
8b96693
f20be9f
19995ef
49d105c
090cb0f
06a00a7
3e47a2a
07fe975
2265787
2fef17c
f7ab094
dbbb328
65d58d2
56b093f
5c6b21e
67fcc5a
74f098d
9763754
a16726a
69871ca
169ec00
ea543eb
e81fa34
047d483
2dfe464
42cdbd6
62f451d
422af42
494e2d9
b2b1220
7dbb8cd
67b09e4
5c280c4
8eac61b
beeb626
570c4d7
6f6b09a
95f8ec1
f31cacc
e43ee2d
7e4cd52
f8dce70
ac02d9c
d48bce2
ea12de3
01b8efe
7cf78a6
174ff32
6b26030
1d96fb1
0bd1928
76e1d31
6473cca
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 | ||
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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||
} | ||
} |
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()); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [�Comment]
이 run 함수는 매우 기네요. 다른 분들도 Controller의 함수가 길어지는 현상을 간혹 봤는데요. 상희님만의 새로운 요구사항이 생길 수 있다 가정해볼까요? 네이버 사다리타기는 이렇게, 한명 한명 결과를 확인하거나, 전체 결과를 한번에 해결하거나 할 수 있죠. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 헉 규칙을 까먹어 버렸네요ㅜ 이거는 함수 분리랑 다른 내용이긴 한데 전부터 궁금했었어서 질문 드립니다 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 글쎄요... 저도 잘모르겠어요!! controller에 동작을 1개(run)만 정의하는 이유를 모르겠어요!! controller는 게임의 흐름(run)을 결정하는 주체라고 생각하지 않아요. 이에 대해서는 밑에서 더 다뤄보죠!! 아주 좋은 고민이네요 |
||
|
||
} |
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); | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Comment] getter 만 존재하는 일급컬렉션을 선언하신 이유가 있을까요? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
이 두 가지의 큰 장점이 생기는 것 같습니다. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 저는 moveAllPlayers 메서드가 '플레이어들이 사다리 위를 움직인다'는 관점으로 봐서 1차 리뷰 수정 때 Players 클래스에 넣었는데 생각해보니까 '사다리' 게임인 만큼 리뷰어 님이 말해주신 저 관점으로 보면 Ladder 클래스에 넣는 게 더 적합할 수도 있겠네요 |
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)); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Request Change] 이 두 메서드는 테스트를 제외하곤 LadderGame 내부에서만 사용되네요. 만약 이 로직이 test 대상처럼 느껴진다면, 고민해볼 필요가 있겠네요.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 자료를 찾아보니 private 함수는 단위 테스트를 진행하지 않는 방향이 맞을 것 같아요
|
||
|
||
public void runGame(Ladder ladder, Players players) { | ||
players.getPlayers().forEach(player -> moveEachPlayer(ladder, player)); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Request Change] 디미터의 법칙을 들어보신 적이 있을까요? Players 객체 생성 -> Players 객체에서 List get -> List를 순회하며 move를 한다 디미터의 법칙에 대해서 한번 스터디 해보고, getter를 사용하는 것보다 객체에게 메시지를 던져서 동작하도록 하면 좋겠어요. |
||
} |
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(미포함) 사이의 랜덤한 실수 반환 | ||||||||||||
SANGHEEJEONG marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||
} | ||||||||||||
|
||||||||||||
public boolean createValue(List<Boolean> line, int lineIndex) { | ||||||||||||
if (lineIndex != 0 && line.get(lineIndex - 1) == TRUE) // 이전 값이 true 면 false 반환 | ||||||||||||
return false; | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Comment]
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Comment] 사실 이 코드가 랜덤 사다리 생성 시, 매우매우 중요한 로직으로 보여지는데요. false가 무엇이고, true가 무슨 의미인지 알기 어렵네요... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||||||||
|
||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||||||||||||
} | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Comment] 상희님은 왜 Ladder를 생성하는 로직을 Ladder 스스로에게 부여하지 않고 Factory로 빼셨을까요? 어떤 장단이 있어서 빼신것인지 생각을 들려주세요. Factory로 빼는 것은 Ladder에 대한 생성 로직을 Ladder 클래스만 봐선 알기 힘들다는 단점이 있을 것 같은데요. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Boolean값이 모여 Line을 만들고 Line이 모여 Ladder클래스를 만들어 결국 사다리가 완성되는 것이기 때문에 가시성의 측면에서 이를 한 곳에 모음으로써 사다리 생성의 전체 과정을 쉽게 파악하고자 했습니다. 또한 사다리 생성 방식에 변화가 있을 경우에 대처하기가 쉽고 각 클래스의 생성 로직과 기능을 분리함으로써 책임 분리를 할 수 있다는 것이 장점이라고 생각합니다. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Comment] 확실히 생성 로직이 복잡하고 그 변경사항이 다른 코드들과 협력이 많이 필요하거나 외부 인프라 등을 활용해야할 때 외부에 팩터리를 두는 것이 유리할때가 있죠. 하지만, 저는 여전히 해당 createLadder(29번-36번)은 그리 복잡해보이지 않은 로직이라, Ladder안에 정적팩터리매서드로 존재해도 좋다고 생각해요. 그렇게 한다면, 위에 Line과 Point를 만드는 로직 또한 각각의 클래스 내부로 넣어야겠죠.
라는 의견이 틀렸다는 말은 아니지만, 저는 꼭 그렇진 않다고 생각해요. 만약 제가 상희님 후임자로 대신 이 프로그램을 유지보수한다고 생각했을 때, 사다리 생성 요구사항에 변경이 생기면
속는 셈치고 루카 훈수 따라보기
|
||||||||||||
} |
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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Request Change] Boolean은 어떤 의미를 갖는 값인가요. 과연 Boolean을 100명에게 보여줬을 때, 100명 모두에게
이를 납득시킬 수 있으신가요? 더 의미있는 값으로 변경하기에 좋은 방법이 뭐가 있을까요? |
||
|
||
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); | ||
} | ||
SANGHEEJEONG marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} |
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(); | ||
} | ||
} |
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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일 수 없습니다."); | ||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Approve]
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||
public String getName() { | ||||||||||||||||||||||||||||||||||||||||||||||
return name; | ||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||
} |
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)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Request Chnage] PlayerName을 VO로서 사용해보면 어떨까요? (실제로도 거의 그렇게 사용되고 있음) 고민점
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 값 객체라는 키워드를 처음 접해봤는데요, 찾아본 결과 PlayerName은 불변성의 측면에서 한 번 생성되면 내부 값이 변경되지 않는다는 특징이 있고 고유 식별자가 없어 객체 자체 보다는 내부 속성에 의미가 있기 때문에 VO로 사용하면 좋을 것 같습니다. 하지만 Player는 요구사항에 따라 내부 속성이 변경될 가능성이 크고 객체 자체가 도메인에서 의미가 있는 대상이니까 값 객체 보다는 엔티티의 역할을 하는 것이 맞는 것 같습니다.
|
||
.findFirst() | ||
.orElseThrow(() -> new NoSuchElementException("플레이어 이름 '" + viewerName + "' 이 존재하지 않습니다.")); | ||
} | ||
|
||
public List<Player> getPlayers() { | ||
return Collections.unmodifiableList(players); | ||
} | ||
Comment on lines
+53
to
+55
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Approve] 방어적 복사 👍👍 |
||
|
||
public int getPlayersSize() { | ||
return players.size(); | ||
} | ||
} |
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--; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Comment] position을 getter로 꺼내는 방식 보다 조금 더 이 값에 대해서 판단을 이 Position이란 객체에게 맡겨보는 것은 어떨까요? 위의 리뷰들을 참고해서 한번 변경해보면 좋을 것 같아요 |
||
} |
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [�Comment] 이 List은 List 같이 의도가 분명한 Wrapper클래스로 감싸지 않은 이유가 있나요? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Result에 대한 속성이 단순히 문자형 결과이고 이에 따른 책임이 유효성 검증밖에 없다고 생각해서 굳이 나누지 않았습니다 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Comment] 사실 위에서 한 유효성 검증은 ResultType 하나의 요소에 대한 검증이지, List 자체에 대한 검증은 아닌 것 같기도 하네요. 그치만, 저도 상희님이 말씀하신 것 처럼 꼭 래핑할 이유는 없다고 생각해요 해당 클래스만으로도 충분히 의미전달이 된다고 생각합니다 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
package view; | ||
|
||
public class InputException { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Comment] Input에 대한 Validate를 따로 분리시킨 이유가 있을까요? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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("이름은 공백일 수 없습니다."); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Request Change]
README를 작성하신 이유가 있을까요?
저는 상희님이 작성하신 패키지 목록만 봐도 위의 내용은 예상 가능합니다.

만약, Position 클래스의 이름을 Location으로 변경한다하면, README도 변경해줘야겠죠?
오히려 관리 포인트가 늘어나는 일인데요.
고민포인트
이렇게 번거러움에도 불구하고 사람들이 README를 작성하는 이유가 무엇일까요?
개선점
README가 읽는 대상이 누구일지 생각하고 더 이해하기 쉽게 작성하시면 좋을 것 같습니다
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
앗 리뷰어님이 말씀하신대로 제가 '읽는 대상'과 '쓰는 목적'에 대한 고민을 덜 한 채로 README를 작성한 것 같네요
많은 사람들의 README를 참고해서 고민한 결과로
이 두 가지가 가장 큰 목적인 것 같습니다.
물론 저는 README를 기능 구현 후에 작성했지만 다음 프로젝트에서는 구현 전에 작성해보는 게 좋겠네요! 수정해서 올렸는데 더 수정 및 보완할 부분이 있다면 알려주시면 감사하겠습니다.