1주차 미션/ 서버 4조 임동준 #3
Conversation
| public class LadderCreator { | ||
| private final Row[] rows; | ||
|
|
||
| public LadderCreator(NaturalNumber numberOfRow, NaturalNumber numberOfPerson) { |
There was a problem hiding this comment.
사다리의 행, 열의 개수를 필드로 가지는 LadderSize 클래스를 생성해보는 것은 어떨까요? 힌트를 조금 더 드리자면LadderCreator는 LadderSize를 필드로 갖게 되고 생성자에도 행, 열의 개수를 모두 주입 받는 것이 아닌 LadderSize만 주입 받는 방식으로 수정 가능할 것 같습니다:)
+) numberOfPerson 의 변수명과 역할에 괴리감이 있는 것 같아요. 다른 이름으로 변경하는 것이 좋아 보입니다.
| return rows; | ||
| } | ||
|
|
||
| public void drawLine(Position row, Position col) { |
There was a problem hiding this comment.
해당 부분도 마찬가지로 row와 col을 모두 필드로 가지는 LadderPosition 클래스를 생성해볼 수 있겠네요:)
| this.ladderCreator = ladderCreator; | ||
| } | ||
| public LadderRunner startGame(){ | ||
| return new LadderRunner(ladderCreator.getRows()); |
There was a problem hiding this comment.
메서드명으로 보아 사다리 게임을 시작하는 메서드 인 것 같아요. LadderRunner를 생성하는 것은 잘 하셨지만 run() 을 추가로 호출해야할 필요가 있어보입니다. 추가로 run 메서드는 최종 위치를 반환한다는 점을 참고하여 리턴 타입도 알맞게 수정해주시면 좋을 것 같아요 👍
| return num; | ||
| } | ||
| private void validate(int num){ | ||
| if(num<1){throw new IllegalArgumentException(String.valueOf(ExeptionMessage.NOT_NATURAL_NUMBER));} |
There was a problem hiding this comment.
num < 1 부분을 메서드로 따로 추출해보는 것은 어떨까요? 아주 사소한 차이이지만 훨씬 더 가독성 좋은 코드를 만들 수 있을 것 같습니다.
| validateNumberOfPerson(numberOfPerson); | ||
| row = new int[numberOfPerson]; | ||
| row = new int[numberOfPerson.getNum()]; | ||
| } |
There was a problem hiding this comment.
로직이 하나 빠진 것 같습니다. row가 배열이라는 점을 유의해주세요!
| throw new IllegalArgumentException("라인 생성이 불가능한 위치 입니다."); | ||
| private void validateDrawLinePosition(Position lineStartPosition) { | ||
| int pos = lineStartPosition.getPosition(); | ||
| if(pos < 0 || pos >= row.length - 1 || row[pos] == -1 || row[pos + 1] == 1) { |
There was a problem hiding this comment.
해당 내부 로직을 Row 클래스에서까지는 알 필요 없어보여요. 정확히는 pos의 범위를 체크하는 로직은 Row 클래스의 책임이 아닌 Position의 책임이라는 말이 더 맞는 것 같네요. Position 클래스에 isBiggerThan(int), isSmallerThan(int) 와 같은 메서드를 만들어 범위를 체크하는 책임을 넘기고 Row 클래스는 이를 호출하여 판별 여부만 반환 받도록 수정하면 책임도 분리하고 훨씬 깔끔한 코드를 작성할 수 있을 것 같습니다.
| private void validateDrawLinePosition(Position lineStartPosition) { | ||
| int pos = lineStartPosition.getPosition(); | ||
| if(pos < 0 || pos >= row.length - 1 || row[pos] == -1 || row[pos + 1] == 1) { | ||
| throw new IllegalArgumentException(String.valueOf(ExeptionMessage.INVALID_LADDER_POSITION)); | ||
| } | ||
| } | ||
|
|
||
| private void validatePosition(int position) { | ||
| if(position >= row.length || position < 0 ) { |
There was a problem hiding this comment.
이 부분도 마찬가지로 position 의 범위를 체크하는 책임은 Position 클래스가 가져가는게 좋아보입니다.
| } | ||
|
|
||
| public int nextPosition(int position) { | ||
| public Position nextPosition(Position position) { | ||
| int newPosition = position.getPosition(); |
There was a problem hiding this comment.
newPosition 이라는 변수명에 현재 Position을 가져오는 것은 맞지 않는 것 같아요. 변수명을 수정해주는 것이 좋아보입니다.
| if (isRight(position)) { | ||
| return position + 1; | ||
| if (isRight(newPosition)) { | ||
| return position.next(); |
There was a problem hiding this comment.
Node 클래스에 move()를 만들어주었다는 점을 다시 기억해 보도록 해요!
위에서 int[]가 아닌 Node[] 가 쓰여야 하는 이유가 여기서 설명될 수 있을 것 같아요 👍
| @@ -0,0 +1,21 @@ | |||
| package ladder; | |||
|
|
|||
| public class LadderCreator { | |||
There was a problem hiding this comment.
미션의 추가 기능2: 사다리 자동 생성 부분이 아직 구현되지 않았지만 추후의 확장성을 고려해본다면 LadderCreator는 인터페이스로 만드는 것이 좋아 보입니다. 그렇게 된다면 현재 구현되어 있는 이 클래스는 LadderCreator 인터페이스를 구현하는 구현체로 만들어 줄 수 있겠네요.
추가 기능 2부터 천천히 보시면서 LadderCreator 인터페이스를 만들어 다형성을 활용한다면 어떤 이점을 가지게 될지 꼭 고민해보시면 좋을 것 같아요! 😄
리팩토링까지 하다가 완성 못했습니다,,