Conversation
testrace
left a comment
testrace
left a comment
There was a problem hiding this comment.
안녕하세요 동인님 😃
2단계 구현 잘해주셨네요 👍
몇 가지 코멘트 남겨두었는데 확인해 주시고 다시 리뷰 요청해 주세요.
| import java.util.Random; | ||
|
|
||
| public class HorizontalLine { | ||
| private final List<Boolean> hasLineOrNotList; |
There was a problem hiding this comment.
변수명이 쉽지 않네요 😅
조금 더 간단하게 표현하면 어떨까요?
| private HorizontalLine(List<Boolean> hasLineOrNotList) { | ||
| this.hasLineOrNotList = hasLineOrNotList; | ||
| } |
There was a problem hiding this comment.
가로라인은 겹칠 수 없다는 제약을 지키기 위해 할당하기 전에 검증하면 어떨까요?
| public HorizontalLine(int userCount) { | ||
| this(generateLines(userCount)); | ||
| } |
There was a problem hiding this comment.
관례적으로 부 생성자는 주 생성자보다 상위에 위치합니다.
로직이 필요하다면 생성자가 아닌 정적 팩토리 메서드를 활용하면 어떨까요?
| } | ||
|
|
||
| private static List<Boolean> generateLines(int userCount) { | ||
| Random random = new Random(); |
There was a problem hiding this comment.
이전 단계에서도 경험하셨겠지만 random은 테스트하기 어려운 구조로 만들게 됩니다.
랜덤에 직접 의존하지 않는 구조로 변경해 보시면 좋을 것 같아요.
| if (name == null || name.length() > MAX_NAME_LENGTH) { | ||
| throw new IllegalArgumentException("참여자의 이름은 최대 5글자입니다"); |
There was a problem hiding this comment.
null인 경우는 메시지가 달라야 하지 않을까요?
빈 값인 경우에 대한 예외처리도 추가하면 좋을 것 같아요.
| private static int getInputInteger() { | ||
| try { | ||
| Scanner scanner = new Scanner(System.in); | ||
| return scanner.nextInt(); | ||
| } catch (Exception e) { | ||
| return 0; | ||
| } | ||
| } |
There was a problem hiding this comment.
임의의 값으로 리턴하기 보다 재입력을 받아 정상 동작하도록 유도하면 어떨까요?
| for (int i = 0; i < horizontalLine.size(); i++) { | ||
| Boolean hasLineOnIndex = horizontalLine.hasLineOnIndex(i); | ||
| printHorizontalLineOrBlank(hasLineOnIndex); | ||
| System.out.print("|"); | ||
| } |
There was a problem hiding this comment.
size(), hasLineOnIndex() 는 출력에 필요한 메서드라고 생각되는데요.
스트림, 람다 미션이니 index로 접근하기 보다 불변 컬렉션을 리턴해서 스트림을 최대한 활용해 보시면 좋을 것 같아요.
| void 생성된_가로선은_연속되지_않아야_한다() { | ||
| // given | ||
| int userCount = 4; | ||
|
|
||
| // when | ||
| HorizontalLine horizontalLine = new HorizontalLine(userCount); | ||
|
|
||
| // then | ||
| for (int i = 0; i < horizontalLine.size() - 1; i++) { | ||
| boolean currentGeneratedLine = horizontalLine.hasLineOnIndex(i); | ||
| boolean nextGeneratedLine = horizontalLine.hasLineOnIndex(i + 1); | ||
| Assertions.assertThat(currentGeneratedLine).isNotEqualTo(nextGeneratedLine); | ||
| } |
There was a problem hiding this comment.
모든 값을 꺼내어서 결과를 검증하다보니 테스트 코드가 복잡해질 수 밖에 없을 것 같아요.
'연속된 가로선을 가질 수 없다'는 책임을 생성 시점에 검증하면 어떨까요?
new HorizontalLine(List.of(true, true)); // exception throw
혹은 위 책임을 수행할 별도의 객체를 도출해 보셔도 좋을 것 같아요.
| // when | ||
| Ladder ladder = new Ladder(userCount, ladderMaxHeight); | ||
|
|
||
| // then | ||
| Assertions.assertThat(ladder.size()).isEqualTo(ladderMaxHeight); |
There was a problem hiding this comment.
size 검증만으로 정상 사다리라고 보기 어렵지 않을까요.
userCount는 어떤 역할을 하는지 테스트 코드를 아무리 봐도 알 수가 없는데요.
사다리를 생성한 다음 예상되는 사다리 객체를 하드코딩해서 객체끼리 비교하면 어떨까요?
생성 전략을 외부에서 주입한다면 원하는 형태의 사다리를 만들 수 있으니 객체간의 비교도 가능할 것 같아요.
ghost
closed this by deleting the head repository
1 participant
안녕하세요~!
2단계 - 사다리(생성) 리뷰 요청드립니다.
감사합니다! 🙇