2주차 미션 / 서버 4조 김재윤 #30
Conversation
hamhyeongju
left a comment
hamhyeongju
left a comment
There was a problem hiding this comment.
코드 잘 봤습니다. 아직 객체와 클래스에 적절한 책임을 부여하는게 익숙치 않으신 것 같은데, 이 부분 더 고민해보시면 좋을 것 같습니다. 객체에 알맞은 책임을 부여하고 그에 맞는 일을 하는 메서드를 잘 작성하면 객체간 결합도가 낮아지고, 코드의 응집도가 올라가서 더 가독성 좋은 코드를 작성할 수 있게 됩니다!
질문해주신 부분에 대해서는 LadderRunner에서 run 메서드를 통해 Positon을 받아 실행하는거라 클라이언트가 Position을 결정하게 되는 구조로 생각하고 코드를 작성한거로 치면, 사용자가 잘못된 값을 입력했을 때 예외가 잘 발생한다면 그거로 괜찮은 것 아닐까요? 제가 재윤님의 코드만 보는게 아니라 실행해보고 디버깅해보면서 깊게 고민할 시간이 없어 자세한 맥락은 모르겠지만 사용자의 잘못된 입력에 대해서 예외가 잘 내려간다면 문제 없을 것 같습니다.
실제로 클라이언트와 상호작용하는 코드(프레젠테이션 레이어)가 추가된다면 해당 부분에서 예외를 잡아 적절한 응답을 내려주면 해결될 일인 것 같습니다.
어려운 미션 하시느라 수고 많으셨습니다. 코드 리뷰와 2week-completed 브랜치 보시면서 어느 부분을 더 리팩토링 했으면 좋았을지 고민해 보시면 좋을 것 같습니다.
| rows[0].validatePosition(ladderPosition.getCol()); //validatePosition()를 public 으로 변경, 지저분한데... | ||
| //사다리 출력 | ||
| for (Row row : rows) { | ||
| ladderPosition.next(); //불필요하게 한줄 추가된 거 아닌가? |
There was a problem hiding this comment.
for-each 문이 아니라 fori 문을 사용하면 인덱스 처리를 쉽게 할 수 있을 것 같네요
There was a problem hiding this comment.
좌표를 하나의 LadderPosition으로 묶어서 사용하라 한 것에 너무 사로 잡혀
생각이 유연하지 못했던 것 같습니다.
position이 아닌 ladderPositon만을 사용해야 하는 느낌으로...
LadderRunner 클래스에서는 계속해서 position을 사용하고
다른 클래스의 함수의 인자로 넣어줄 때 ladderPosition을 넘겨주는 방향으로 수정하였습니다.
| //사다리 출력 | ||
| for (Row row : rows) { | ||
| ladderPosition.next(); //불필요하게 한줄 추가된 거 아닌가? | ||
| printLadderRow(ladderPosition, AFTER); //기존에는 rows[i].nextPosition(position)에서 시작위치 유효성 검사를 해줬는데 해주기 전에 그려버리니까 오류 발생 |
There was a problem hiding this comment.
어떤 오류가 발생하는지도 적어주시면 좋을 것 같아요...
그리고 아래 BEFORE이 파라미터로 전달되는 메서드가 앞에 위치해야 하는 것 아닌가요?
There was a problem hiding this comment.
해당 for 문 전에
rows[0].validatePosition(ladderPosition.getCol()); 을 실행시켜서
run(ladderPosition); 에 들어온 ladderPosition -> 의 col 즉 시작 열을 검사하여
주석에서 언급한 시작위치 에 따른 오류가 발생하지는 않습니다.
구현하면서 '오류 발생하네?' 하면서 남겨논 주석이라...
AFTER, BEFORE은 잘못 적어놨네요
src/main/java/ladder/Row.java
Outdated
| rowStringBuilder = new StringBuilder(); | ||
|
|
||
| for (Node node : nodes) { | ||
| rowStringBuilder.append(node.getNodeValue()).append(" "); |
There was a problem hiding this comment.
getNodeValue 를 통해 값을 가져오는 것이 아니라 해당 메서드를 Node에서 작성해보는게 어떨까요?
|
|
||
| for(int i = 0; i < rows.length; i++) { | ||
| if(i != ladderPosition.getRow()){ | ||
| sb.append(rows[i].getRowStringBuilder()); |
There was a problem hiding this comment.
Row에서 StringBuilder를 생성해서 넘겨준 것을 합치는게 아니라 위에서 생성한 sb를 파라미터로 넘겨서 사용하면 더 깔끔할 것 같습니다!
| } | ||
| if(i == ladderPosition.getRow()){ | ||
| sb.append(rows[i].getRowStringBuilder(ladderPosition.getCol())); | ||
| } |
There was a problem hiding this comment.
이중 들여쓰기가 되었네요. 메서드 추출이나 메서드의 위치, 작성 방식의 변경을 고민해 보시면 좋을 것 같습니다!
| if(rowStringBuilder.indexOf("*") != -1){ | ||
| rowStringBuilder.deleteCharAt(rowStringBuilder.indexOf("*")); | ||
| } | ||
| } |
There was a problem hiding this comment.
현재 코드가 너무 복잡해서 가독성이 너무 떨어지는 것 같아요.. 제가 부족한 탓이 크겠지만 솔직히 당장 코드만 보고 어떤 맥락으로 코드가 진행되는지 알기 너무 힘들 것 같습니다. 특히 객체에 부여된 책임과 메서드가 하는 일이 제대로 정의되어 있지 않은 것 같은데 리팩토링이 필요해 보입니다!
어렵게 생각할 필요 없이 반복문을 통해 모든 좌표를 탐색하기 때문에 Node는 그냥 자신의 값을 rb에 추가하면 될 것 같고 추가로 Row에선 LadderPosition과 출력하는 좌표가 같은지 확인하는 로직만 있어도 충분할 것 같습니다.
잘 이해가 안되시면 2week-complete 참고하시면 좋을 것 같습니다!
| this.col = position; | ||
| } | ||
|
|
||
| public static LadderPosition autoLadderPosition(int numberOfCol, int numberOfRow){ |
There was a problem hiding this comment.
LadderAutoCreator가 랜덤 처리에 관한 역할을 하고 있는 것 같은데 LadderPosition에서 랜덤 값을 직접 다루는 건 해당 클래스의 책임을 벗어나는 것 같습니다.
LadderAutoCreator 에서 랜덤 값을 다루고, 생성된 랜덤 값을 단순히 파라미터로 받아 LadderPosition 객체를 생성하는게 바람직해 보입니다.
|
|
||
| for(int i = 0; i < numberOfLine; ){ | ||
| if(canDrawLine(LadderPosition.autoLadderPosition(numberOfCol.getValue(), numberOfRow.getValue()))){ | ||
| i++; //실제로 Line이 그려진 경우 for문의 i를 증가시킴 |
There was a problem hiding this comment.
미션 힌트에서 언급했듯이 HashSet의 size를 비교를 통해 while 반복문을 구현했다면 i 값의 제어를 개발자가 고민할 필요가 없었을 것 같네요. 반드시 HashSet을 이용한 코드일 필요는 없지만 HashSet을 사용했을 때가 더 가독성이 좋은 코드가 될 것 같습니다!
|
|
||
| private final Row[] rows; | ||
|
|
||
| public LadderAutoCreator(GreaterThanOne numberOfRow, GreaterThanOne numberOfPerson) { |
There was a problem hiding this comment.
미션 힌트에서 조합을 이용해 LadderAutoCreator를 구현해보면 좋겠다고 언급했었는데, 만약 조합을 잘 이용했다면 이미 구현해놓은 drawline 부분을 재사용할 수 있었을 것 같습니다!
2 participants
질문 )
사다리 요구사항 2를 구현하기 위해, 사다리의 현재 상황을 출력하기 위한 작업을 하는 과정에서 생긴 의문점
ladderGame.run(position)을 작동시켜 ladderRunner.run(position)이 작동되는 시점에서
ladderRunner.run(position) 함수 안에서 사다리의 현재 상태와 사다리의 (좌우)이동 후 상태를 출력하도록 구현하였습니다.
하여 현재 사다리 출력 > rows[i].nextPosition(position) 로 position 이동 > 이동 후 사다리 출력 의 작업을 진행하였습니다.
기존 구현에서 position의 유효성 검사를 rows[i].nextPosition(position)에서 진행하였습니다.
처음에 유효하지 않은 position이 주어지게 되면 rows[i].nextPosition(position)에서 유효성 검사를 하기 전에
현재 사다리를 출력해서 잘못된 col을 접근했다는 오류가 발생하게 됩니다.
이를 방지하기 위해 ladderRunner.run(position)의 맨 처음에도 position의 유효성 검사가 들어가야 하는데
기존에 사용하던 유효성 검사 관련 함수들이 private로 설정되어 있습니다.
(Row.isInvalidPosition or Row.isInvalidPosition)
해당 함수 중 하나를 public 으로 변경하는 방향과 해당 private 함수를 사용하는 public 함수를 생성하는 방향 중 어느 방향이 더 옳은 방향인지 궁금합니다.