[사다리 - 1단계] 홍석주 미션 제출합니다 #10
Conversation
src/main/java/ladder/Size.java
Outdated
| public Size(int width, int height) { | ||
| this.width = width; | ||
| this.height = height; | ||
| } |
There was a problem hiding this comment.
현재 Size 객체 내에 width와 height 값에 대한 검증이 필요할 것 같습니다!
| import java.util.List; | ||
| import java.util.Random; | ||
|
|
||
| public class Points { |
There was a problem hiding this comment.
현재 Points 객체를 보면 빈 boolean 리스트가 생성이 되고 연결 여부가 connectLink()를 통해서 완성되어가고 있습니다.
즉, Points 객체가 생성되는 시점은 미완으로 관리되다가 connectLink()이후에 완성이 되어가는 상황으로 볼 수 있을 것 같습니다.
해당 코드가 잘못된 것은 아니지만 이렇게 구성이 되면 Random을 이용하다보니 테스트를 하는 것이 더욱 어려울 것 같습니다.
이번 사다리의 핵심은 연속으로 건널 수 있는 부분(|-----|-----|)이 등장하지 않는 것이 핵심이었습니다.
그렇다면 List을 파라미터로 받는 생성자에서 해당 조건(연속으로 건널 수 있는 부분 포함 여부)을 검증하는 방향으로 구성하면 테스트 하는데 용이할 것 같습니다.
테스트에서는 어떤 사다리 모양이 등장했는지는 랜덤이다보니 모를 순 있으나 사다리 내에 연속으로 건널 수 있는 부분의 유무를 확인할 수 있을 것 같습니다!
There was a problem hiding this comment.
이번 미션을 진행해보시면서 테스트를 하기에 용이한 코드를 작성해보는 것도 좋을 것 같습니다!
There was a problem hiding this comment.
오호 좋네요!
너무 구현에만 집중하다보니 놓친 부분들이 많네요 ㅠㅠ
한번 피드백 내용으로 리팩토링 좀 해보겠습니다~! :)
| @@ -0,0 +1,16 @@ | |||
| package ladder; | |||
|
|
|||
| public class Line { | |||
There was a problem hiding this comment.
Line 객체는 Points를 한번더 Wrapping한 객체인데요 혹시 Wrapping 하신 이유가 있으신가요?
현재 따로 기능이 있는 것이 아니어서 wrapping하신 이유가 궁금합니다
There was a problem hiding this comment.
안녕하세요 석환님~
요구사항 중에 일급컬렉션을 사용하란게 있어서, 우선 컬렉션들에 대해서 감싸본거긴 합니다 ㅎㅎ
저도 약간 무지성으로 바꾼 감이 있는데, 미션을 해보면서 필요여부에 따라 풀거나 해보겠습니다~!
There was a problem hiding this comment.
제가 느낀 것은 Points나 Lines 객체가 현재 일급컬랙션인데 이를 한번더 wrapping을 하신 이유가 궁금했습니다! (Points는 Line으로 Lines는 Ladder로 wrapping 하신 이유가 궁금합니다)
src/main/java/ladder/Ladder.java
Outdated
| public class Ladder { | ||
| private final Lines lines; | ||
|
|
||
| public Ladder(final Size size) { | ||
| List<Line> lines = new ArrayList<>(); | ||
| for (int i = 0; i < size.getHeight(); i++) { | ||
| lines.add(new Line(size.getWidth())); | ||
| } | ||
| this.lines = Lines.of(lines); | ||
| } | ||
|
|
||
| public Lines getLines() { | ||
| return lines; | ||
| } |
There was a problem hiding this comment.
여기도 앞선 리뷰와 같습니다
Lines 객체를 한번더 wrapping한 이유가 궁금합니다
| package ladder.io; | ||
|
|
||
| import ladder.Size; | ||
|
|
||
| public interface InputHandler { | ||
|
|
||
| Size inputSize(); | ||
| } |
There was a problem hiding this comment.
이렇게 설계를 하면 2단계로 넘어갔을 때 기존 코드를 수정하는 대신에 사용자 입력을 받는 클래스를 생성하면 되니
객체 지향 원칙 중 OCP를 만족할 수 있겠네요!
안녕하세요 석환님 |
|
@seokhwan-an 안녕하세요 석환님 우선 테스트 코드를 만들면서 Point 부분을 석환님 피드백처럼 생성자에서 List를 주입받는 형태로 했고, 여기서 고민되는 점은 OCP를 위해서 PointGenerator라는 인터페이스를 LadderApplication에서 주입을 시작하여
|
There was a problem hiding this comment.
석주님 리뷰 반영하시느라 고생하셨습니다.
리뷰 반영하신 부분에 궁금한 점과 수정했으면 하는 점 남겼으니 확인해주세요!
여기서 고민되는 점은 OCP를 위해서 PointGenerator라는 인터페이스를 LadderApplication에서 주입을 시작하여
Line까지 내려오는 형태인데요.
사실 Ladder가 PointGenerator를 알 필요는 없다고 생각은 합니다만,
애플리케이션 시작 지점에서 주입 관리를 해줘야 관리가 편할까(?) 생각해서 우선은 넣어봤는데, 더 좋은 아이디어가 있을지 석환님께
자문을 구해봅니다. 시간 괜찮으실 때 천천히 확인 부탁드립니다.
감사합니다!
Ladder는 Point들로 구성되어 있기 때문에 지금처럼 직접적인 필드로 가지는 것은 아니지만 잠시동안 PointGenerator와 협력을 해야한다고 생각합니다. 특히 테스트를 하기에도 이 방식이 용이하다고 생각합니다.
다만 석주님이 생각하는 것처럼 Ladder객체가 PointGenerator를 직간접적으로 알지 못하게하는 방안이 있을 것 같습니다. 떠오른 방안은 생성 책임 객체를 따로 분리해서 관리하는 방안이 있을 것 같습니다.
예를 들면 사다리 생성책임을 가지는 LadderGenerator 객체를 두어 해당 객체는 Ladder를 생성하는 것에만 책임을 지고 Ladder와 관리된 비즈니스 로직은 Ladder에서 처리하는 방안이 있을 것 같습니다.
저는 해당 방안은 객체가 생성되는 과정이 복잡하거나 여러 방식으로 생성된다면 분리하는 것을 고려할 것 같지만 그렇지 않으면 지금처럼 관리하는 것도 좋은 것 같습니다!
src/test/java/ladder/SizeTest.java
Outdated
| @Test | ||
| void normalSize() { | ||
| final int width = 10; | ||
| final int height = 10; | ||
| final Size size = new Size(width, height); | ||
|
|
||
| assertThat(size.getWidth()).isEqualTo(width); | ||
| assertThat(size.getHeight()).isEqualTo(height); | ||
| } |
There was a problem hiding this comment.
저 개인적으로는 생성시 예외가 발생하지 않는다는 것을 테스트 하는 것이 목적이라면
각 필드를 검증하는 것 보다는 assertDoesNotThrow()로 테스트를 진행하는 것이 목적에 더 알맞다고 생각합니다!
There was a problem hiding this comment.
아 이런 메서드도 있군요 맨날 잊어먹네요 ㅎㅎ
리마인드 주셔서 감사합니다! :)
| @Test | ||
| void pointTest() { | ||
| List<Boolean> points = List.of(true, false, true, false); | ||
| assertDoesNotThrow(() -> Points.of(points)); | ||
| } | ||
|
|
||
| @Test | ||
| void errorTest() { | ||
| List<Boolean> points = List.of(true, true, true, false); | ||
| assertThatThrownBy(() -> Points.of(points)) | ||
| .hasMessage("연속된 가로선은 올 수 없습니다."); | ||
| } |
|
|
||
| @Override | ||
| public List<Boolean> generate(int width) { | ||
| final ArrayList<Boolean> points = new ArrayList<>(); |
There was a problem hiding this comment.
구현체를 이용하기보다는 다형성을 이용할 수 있게 추상체인 List<Boolean>을 이용하는 것이 좋을 것 같습니다.
| return points; | ||
| } | ||
|
|
||
| public void connectLink(ArrayList<Boolean> points, int position) { |
src/main/java/ladder/Points.java
Outdated
| public static Points of(List<Boolean> points) { | ||
| return new Points(points); | ||
| } |
There was a problem hiding this comment.
현재 이 정적팩토리 메소드는 이미 존재하는 생성자로도 충분이 대체될 수 있을 것 같은데 정적팩토리 메소드를 만드신 이유는 무엇인가요?
There was a problem hiding this comment.
아 요건 강의를 보면서 강사님의 스타일을 한 번 따라해봤습니다 ㅎㅎ
지금 현재 Points를 만들 땐 단순하기에 바로 생성자를 써도 되지만,
무언가 의미있게 인스턴스를 만들고 싶을 땐 정적팩토리 메서드에 이름을 적절히 부여하여 쓴다고 들어서 한 번 차용해봤습니다.
(지금은 단순히 of지만 말이죠 ㅎㅎ..!)
이번 미션 때 한 번 따라해보면서 어떤 스타일이 더 좋은지 한 번 확인코자 합니다 ㅎㅎ
안녕하세요.
1단계 작성해서 PR 올립니다!
시간 괜찮으실 때 천천히 확인 부탁드립니다!
이번 미션은 생각보다 머리가 잘 안 돌아가서 일단 힌트 기반으로 구현은 해보았는데,
지금 구현한 걸로 과연 사다리 게임처럼 동작될 수 있을까는 아직 의문이네요 ㅠㅠ
현재로는 단순 Boolean을 통해 양쪽 라인이 연결된것처럼 보이게는 해두었습니다.
가령, 1번이 true면 2랑 연결되어있는 느낌으로.. 근데 2번 라인에서는 1번으로의 이동이 안 될꺼 같네요 ㅎㅎ
일단 1단계에만 충실하는 걸로 했고, 단계가 높아질 수록 변경되는 요구사항에 맞춰 한 번 리팩토링 해보겠습니다.
(선 배치에 대한 전략도 현재 1단계가 랜덤이니 랜덤 배치에만 집중했습니다)
감사합니다!