-
Notifications
You must be signed in to change notification settings - Fork 26
2주차 미션/ 서버 4조 이정연 #35
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 1week-completed
Are you sure you want to change the base?
Conversation
hamhyeongju
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
코드 잘 봤습니다.
전반적으로 코드를 깔끔하게 작성해주셔서 코드 리뷰드리기 수월했네요.
코드 리뷰와 2week-completed 코드 보시면서 본인의 코드와 비교해보는 시간을 가져보시면 좋을 것 같습니다. 수고 많으셨습니다!
| for (int i = 0; i < rows.length; i++) { | ||
| LadderPosition ladderPosition = new LadderPosition(Position.from(i), position); | ||
|
|
||
| System.out.println("Before"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before와 After 로직을 enum으로 분리해 보시는게 어떨까요?
| private void printLadder(LadderPosition ladderPosition){ | ||
| StringBuilder sb = new StringBuilder(); | ||
| for(int i = 0; i < rows.length; i++){ | ||
| if( i == ladderPosition.getX().getValue()){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LadderPosition에서 getX로 값을 가져와서 비교하는건 객체 지향적이지 못하다는 생각이 듭니다!
| } | ||
|
|
||
| public Direction getDirection() { | ||
| return direction; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
노드의 값을 외부로 노출시키지 않고 노드 내부에서 처리할 수 있도록 바꾸는게 좋을 것 같습니다!
| return sb; | ||
| } | ||
|
|
||
| public StringBuilder convertPositionRowToString(Position y){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
convertRowtoString() 와 convertPositionRowToString() 는 중복되는 부분이 있는 것 같습니다. 하나의 메서드를 사용하되, *을 append 하는 부분은 메서드로 따로 추출하여 사용하면 해당 코드를 사용하는 입장에서도 사용 방법에 대해 명확히 이해할 수 있을 것 같습니다.
| public void drawLine(Position row, Position col){ | ||
| Set<LadderPosition> lines = new HashSet<>(); | ||
|
|
||
| int numberOfLines = (int)(rows.length * rows[0].getNodeLength() * 0.3) - 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LadderSize 클래스를 활용해서 해당 객체를 구성하면 코드를 직관적으로 만들 수 있지 않을까요?
| public Row[] getRows() {return rows;} | ||
|
|
||
| public void drawLine(Position row, Position col){ | ||
| Set<LadderPosition> lines = new HashSet<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Set 같은 컬렉션에 객체를 저장하여 활용하려면 LadderPosition 에서 equals, hashCode 메서드를 오버라이드해야 중복 검증을 제대로 할 수 있습니다.
| @@ -0,0 +1,19 @@ | |||
| package ladder; | |||
|
|
|||
| public class LadderSize { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LadderSize 클래스가 사용되고 있지 않은 것 같은데 기분탓일까요..?
No description provided.