-
Notifications
You must be signed in to change notification settings - Fork 251
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[1단계 - 사다리 생성] 에버(손채영) 미션 제출합니다. #316
Conversation
Co-authored-by: takoyakimchi <c4nyou@gmail.com>
Co-authored-by: takoyakimchi <c4nyou@gmail.com>
Co-authored-by: takoyakimchi <c4nyou@gmail.com>
Co-authored-by: takoyakimchi <c4nyou@gmail.com>
Co-authored-by: takoyakimchi <c4nyou결@gmail.com>
Co-authored-by: takoyakimchi <c4nyou@gmail.com>
Co-authored-by: takoyakimchi <c4nyou@gmail.com>
Co-authored-by: takoyakimchi <c4nyou@gmail.com>
Co-authored-by: takoyakimchi <c4nyou@gmail.com>
Co-authored-by: takoyakimchi <c4nyou@gmail.com>
Co-authored-by: takoyakimchi <c4nyou@gmail.com>
Co-authored-by: takoyakimchi <c4nyou@gmail.com>
Co-authored-by: takoyakimchi <c4nyou@gmail.com>
Co-authored-by: takoyakimchi <c4nyou@gmail.com>
Co-authored-by: takoyakimchi <c4nyou@gmail.com>
Co-authored-by: takoyakimchi <c4nyou@gmail.com>
Co-authored-by: takoyakimchi <c4nyou@gmail.com>
Co-authored-by: takoyakimchi <c4nyou@gmail.com>
Co-authored-by: takoyakimchi <c4nyou@gmail.com>
Co-authored-by: takoyakimchi <c4nyou@gmail.com>
Co-authored-by: takoyakimchi <c4nyou@gmail.com>
Co-authored-by: takoyakimchi <c4nyou@gmail.com>
Co-authored-by: takoyakimchi <c4nyou@gmail.com>
Co-authored-by: takoyakimchi <c4nyou@gmail.com>
Co-authored-by: takoyakimchi <c4nyou@gmail.com>
Co-authored-by: takoyakimchi <c4nyou@gmail.com>
Co-authored-by: takoyakimchi <c4nyou@gmail.com>
Co-authored-by: takoyakimchi <c4nyou@gmail.com>
Co-authored-by: takoyakimchi <c4nyou@gmail.com>
Co-authored-by: takoyakimchi <c4nyou@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요 에버! 사다리 미션 리뷰하게 된 알렉스입니다!
미션 구현하시느라 수고 많으셨습니다. 많은 고민을 하신 것 같네요!
생각해볼만한 부분들 코멘트 남겼으니 확인해보시고 다시 고민해보면 어떨까 싶습니다!
질문 답변
컨트롤러에서 현재는 뷰만을 파라미터로 주입 받고 있는데, 나머지 두 객체도 인자를 통해 주입받는 게 좋을까?
ErrorHandler의 범위에 따라 역할이 다를 것 같아요. 정의하신 역할이 에러를 캐치해서 뷰에 적절한 에러 형태로 전송하는 역할이라면 View와 같은 레벨인 클래스로 보입니다.
뷰와 메시지 리졸버의 역할은 구분하는 것이 좋을까?
리뷰 코멘트들 확인해보시고 코드 수정해본 후에 다시 고민해보시면 어떨까 싶어요!
변환과 검증 로직을 분리해야할까?
위 답변과 마찬가지로 코멘트들 확인해주세요 🙂
다리에서 Point.CONNECTED가 연속으로 두 번 나오는 경우 에러를 출력한다.의 경우에 대해서 검증 로직 및 테스트 코드를 만들어야 할까?
CONNECTED가 연속인 경우는 무결성이 해쳐지는 케이스네요! 어느 클래스 입장에서 무결성이 해쳐진 것인지 생각해보면 어디에 어떤 식으로 테스트 코드를 추가해볼지 생각해볼 수 있을 것 같아요!
좋은 리뷰 감사합니다 알렉스! 알렉스의 피드백 반영하여 코드 수정해보았어요. 하지만 여전히 알렉스의 의견이 궁금한 부분이 있어 추가로 코멘트 달아두었습니다. 또한 무결성을 해치는 테스트 케이스인 'CONNECTED가 연속으로 나오는 경우'를 테스트할 방법을 아직 찾지 못하였습니다..🥹 테스트할 수 있다면 더할 나위 없이 좋은 코드이지만, 혹시 알렉스에게 관련 의견이 있다면 언급 부탁드려요! 감사합니다! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요 에버! 수정할 부분이 많았을 텐데 리뷰 반영 잘 해주셨네요!
질문주신 부분들은 최대한 코멘트로 답변 달아봤는데 확인 부탁드리고 이해가 안가는 부분이 있다면 언제든 슬랙으로 DM 주셔도 좋습니다!
무결성을 해치는 테스트 케이스인 'CONNECTED가 연속으로 나오는 경우'를 테스트할 방법을 아직 찾지 못하였습니다..🥹
현재 Line의 생성자에서 이전 이 CONNECTED면 고정으로 UNCONNECTED 아니면 랜덤뽑기 하는 로직이 있는걸로 보여요! 이부분을 바깥으로 빼보면 어떨까요? Line에선 CONNECTED가 연속으로 들어왔는지에 대한 검증만 해볼 수 있지 않을까요?
개인적으론 꼭 테스트하지 않아도 된다고 생각하긴 합니다! 연속으로 CONNECTED가 절대 생성되지 않는다
정도만 테스트해도 괜찮다고 생각해요!
테스트 하는 방법에 대한 힌트는 고민해보시면 좋을 것 같고, 반영할지 말지는 에버의 생각을 주로 해서 결론내보면 좋을 것 같아요! 아직 이 PR에서 고민해보면 좋을 것들이 남아있는 것 같고, 미션 시간도 많이 남은 것 같아서 한번 더 RC드릴게요! 💪
private String requestNames() { | ||
return inputView.readNames(); | ||
} | ||
|
||
private Members makeMembers(String rawNames) { | ||
return new Members(rawNames); | ||
} | ||
|
||
private String requestHeight() { | ||
return inputView.readHeight(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
꼭 필요한 메소드 분리일까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이전에 주셨던 코멘트가
메서드 분리를 의도하신 것이라고 판단하였습니다.
다른 방법을 고민해보겠습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
알렉스가 주신 코멘트 두가지를 정리하면 아래와 같습니다!
- 하나의 메서드가 입력 로직과 객체 생성 로직 두가지를 수행하는 것은 역할이 많다.
- 각각을 메서드로 분리하는 작업이 꼭 필요할까?
제가 역할을 분리하기 위해 선택했던 방법이 메서드 분리였습니다.
메서드를 분리하지 않고 역할을 분리하는 방법은 찾지 못하였습니다..
메서드 분리가 아닌 방법으로 어떻게 역할을 분리할 수 있는지 궁금합니다! 🥹
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이전 메소드는 입력 + Members생성까지 같이 한다고 생각해서 많은 역할을 가진다고 생각했던 것 같아요!
이번 케이스에선 new Members()
라는 코드 한줄을 호출하는데 굳이 makeMembers라는 함수가 필요할까란 생각이 드네요!
생성자 자체로도 이미 Members를 생성하는 로직인게 잘 드러나는 것 같아서요! 메소드로 래핑하지 않고 그냥 바로 생성자를 호출하지 않은 이유는 무엇인가요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
사실 이유가 없었습니다..
메서드의 역할이 많다는 알렉스의 코멘트를 제 나름대로 반영하려고 하다보니
오히려 더 좋지 않은 구조의 코드가 된 것 같아요 ㅠㅠ
다른 방법을 더 고민해보겠습니다! 🥲
return stringBuilder.toString(); | ||
} | ||
|
||
public String resolveLines(Lines ladder) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
변수명과 타입이 매치가 안되는 것 같아요! 의도하신 부분일까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 메서드가 Lines 클래스에서 lines 변수를 꺼내와야 하는 상황이라,
변수명 중복을 피하기 위해 ladder 라는 이름을 사용하였습니다!
src/main/java/domain/Line.java
Outdated
|
||
public class Line { | ||
|
||
private final List<Connection> points = new ArrayList<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
변수 이름도 변경되어야하지 않을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PointStrategy와 RandomPointStrategy 클래스가 존재하여
변수명은 points로 유지하자는 생각으로 변경하지 않았습니다.
하지만 지금 다시 생각해보니 두 단어를 혼용하는 것은 혼돈만 줄 것 같네요..
관련 클래스 및 변수명 모두 Connection 단어를 사용하도록 수정하겠습니다!
위 테스트를 작성하면 무결성이 100% 보장됨을 확인시킬 수 있네요...! 연속으로 CONNECTED가 나오는 경우를 테스트하는 방법은 피드백 바탕으로 코드 수정하였습니다. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요 에버! 늦은시간까지 리뷰 반영해주셨네요! 고생 많으셨습니다!🙂
1단계 진행하시면서 충분히 많은 부분들을 고민하신 것 같습니다!
간단한 코멘트 몇 가지 남겼고 다음 단계에서 반영해주시면 될 것 같아서 이만 머지하도록 할게요!
다음 단계도 화이팅입니다!💪
private Members makeMembers() { | ||
return errorHandler.readUntilNoError(() -> { | ||
String rawNames = inputView.readNames(); | ||
return Members.from(rawNames); | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
입력 + Members 생성까지 해서 너무 많은 역할을 한다고 피드백 드렸었는데, 이번엔 메소드 역할에 에러 핸들링까지 추가되었어요!
어떻게 변경해보면 좋을까요?
public class Height { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
테스트에서만 사용하는 클래스가 되었네요! 이제 안쓰는 클래스일까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
리팩토링하는 과정에서 Height 클래스 내의 로직들을 외부로 넘기다보니 무의미한 클래스가 되어버렸네요...
해당 클래스는 삭제했습니다!
} | ||
|
||
public static Members from(String rawNames) { | ||
List<String> names = StringParser.splitByDelimiter(rawNames, DELIMITER); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 객체의 역할이 맞을까요? input이 ,
으로 split할 수 있는 형태로 들어온다는건 어떤 객체의 책임이 적절할까요?
현재는 만약 UI가 console 이 아니라 Web이고 각 이름을 개행으로 구분해서 입력한다면 이 도메인까지 수정되어야 할 것 같은데, view와 비즈니스 로직이 분리되었다고 볼 수 있을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
헉 그러네요. 구분자가 변경될 경우 Members 클래스가 수정되어야하는 상황이 발생하네요.
그렇다면 도메인보다는 컨트롤러의 책임이라고 생각됩니다.
생각 반영하여 코드 수정하겠습니다!
private static List<Member> makeMembers(List<String> names) { | ||
List<Member> members = new ArrayList<>(); | ||
names.forEach(name -> members.add(new Member(name))); | ||
return members; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private static List<Member> makeMembers(List<String> names) { | |
List<Member> members = new ArrayList<>(); | |
names.forEach(name -> members.add(new Member(name))); | |
return members; | |
} | |
private static List<Member> makeMembers(List<String> names) { | |
return names.stream() | |
.map(Member::new) | |
.toList(); | |
} |
return Connection.DISCONNECTED; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
파일 끝에 개행이 빠졌네요! EOF!
void test_exception_moreThanFiveLetters(String name) { | ||
assertThatThrownBy(() -> new Name(name)) | ||
.isInstanceOf(IllegalArgumentException.class) | ||
.hasMessageContaining("자의 이름만 허용합니다."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
에러 메시지도 검증하고 계시네요! 메시지를 검증할 때 문구보단 몇자 이하로 들어와야하는 지가 잘 노출되는게 좀 더 중요하지 않을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
알렉스 의견에 동의합니다! 제가 놓친 것 같아요.
앞으로는 더욱 꼼꼼히 체크해야겠네요. 감사합니다!
안녕하세요 알렉스!
페어 트레와 사다리 생성 미션을 진행하게 된 에버 입니다. 😁
저희는 TDD 방식을 처음 접해보아서 사이클을 정확히 지켜가며 페어 프로그래밍을 진행해보았습니다.
저와 페어 간 의견을 조율하는 과정에서 리뷰어님의 생각이 궁금했던 부분이 있었습니다.
컨트롤러에서 현재는 뷰만을 파라미터로 주입 받고 있는데, 나머지 두 객체도 인자를 통해 주입받는 게 좋을까?
GameController
파일에서InputView
와OutputView
를 파라미터로 받아 인스턴스 변수에 저장합니다. 컨트롤러와 뷰는 상관관계가 있다고 생각하여 의존성을 주입하는 형태로 구현하였지만, 이 외에ErrorHandler
객체에 대해서는 컨트롤러 내에서 새로 객체를 생성하여 저장하도록 구현하였습니다. 사실 두 경우의 큰 차이를 아직 잘 모르겠고, 이 경우 리뷰어님은 어떤 방식의 스타일을 선호하는지 궁금합니다.뷰와 메시지 리졸버의 역할은 구분하는 것이 좋을까?
OutputView
의 역할을 줄이기 위해 메시지를 출력 형태로 가공하는 리졸버 클래스MessageResolver
를 추가로 생성하였습니다. 역할 분리와 중요 데이터 전달 최소화를 위해 리졸버를 생성하자는 의견과, 해당 역할을 뷰에 부여해도 괜찮을 것이라는 의견이 있었습니다. 이에 대한 리뷰어님의 생각이 궁금합니다.변환과 검증 로직을 분리해야할까?
Height
클래스에서 문자열->정수 변환 작업과 정수 형태가 맞는지 검증하는 로직을 하나의 메서드에 담았습니다. 하지만 이러한 경우 validate의 역할을 갖는 메서드가 변환 및 반환의 역할까지 갖게 되어 역할을 제대로 분배하지 않은 것 같다고 생각됩니다. 변환 및 반환의 역할을 다른 메서드로 분리를 하려고 하였지만 명료하게 두 역할을 나누기도 어려웠습니다. 이 경우,다음과 같이 문자열→정수 변환 로직을 중복으로 사용하여 변환과 검증 로직 분리가 가능합니다.
중복이라는 단점을 감수하고 역할을 분리하는 쪽이 합리적인지, 다중 역할을 갖더라도 중복을 최소화하는 것이 합리적인지 리뷰어님의 생각이 궁금합니다!
사다리에서 Point.CONNECTED가 연속으로 두 번 나오는 경우 에러를 출력한다.
의 경우에 대해서 검증 로직 및 테스트 코드를 만들어야 할까?감사합니다 알렉스! 가감없이 리뷰해주세요 😊