[1단계 사다리 생성] 마코(이규성) 미션 제출합니다. #89
Conversation
There was a problem hiding this comment.
안녕하세요 마코~ 리뷰어 파즈입니다!
1단계 수고하셨습니다. 코드 잘 봤습니다. 다들 잘 짜시네요 ㅎㅎ
코멘트 일부 남겼으니 확인부탁드리겠습니다!
- enum 위치 적절? enum 잘 적용한게 맞나요? 다른거 또 없을까요?
->ValidationTypeEnum을 활용해 적절하게 잘 사용하신 것 같습니다. 5기분들 리뷰하다 보니깐 validation 클래스들 체이닝해서 잘들 사용하시는 것 같습니다. 활용 잘하셨습니다. 또 적용할만한거라... 크게 없어보이기는 하는데 굳이 굳이 한다면OutputView의
private static final String CONNECTED_LINE = "-----";
private static final String UNCONNECTED_LINE = " ";이것들을 하자면 할 수 있을 것 같네요. Enum 자체가 상수들의 집합이기도 하니까요. 카테고리 느낌으로 사용하실 수도 있고 또는 뭐 이후에 특정 엔티티에 대해서 특정 필드를 where 을 이용해서 조회가 잦은 경우에 해당 엔티티의 필드들을 Enum에다가 다 집어넣기도 합니다. where 절에서 쿼리 조회 시에 직접 String으로 입력하다보면 오타가 종종나기 때문에 이를 방지하기 위해서 사용되는 경우도 있습니다.
- 단순 개행 controller vs view 누가 책임?
-> controller에서 한다면 controller 내부 코드에서System.out.println()과 같은 개행 코드를 실행한다는 뜻일까요? 그렇다면 저는 view가 책임지는게 맞다고 봅니다. 개행 또한 바깥에 보여지는 view이고, controller와 view 라는 layer로 각각 나눠진 이상 해당 역할은 view의 역할이라고 생각합니다. 개인적인 의견임을 참고하시면 좋겠습니다. 그러면 마코는 이거는 또 어떻게 생각하시는지 궁금하네요. Ladder 및 Layer에 대한 출력 정보를( '|', '----', ' ' 요런 것들) 해당 객체에서 조립해주고, OutputView는 단순히 해당 객체의 메서드를 호출해서 print만 해준다. 이것에 대해서는 어떻게 생각하시는지 의견 들어보고 싶네요
피드백 반영 후 재리뷰요청 기다리고 있겠습니다. 화이팅!
src/main/java/Application.java
Outdated
| private static LadderMaker makeLadderMaker() { | ||
| return new LadderMaker(new RandomBooleanGenerator()); | ||
| } | ||
|
|
||
| private static InputView makeInputView() { | ||
| return new InputView(makeInputValidator()); | ||
| } |
There was a problem hiding this comment.
두 메서드의 위치가 변경되면 좋을 것 같습니다. main 메서드에서 makeInputView를 먼저 호출하기 때문에 해당 메서드가 위로 가있는 것이 코드 가독성이 더 올라갈 것 같습니다.
There was a problem hiding this comment.
필요한 메서드 순서대로 확인하기가 편하겠네요!
말씀하신대로 수정했습니다.
src/main/java/view/InputView.java
Outdated
|
|
||
| public List<String> inputNames() { | ||
| System.out.println(INPUT_NAMES_MESSAGE); | ||
| return List.of(scanner.nextLine().trim().split(NAME_DELIMITER)); |
There was a problem hiding this comment.
a, a 로 입력하면 a와 a Name으로 입력이 될 것인데 사용자 입장에서 동일하게 앞 공백이 있는 a 를 입력했는데 하나는 공백이 다 잘리고 하나는 공백을 허용하고 하는 것이 혼란스러울 것 같습니다. 차라리 이름별로 앞뒤 공백 자르는건 어떨까요? 더불어서 중복되는 이름은 입력할 수 없도록 해보시면 좋을 것 같습니다.
There was a problem hiding this comment.
차라리 이름별로 앞뒤 공백 자르는건 어떨까요?
공백 처리에 대해 신중하지 못했네요. trim만 사용해야지 하고 깊게 생각하지 못했습니다. trimNames() 메서드를 만들어 List내의 모든 String에서 trim() 을 호출하도록 변경했습니다.
더불어서 중복되는 이름은 입력할 수 없도록 해보시면 좋을 것 같습니다.
우선, 중복을 검증하기 위해 새로운 ValidatorChain인 DuplicateNameValidatorChain을 추가하는 과정에서 체이닝이 부자연스럽다라는 느낌을 받았습니다. 따라서 InputValidator의 일급컬렉션인 InputValidators에서 InputValidator를 관리하고, InputValidator는 builder 패턴을 적용하여 체인을 추가할 수 있도록 수정했습니다.
결과적으로 사용 시 아래와 같이 자연스러운 체인을 만들 수 있었습니다.
private static InputValidator makeInputValidator() {
return InputValidator.builder()
.add(new EmptyInputValidatorChain())
.add(new NotIntegerValidatorChain())
.add(new DuplicateNameValidatorChain())
.build();
}There was a problem hiding this comment.
기존 setNext 가독성이 너무 안좋았는데 확 좋아졌네요~ 이해하기도 더 편한 것 같습니다.
src/main/java/domain/vo/Name.java
Outdated
| public String get() { | ||
| return value; | ||
| } |
There was a problem hiding this comment.
자바 getter 기본생성 네임 컨벤션이 get{FieldName}} 이기 때문에 해당 이름을 따르는 것이 혼동을 주지 않습니다.
get()은 정확히 무얼 get 하는지 알 수 없기 때문에 Name 객체에 한 번 더 들어와야 한다는 단점이 있습니다.
추측하자면 객체 이름 자체가 Name 이기 때문에 get으로 충분하다 라고 생각하셨을 것 같습니다만 코드를 처음 접하는 입장에서는 Name이 원시값 포장 객체임을 모르는 상황이기 때문에 어떤것을 get 하는지를 명확하게 해주셔야 합니다.
There was a problem hiding this comment.
추측하자면 객체 이름 자체가 Name 이기 때문에 get으로 충분하다 라고 생각하셨을 것 같습니다만
깜짝 놀랐습니다!! 😯 처음에 변수를 name으로 지어서 Name.getName()과 같이 사용했다가 이상해서 바꿨습니다.
그 이후 어차피 Name안에 있는 값이니 value로 변수의 이름을 수정했고요.
어떤것을 get 하는지를 명확하게 해주셔야 합니다.
그렇다면 getValue() 로 작성하는 것이 옳겠네요. 이에 따라 다른 VO 객체들도 getValue()로 수정했습니다.
이 주제에 대해 작은 의문점이 또 생겼는데요, 값 객체의 값을 가져온다는 의미에서 value()로 수정하는 것에 대해서는 어떻게 생각하시나요? ladder.getName.getValue() 와 같이 get을 두 번 호출하는 것은 좀 부자연스러우며
ladder.getName.value() 와 같은 형식이 자연스럽다는 느낌이 듭니다.
There was a problem hiding this comment.
get 2번 호출이 부자연스러우시다면 getName 말고 getNameValue 같은 메서드를 두고 내부 구현으로 Name의 value를 리턴하게끔 만드시면 될 것 같습니다!
저도 읽는 측면에서는 모든 getter 메서드들이 get을 떼고 그냥 필드만 쓰면 좋다고 생각합니다. 하지만 일반적으로 기대하는 getter에 대한 메서드 형식이 있기 때문에 get 을 붙이는 편입니다. 이것과 관련해서 '엘레강트 오브젝트' 라는 책에 객체지향에 대한 재밌는 관점의 의견들이 꽤나 많습니다. 마코가 말씀주신 .value() 에 대한 주장도 해당 책에 나와있습니다. 저도 우테코 할 때에 이 책이 유행이 돼서 크루들끼리 읽고 토론하던게 생각나네요! 포비는 아직 읽지말라고 말리긴 하셨지만 관심가신다면 나중에 한 번 읽어보시면 재밌으실겁니다!
src/main/java/view/InputView.java
Outdated
| public int inputLadderHeight() { | ||
| System.out.println(INPUT_LADDER_HEIGHT_MESSAGE); | ||
| String input = scanner.nextLine(); | ||
| validator.validate(new InputValidationRequest(List.of(ValidateType.EMPTY_VALUE, ValidateType.INTEGER_VALUE), input)); | ||
| return Integer.parseInt(input); |
There was a problem hiding this comment.
scanner.nextInt() 메서드는 고려된 코드인지 알고 싶습니다.
There was a problem hiding this comment.
숫자만 입력 받기 때문에 scanner.nextInt() 로 입력 받는 것이 더 직관적일 수 있습니다.
하지만 입력 시 줄바꿈 문자가 남아 다음 scanner.nextLine() 의 입력으로 사용됩니다.
이를 해결하기 위해 scanner.nextInt()를 사용하고 바로 scanner.nextLine()으로 줄바꿈 문자를 지워줄 수 있는데요, 이러한 방법은 scanner.nextLine() 을 빠뜨리는 실수를 하기도 쉽고, 실수를 했을 때 어디가 문제인지 찾기 어려울 수 있습니다.
따라서 저는 scanner.nextLine() 으로 입력을 받고 Integer.parseInt()로 변환하여 사용하는 편 입니다.
| public Ladder(final Height height, final Width width) { | ||
| this.height = height; | ||
| this.width = width; | ||
| this.layers = new ArrayList<>(); |
There was a problem hiding this comment.
굳이 생성자에서 할당할 필요가 없었군요!
생성자가 한 층 깔끔해졌네요 😄
| public void makeLine(final boolean condition) { | ||
| if (condition && lines.isEmpty()) { | ||
| lines.add(true); | ||
| return; | ||
| } | ||
| if (condition && !lines.get(lines.size() - 1)) { | ||
| lines.add(true); | ||
| return; | ||
| } | ||
| lines.add(false); | ||
| } |
src/main/java/view/OutputView.java
Outdated
| StringBuilder stringBuilder = new StringBuilder(); | ||
| stringBuilder.append("\n"); | ||
| names.forEach(name -> { | ||
| int difference = INTERVAL_UNIT - name.get().length(); | ||
| stringBuilder.append(name.get()).append(NAME_SPACE.repeat(difference)); | ||
| }); | ||
| System.out.println(stringBuilder); |
There was a problem hiding this comment.
String 대신 StringBuilder를 사용하신 이유는 무엇인가요?
StringBuffer는 고려하셨을까요?
There was a problem hiding this comment.
String 대신 StringBuilder를 사용하신 이유는 무엇인가요?
String은 불변이기 때문에 + 연산마다 새로운 객체를 만들어 주어야 하기 때문에 가변인 StringBuilder를 사용하여 append를 하는 방식을 채택했습니다. 공부하는 과정에서 아래의 내용을 알게되었습니다!
JAVA9 부터는 String의 + 연산자의 내부 구현이 StringConcatFactory.makeConcatWithconstants() 를 사용하여
효율적으로 변경되긴 했으나 하지만 연산이 많은 경우에는 여전히 StringBuilder를 사용하는 것이 좋다.
StringBuffer는 고려하셨을까요?
부끄럽게도 StringBuffer의 존재에 대해서 몰랐습니다! 이를 공부하면서 다음의 내용들을 알게 되었습니다.
- StringBuffer가 멀티 스레드 환경에서 효율적이다.
- 단일 스레드 환경에서 성능적으로 더 뛰어난 StringBuilder가 JDK1.5에서 출시
| return ladder; | ||
| } | ||
|
|
||
| private Layer makeLayer(int lineCount) { |
There was a problem hiding this comment.
다른 클래스의 특정 메서드의 파라미터에는 final이 붙어있는데 이곳에는 붙어있지 않습니다. 의도하신 걸까요?
There was a problem hiding this comment.
페어로 작성하다 보니 일관되지 못했네요.
이러한 차이가 있다면 페어와 컨벤션을 정해서 일관되게 작성해야겠어요!
이전에는 굳이 파라미터 까지 final을 붙여야 하나 라는 생각이었지만
현재는 파라미터도 가능한 불변을 보장하여 안정성을 더 높이는 것이 좋겠다 라는 생각으로 바꼈습니다!
| public void addLayer(Layer layer) { | ||
| layers.add(layer); | ||
| } |
There was a problem hiding this comment.
의도된 누락이었습니다.
단순히 List에 add를 하는 함수이기 때문에 굳이 해야 하는가? 라는 생각이 들었습니다.
add에 대해 어떤 테스트를 할 수 있을까 라는 고민을 해보았는데, 객체를 넣고 잘 들어갔는지 확인을 하는 것 정도 밖에는 생각이 나지 않네요. 이것으로 의미 있는 테스트라고 볼 수 있을까요?
TDD를 한다는 관점에서 생각해보면 테스트에서는 내부 구현을 모르는 상태에서 사용하기 때문에 의미가 있을 수도 있겠다는 생각이 들지만 아직 잘 모르겠습니다.
@Test
void addLayer() {
Layer layer = new Layer();
layer.makeLine(true);
Ladder ladder = new Ladder(new Height(5), new Width(5));
ladder.addLayer(layer);
assertThat(matchLayers(ladder, layer)).isEqualTo(true);
}
private boolean matchLayers(Ladder ladder, Layer layer) {
if (ladder.getLayers().isEmpty()) {
return false;
}
return ladder.getLayers()
.stream()
.allMatch(it -> it.equals(layer));
}일단 List에 넣고 잘 들어갔는지 확인하는 테스트를 작성해보았습니다.
There was a problem hiding this comment.
저도 단순히 add 하는 것에 대한 테스트의 의미가 있는가? 라고 생각을 합니다.
하지만, 요번 미션 중점 자체가 tdd이기 때문에 해당 메서드를 추가하기 전에 테스트를 먼저 짜고 추가가 되어야 했기 때문에 남긴 리뷰였습니다. 저도 그냥 한다면 스킵할 메서드이긴 합니다 ㅎㅎ ..
| public InputValidationRequest(List<ValidateType> validateTypes, String target) { | ||
| this.validateTypes = validateTypes; | ||
| this.target = target; | ||
| } | ||
|
|
||
| public List<ValidateType> getValidateTypes() { | ||
| return validateTypes; | ||
| } |
There was a problem hiding this comment.
List를 생성자를 받을 때에 this.validateTypes = validateTypes; 와
List를 get 할 때 return validateTypes;
이 2개의 코드는 의도하신 코드인지 알고 싶습니다! 다른 클래스에서는 다른 방식으로 처리했었어가지고 의도하신건지 알고싶었습니다!
There was a problem hiding this comment.
페어와 헤어진 이후 코드를 한번 더 리팩토링 했습니다.
이 때 생성자에서는 방어적 복사를 하고 getter에서는 불변 컬렉션을 반환하도록 변경했는데, 이 부분은 누락이 된 것 같습니다...
코드의 의도는 외부에서 내부 컬렉션에 대한 변경을 방지하고자 이러한 방식으로 처리했습니다.
|
안녕하세요 파즈!
OutputView에서 개행을 위한
OutputView에서는 View에 관한 로직을 담당한다는 것이 제 의견입니다! 코드의 의도, 다른 방법도 고려하였는지 물어봐 주셔서 무심코 사용했던 것들에 대해 다시 한번 깊게 고민해보는 좋은 시간을 가졌습니다! |
|
그런 경우의 개행을 말씀하신 거였군요. 저는 view에다가 둘 것 같습니다. 특정 정보를 출력하고 개행까지 같이 가는거를 한 흐름으로 생각하고 정보 출력 메서드 내부에 동일하게 둘 것 같습니다. class view {
public 정보출력하는메서드(정보들) {
정보출력
개행_출력();
}
private 개행_출력() {
sout();
}그냥 요렇게 할 것 같네요~
👍👍 |
안녕하세요 마코입니다!
노력한 점
TDD를 처음 적용해보았는데 domain의 class들은 최대한 test를 먼저 작성하고 프로덕션 코드를 작성하려고 노력했습니다.
궁금한 점
사다리 미션 1단계를 진행하면서 다음과 같은 궁금했던 점들이 있었습니다. 리뷰어분의 의견이 궁금합니다.
1. Enum을 적용한 위치가 적절한지 : InputView에 대한 검증 로직을 구현할 때 validateType과 validate를 할 값을 리퀘스트로 묶어서 InputValidator에게 전달하고 각 InputValidatorChain에서
request.getValidatorTypes().contains()를 통해 검증하려는 validateType이면 검증하도록 구현했습니다. 이 때 validateType을 Enum으로 정의하여 사용했는데 Enum을 잘 적용한 것이 맞는지, 적용할 만한 다른 부분은 없는지 궁금합니다!2. 출력 결과의 형식을 요구사항과 맞추기 위한 방법 : 단순히 개행만 하는 함수를 OutputView에 구현하여 개행이 필요할 때 controller에서 사용하여 형식을 맞추는 방법 vs 출력 문자열 자체에 개행을 포함해서 형식을 맞추는 방법.
즉, 형식을 맞추기 위한 단순한 개행이
컨트롤러의 책임 vs 뷰의 책임에 대해 어떻게 생각하시나요? 페어의 의견은 뷰의 책임이었는데 저는 어느 쪽이 맞는 것인지 모호합니다.따끔한 리뷰 부탁드리며 좋은 하루 되시길 바랍니다!!😄