1주차 미션 / 서버 2조 박다솔 #9
Conversation
There was a problem hiding this comment.
기존 코드와 비교하였을 때, 객체를 나누어서 더 객체지향적인 코드가 된 것 같아서 좋습니다 👍
비록 Ladder 객체에 모든 역할을 할당하고 코드를 짜면 편할 수 있겠지만,
"외부의 도움을 무시하고 혼자 모든 것을 스스로 처리하려고 하는 전지전능한 객체(god object)는 내부적인 복잡도에 의해 자멸하고 만다" 라는 말을 봤을 때, 객체가 맡는 책임이 너무 많아지면 유지보수나 확장성에서 문제가 발생할 수 있다는 점을 유의하면서 코드를 짜면 좋을 것 같습니다.
특히 2차원 배열이 아닌 Row, Node로 나누어서 각각 객체에게 역할을 분배하게 해서 한층 더 객체지향적인 코드가 되지 않았나 생각이 듭니다.
고생하셨습니다!!
src/main/java/Direction.java
Outdated
There was a problem hiding this comment.
enum을 활용하여 Direction을 관리하니 코드의 가독성이 좋아진 것 같아요!
src/main/java/Ladder.java
Outdated
There was a problem hiding this comment.
Ladder 클래스가 너무 많은 일을 하고 있어서, draw와 run을 별도의 클래스로 나누면 관리가 더 쉬워질 것 같아요. 예를 들어, LadderCreator와 LadderRunner 클래스를 만들어 각각 그리기와 실행을 맡기면 확장성도 좋아지고, 2주차 미션처럼 요구사항이 바뀔 때도 더 유연하게 대응할 수 있을 것 같아요.
src/main/java/Ladder.java
Outdated
| @@ -1,8 +1,30 @@ | |||
| public class Ladder { | |||
|
|
|||
| private final int[][] rows; | |||
| private Row[] rows; | |||
There was a problem hiding this comment.
rows는 생성된 이후 변경되지 않으므로 final을 붙여주는 게 더 안전할 것 같아요!
src/main/java/Ladder.java
Outdated
| public int run(int start) { | ||
| Position result = Position.of(start); | ||
| for (int i = 0; i < rows.length; i++) { | ||
| rows[i].move(result); | ||
| } | ||
| System.out.println(result.getValue()); | ||
| return result.getValue(); | ||
| } |
There was a problem hiding this comment.
처음 start 매개변수를 받아서, result 변수명으로 저장하는 것은 오해가 생길 수 있을 것 같아요. 간단히 position 이나 currentPosition 같은 변수명은 어떨까요?
| public static Node of(Direction state) { | ||
| return new Node(state); | ||
| } |
There was a problem hiding this comment.
정적 팩토리 매서드를 사용하는 모습 아주 좋습니다. 덕분에 생성자를 직접 new키워드를 사용하지 않아서 가독성도 좋아진 것 같습니다. 정해진 것은 없지만
of는 매개변수를 하나 받아서 해당 타입의 인스턴스를 반환하는 형변환 메소드from는 여러 매개변수를 받아 적합한 타입의 인스턴스를 반환하는 집계 메소드
이런식으로 사용하고 있는 중이니 참고하시면 좋을 것 같아요
src/main/java/Row.java
Outdated
| public void drawLineOnNode(Position position) { | ||
| // 입력 검사 | ||
| if (!validateDrawPosition(position)) return; |
There was a problem hiding this comment.
이 부분 왜 boolen으로 하고서 return했는지 궁금합니다.
이미 저 메서드 안에서 throw 예외 처리를 해줬는데, 한번 더 해서 return처리를 해주면서 혼동을 줄 수 있을 것 같아요.
void로 바꾸고
validateDrawPosition(position);
이런식으로 바꾸면 어떨까요?
src/main/java/Row.java
Outdated
| if (position.getValue()<0 || position.getValue()>=nodes.length-1){ | ||
| throw new IllegalArgumentException("잘못된 사다리 위치입니다."); | ||
| } |
There was a problem hiding this comment.
비교문안에 있는 내용을 따로 메서드로 빼서 하면 가독성이 좋을 것 같아요!
src/main/java/Row.java
Outdated
|
|
||
| //시작위치가 적절한지 확인 | ||
| private boolean validateStartPosition(Position position){ | ||
| if (position.getValue()<0 || position.getValue()>=nodes.length){ |
There was a problem hiding this comment.
추가로 position의 값을 비교하는 부분을 Position 객체에게 책임을 주는 것은 어떨까요??
Position 객체를 만들었다면, 단순히 좌표를 저장하는 역할뿐만 아니라, 위치 검증 역할도 부여하면 더 객체지향적이지 않을까요?
어떤식으로 Position객체에게 역할을 줄 수 있을지 고민해보면 좋을 것 같습니다.
(다만, 이렇게 하면 Position이 Ladder의 정보를 알아야 해서 결합도가 올라갈 수도 있습니다. 이 부분을 어떻게 풀어갈지 고민해보면 좋을 것 같습니다.)
src/main/java/Row.java
Outdated
| private boolean validateStartPosition(Position position){ | ||
| if (position.getValue()<0 || position.getValue()>=nodes.length){ | ||
| throw new IllegalArgumentException("잘못된 시작 위치입니다."); | ||
| } | ||
| return true; | ||
| } |
There was a problem hiding this comment.
이 부분도 위의 코멘트에서 말씀드린 두 부분
- void형으로 바꾸면 어떨지?
- if문 안을 메서드로 추출해서, 가독성을 높여보기
을 고려해보면 좋을 것 같아요
src/test/java/LadderTest.java
Outdated
There was a problem hiding this comment.
여러 테스트도 시도해보면 좋을 것 같아요.
- 사다리가 일직선으로 연결되어 생성되는 테스트
validateDrawPosition메서드 테스트.(사다리가 잘못된 위치에 생성되려고 할때)
No description provided.