2주차 미션 / 서버 3조 장현준 #34
Conversation
| @@ -58,4 +58,25 @@ private boolean isLineAtNextPosition(Position position) { | |||
| return lineAtPosition; | |||
| } | |||
|
|
|||
| public void printRow(StringBuilder sb) { | |||
| for (int i = 0; i < nodes.length; i++) { | |||
| nodes[i].printNode(sb); | |||
| for (int i = 0; i < nodes.length; i++) { | ||
| nodes[i].printNode(sb); | ||
|
|
||
| if (position.isSamePosition(i)) { |
There was a problem hiding this comment.
값 비교를 메소드로 작성하니 가독성도 늘고, 객체 내부에 어떤 데이터가 있는지 자세히 몰라도 되네요! 객체지향적인 코드의 장점이 보이죠?
| if(set.contains(pair)) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
혹시 디버깅하면서 중복확인이 되는지 확인해보셨나요? 제가 찾아보니까 HashSet의 contains()는 두 객체의 주소가 같은지 비교하기 때문에 중복 확인이 안 될 수도 있을 것 같습니다
| rows[i] = new Row(numberOfPerson); | ||
| } | ||
|
|
||
| drawAutoRandomLine(ladderSize, numberOfPerson, numberOfRow); |
| return new LadderGame(ladderCreator); | ||
| } | ||
|
|
||
| public static LadderGame createLadderGame(LadderSize ladderSize) { |
There was a problem hiding this comment.
약간 마이너한 이슈일 수 있는데요. 이 팩토리 메소드에 사용자가 지정한 위치에 사다리를 그릴 수 있도록 수정하면 더 좋을 것 같네요. 이 팩토리 메소드로는 가로선이 없는 사다리를 가진 game 객체만 얻을 수 있으니까요.
| @@ -0,0 +1,15 @@ | |||
| package ladder; | |||
|
|
|||
| public enum TimePeriod { | |||
There was a problem hiding this comment.
enum을 통해서 올바른 매개변수의 값만 사용할 수 있게 한 점 좋아요!
| } | ||
|
|
||
| public int calLineSize() { | ||
| return (int)(numberOfPerson.getNumber()*numberOfRow.getNumber()*0.3); |
There was a problem hiding this comment.
랜덤으로 그릴 사다리 라인 값을 정하는 로직이 LadderSize의 책임 중 하나인지 고민해 보시면 좋을 것 같습니다!
| sb.append("\n"); | ||
| } | ||
|
|
||
| public void printStarRow(Position position, StringBuilder sb) { |
There was a problem hiding this comment.
printRow와 printStarRow는 로직이 매우 비슷해 보입니다. printRow에서 position과 비교하여 *을 append하는 로직만을 메서드로 따로 분리하면 가독성이나 클라이언트 코드 입장에서도 어떻게 코드를 다뤄야 할지 명확할 것 같습니다.
| package ladder; | ||
|
|
||
| public enum TimePeriod { | ||
| BEFORE("Before"), AFTER("After"); |
| //생성해야 하는 line 개수 | ||
| int lineSize = ladderSize.calLineSize(); | ||
|
|
||
| HashSet<PositionPair> set = new HashSet<>(); |
There was a problem hiding this comment.
Set 같은 컬렉션에 들어가는 객체는 equals, hashCode 메서드가 구현되어 있어야 제대로 된 중복 검증이 가능합니다!
| PositionPair pair = new PositionPair(row, col); | ||
|
|
||
| //중복 확인 | ||
| if(set.contains(pair)) { |
There was a problem hiding this comment.
반복문 조건식으로 count를 사용하는 것이 아니라 set.size() 를 사용했다면, 중복처리와 반복문 조건식 핸들링을 한 번에 할 수 있습니다!
| LadderSize ladderSize = new LadderSize(GreaterThanOne.from(3), GreaterThanOne.from(4)); | ||
| LadderGame ladderGame = LadderGameFactory.createLadderGame(ladderSize); | ||
|
|
||
| LadderCreator ladderCreator = ladderGame.getLadderCreator(); |
There was a problem hiding this comment.
팩토리 클래스를 테스트에서도 사용하려고 하다 보니 어색하게 getLadderCreator 가 쓰인 것 같습니다! 테스트 코드에선 단위테스트를 위해 특정 상황을 만들어 내야 하는 경우가 많기 때문에 꼭 팩토리 패턴을 통해 객체를 생성하지 않으려고 해도 괜찮을 것 같습니다!
|
|
||
| public class PositionPair { | ||
| private final int col; | ||
| private final int row; |
There was a problem hiding this comment.
PositionPair 라면 인스턴스 변수로 int 대신 Position 클래스를 갖도록 구현해 보는건 어떠신가요?
| @@ -25,6 +25,10 @@ public void move(Position position) { | |||
| } | |||
| } | |||
|
|
|||
| public void printNode(StringBuilder sb) { | |||
| sb.append(direction.getValue()); | |||
There was a problem hiding this comment.
node의 값을 외부로 노출하지 않고 값을 append 하는 로직을 잘 작성해 주셨네요. 디미터 법칙을 잘 이해하신 것 같습니다!
| @@ -43,4 +43,8 @@ private static void validatePosition(int position) { | |||
| private static boolean isPosition(int position) { | |||
| return position >= 0; | |||
| } | |||
|
|
|||
| public boolean isSamePosition(int i) { | |||
There was a problem hiding this comment.
equals 메서드를 오버라이드해서 구현하면 의미 전달이 더 잘 될 것 같습니다!
No description provided.