Conversation
* docs: 기능목록 작성 * test: 사람 이름 글자 수 검증 테스트 * feat: Person 생성 시, 이름 검증 로직 구현 * test: 사람 이름 중복 검증 테스트 * feat: 사람 이름 중복 검증 로직 구현 * fix: Person 접근제어자 수정 * test: Ladder 생성 테스트 * feat: 사다리 틀 생성 * test: (사람 수 - 1) 만큼 다리 칸 생성 테스트 * feat: 사람 수 - 1 만큼 다리 생성 * feat: 각 칸마다 사다리 생성 여부는 랜덤값으로 결정 * test: BooleanGenerator 결과에 따른 다리 생성 테스트 * feat: 양 옆 다리 여부 검증 후 다리 생성 * test: 사용자 입력 높이가 2 이상 인지 검증 테스트 * feat: Height 클래스 (원시값 포장) 생성 * test: 사다리 생성 테스트 * feat: 사다리 생성 구현 * refactor: 메소드 분리 * test: 가장 이름 긴 사용자 반환 테스트 * feat: 가장 긴 이름 길이 반환 메소드 구현 * feat: 게임 전체 흐름 구현 * feat: 입출력 구현 * fix: 다리가 1칸일 때 예외 처리 * refactor: Height의 성공 테스트 수정 * refactor: 의미없는 @ParameterizedTest 제거 * refactor: boolean을 Bridge정보를 담은 enum으로 수정 * refactor: Line에서 양 옆의 Bridge 유무를 판단하는 로직의 메소드명 수정 * refactor: UnmodifiableList로 수정 * refactor: LadderController를 LadderGameController로 수정 및 README 수정
1. isNameDuplicate stream().distinct().count()대신 Set사용 2. findPersonNameInPosition for문 대신 stream사용
greeng00se
reviewed
Feb 27, 2023
| public class Height { | ||
| private static final int MIN_HEIGHT = 2; | ||
| private static final int MAX_HEIGHT = 100; | ||
| private final int height; |
src/main/java/domain/Line.java
Outdated
| } | ||
|
|
||
| private boolean hasBridgeInLeftOrRight(int bridgeIndex) { | ||
| if (bridgeIndex == 0 && lineSize == 1) { |
There was a problem hiding this comment.
매직넘버를 상수로 분리해보는 건 어떨까요?
추가로 lineSize가 필요할까요?
|
|
||
| public class Person { | ||
| private static final int MAX_NAME_LENGTH = 5; | ||
| private static final int MIN_NAME_LENGTH = 1; |
src/main/java/view/InputView.java
Outdated
| private static final String REQUEST_NAME = "참여할 사람 이름을 입력하세요. (이름은 쉼표(,)로 구분하세요)"; | ||
| private static final String REQUEST_LADDER_HEIGHT = "\n최대 사다리 높이는 몇 개인가요?"; | ||
| private static final String REQUEST_LADDER_PRIZE = "\n실행 결과를 입력하세요. (결과는 쉼표(,)로 구분하세요)"; | ||
| private static final String REQUEST_SHOWING_NAME = "\n결과를 보고 싶은 사람은?"; |
There was a problem hiding this comment.
\n의 경우 OS마다 다르게 동작할 수 있을 것 같은데 System.lineSeparator()와 같은 방법을 사용하는건 어떨까요?
src/main/java/view/OutputView.java
Outdated
| private int printPersons(List<String> names) { | ||
| String firstName = names.remove(0); | ||
| System.out.print(BLANK + firstName); | ||
| names.forEach(name -> System.out.printf(String.format("%%%ds", BRIDGE_SIZE + 1), name)); |
There was a problem hiding this comment.
해당 포맷이 어떤 의미를 가지고 있는지 한 번에 알아보기가 어려운 것 같아요. NAME_MESSAGE_FORMAT과 같이 상수로 분리할 수 있을 것 같아요!
| //when | ||
| ladder.generate(new TestGenerator( | ||
| List.of(true, false, true, false, | ||
| true, false, true, false))); |
There was a problem hiding this comment.
사소한 부분이긴한데 TestGenerator도 given으로 빼보는게 어떨까요?
src/test/java/domain/LineTest.java
Outdated
|
|
||
| //when | ||
| Assertions.assertThat(line.hasBridgeInLeft(1)).isTrue(); | ||
| Assertions.assertThat(line.hasBridgeInLeft(2)).isFalse(); |
There was a problem hiding this comment.
Assertions가 static import된 부분도 있고, 안된 부분도 있는 것 같아요. 안된 부분도 static import를 해보는건 어떨까요?
| Persons persons = Persons.from(names); | ||
|
|
||
| //then | ||
| Assertions.assertThat(persons.getAllPersonPosition()).isEqualTo(List.of(0, 1, 2)); |
src/main/java/domain/Persons.java
Outdated
|
|
||
| public String findPersonNameInPosition(int position) { | ||
| return persons.stream() | ||
| .filter(person -> person.getPosition() == position) |
There was a problem hiding this comment.
person의 값을 가져와서 비교하기 보다 메시지를 던지도록 해보는건 어떨까요?
| if (line.hasBridgeInLeft(position)) { | ||
| position--; | ||
| } | ||
| } |
There was a problem hiding this comment.
1depth가 요구사항이니 메서드 분리를 통해 이를 해결해보는건 어떨까요?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.