Open
Conversation
catsbi
requested changes
Dec 13, 2023
catsbi
left a comment
catsbi
left a comment
There was a problem hiding this comment.
안녕하세요 형재님, 사다리게임 3단계 미션 잘 구현해주셨어요 👍
몇가지 피드백 남겨드렸으니 확인 후 다시 리뷰요청 부탁드려요~
| lines.initialize(names.size(), height, new RandomLadderPoint()); | ||
|
|
||
| OutputView.printNames(names); | ||
| OutputView.printLadders(lines); |
There was a problem hiding this comment.
참가자 수와 결과 수의 갯수가 다른 경우(유효성이 안맞는 경우) 잘못된 사다리를 만들기전에 유효성 검증을 하고 출력하지 않도록 하면 어떨까요? 현재는 다음과 같이 노출되네요!
Comment on lines
9
to
10
| private Names names; | ||
| private Lines lines; |
| private void initialize(Height height, GenerateLadderPoint generateLadderPoint) { | ||
| this.lines = createLines(height, generateLadderPoint); | ||
| this.lines = lines; | ||
| validateResults(results); |
There was a problem hiding this comment.
유효성검증은 항상 최상단에 위치시키는걸 권장드려요. 이펙티브 자바에서도 말하지만, 유효성검증은 비즈니스 로직에서 항상 최우선하라고 하거든요
| import java.util.stream.Collectors; | ||
| import java.util.stream.IntStream; | ||
|
|
||
| public class LadderGame { |
| return position; | ||
| } | ||
|
|
||
| private List<Line> createLines(int size, Height height, GenerateLadderPoint generateLadderPoint) { |
There was a problem hiding this comment.
사용자가 매번 new로 객체를 생성하고 초기화로직을 실행시키는 것과
정적 팩토리 함수를 활용하는것중 어떤게 좀 더 나을까요?
Comment on lines
+28
to
+32
| if (inputName.getName().equals(PARTICIPANT_NAME_ALL)) { | ||
| printAllParticipantResults(ladderResults); | ||
| } else { | ||
| printSingleParticipantResult(inputName, ladderResults); | ||
| } |
| System.out.println(output); | ||
| private static void printAllParticipantResults(LadderResults ladderResults) { | ||
| ladderResults.getAllResults().forEach((participant, result) -> | ||
| System.out.println(participant.getName() + " : " + result.getResult())); |
There was a problem hiding this comment.
StringBuilder를 활용해서 무분별한 System.out을 줄여보면 어떨까요?
| new LadderResult(null); | ||
| }); | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
현재 테스트를 전체적으로 확인해봤는데,
- LadderGame : 전체적으로 사용하지 않는 함수 제거 필요, private 함수들에 대한 테스트 없음(private 함수를 직접 호출하라는게 아니라 public함수에서 분기검증이 되지 않았다는 의미입니다. )
- LadderResult( with LadderResults): 일급 컬렉션 생성자 테스트 및 사다리 결과 조회 테스트 부족
- Line : 전체적으로 테스트 부족
- Lines: getter와 move에 대한 테스트 부족
- Name, Names: getter 테스트 및 불필요 함수 존재
TDD인만큼 테스트에 좀 더 신경써보는건 어떨까요?
|
|
||
| class LadderResultTest { | ||
| @Test | ||
| @DisplayName("결과가 입력되지않으면 예외가 발생한다") |
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.
리뷰 요청드립니다 !!