[사다리 타기] 박소정 미션 제출합니다.#1
[사다리 타기] 박소정 미션 제출합니다.#1sojeong0202 wants to merge 35 commits intogdgoc-skhu-missions:sojeong0202from
Conversation
- 사람 이름 공백 예외 처리 - 사람 이름 null 예외 처리 - 사람 이름 겹칠 때 예외 처리
-사람 이름 공백 예외 처리 -사람 이름 null 예외 처리 -사람 이름 겹칠 때 예외 처리
- 쉼표 기준으로 이름 분리해서 Person 인스턴스 생성 후 - PersonList에 Person 인스턴스 넣는 기능 테스트
- 쉼표 기준으로 이름 분리해서 Person 인스턴스 생성 후 PersonList에 Person 인스턴스 넣는 기능 구현
- 한 줄 랜덤 생성 로직과 재가공하는 로직 분리
HanHyunsoo
left a comment
HanHyunsoo
left a comment
There was a problem hiding this comment.
고생하셨습니다. 아래 궁금한 점에 대해 답변 드립니다
궁금한 점에 대한 답변
- 일급 컬렉션 잘 사용한 게 맞을까요?
- 잘하신 것 같습니다. 일급 컬렉션이 좀 더 객체지향적으로 가기위한 방법인 것이고 필요한 상황일 때 판단하시고 앞으로 사용하시면 좋을 것 같아요.
- 더 테스트 해야 할 부분이 있을까요?
- 특정 입력 값에 대한 것이 아니라 다양한 입력 값에 대해서도 테스트 하시면 좋을 것 같습니다. 예를 들면 입력값이 (소정,예은,GDSC) 고 출력이 정상적으로 작동한다고 하면 다른 입력 (현수, 한길)도 제대로 작동되는지 보장되어야겠죠? 보통 이런 여러 입력을 여러개 정의하여 하는 경우도 있지만 랜덤한 값으로 하는 방법도 있어요
- 어떤 테스트 메소드를 많이 사용하는지 궁금합니다.
- 저의 경우에는 eqauls랑 throw관련 메서드 많이 사용하는 것 같습니다.
| return firstNameBuilder.toString(); | ||
| } | ||
|
|
||
| private String onlyVerticalLine() { |
There was a problem hiding this comment.
해당 함수를 상수로 따로 뺴는 것이 좋을 것 같습니다.
| return " |"; | ||
| } | ||
|
|
||
| private String containPrintHorizontalLine() { |
There was a problem hiding this comment.
해당 함수를 상수로 따로 뺴는 것이 좋을 것 같습니다.
| @@ -0,0 +1,15 @@ | |||
| package ladder.util; | |||
|
|
|||
| public enum ErrorMessage { | |||
There was a problem hiding this comment.
error 메세지를 enum으로 나눈 것에 칭찬드려요!
하지만 이 프로젝트에서 거의 모든 예외가 RuntimeException(IllegalArgumentException)으로 처리되서 개발자가 봤을 때 명확한 오류를 찾기위해서는 message를 봐야한다는 점이 아쉬운 것 같아요.
enum을 클래스 변수로 잡고 각 예외 상황에 대한 예외클래스들로 변경하여 리팩토링 하면 로직, 테스트 면에서도 더 명확해질 것 같아요.
| @DisplayName("사다리 가로라인(Line)을 겹치지 않게 생성하는 메소드 테스트") | ||
| void should_ReraangeLadderLine_When_DuplicateTrue() { | ||
| // given | ||
| List<Boolean> points = Arrays.asList(true, true, true, true, true); |
There was a problem hiding this comment.
List.of로 변경해주시면 좋을 것 같습니다.(Array -> List로 변환하는 과정에서 리소스가 듦)
|
|
||
| Scanner scanner = new Scanner(System.in); | ||
|
|
||
| public String inputName() { |
There was a problem hiding this comment.
여러 사람들의 이름을 입력받으니 이에 맞게 함수명에 복수형을 추가하는 것이 좋을 것 같습니다.
| } | ||
|
|
||
| private static boolean hasDuplicatePersonName(String input) { | ||
| List<String> allPersonNames = new ArrayList<>(Arrays.asList(input.split(","))); |
There was a problem hiding this comment.
stream으로 변경하면 더 가독성 있는 코드가 될 것 같아요.
| import java.util.List; | ||
| import java.util.Random; | ||
|
|
||
| public class RandomBoolean { |
There was a problem hiding this comment.
해당 클래스가 model의 역할을 하는지 명확하지 않은 것 같아요. random으로 boolean들을 생성하고 이를 통해 사다리를 구축해야하는데 사다리(LadderMaker or Line)에 의존하는 클래스라.. 이를 사용하는 클래스 안에 함수로 따로 빼는 것이 더 좋은 방향일 것 같아요.
|
바쁜 시간 내어 리뷰 해주셔서 감사합니다! |
This reverts commit 7ee1e78.
|
늦어서 죄송합니다! 리뷰 후 리팩토링 사항👀 개선한 점
🧐 궁금한 점
|
3 participants
😱 어려웠던 점
🧐 궁금한 점
✍️ 느낀 점