1주차 미션 / 서버 2조 정상현 #23
Conversation
Mr-Planner
requested review from
hamhyeongju
and removed request for
hamhyeongju
hamhyeongju
left a comment
hamhyeongju
left a comment
There was a problem hiding this comment.
코드 리뷰 요청을 이제야 봤네요. 늦었지만 코드 잘 봤습니다!
전반적으로 코드를 잘 작성해 주셔서 오히려 눈에 불을 켜고 보느라 코멘트를 많이 남기게 되었는데 그 만큼 코드를 보기 좋게 잘 짜셨다는 걸로 이해해 주셨으면 좋을 것 같습니다. 최대한 객체 지향적으로 고민하시면서 작성하려고 노력하신게 눈에 보이는 거 같아서 강의 진행하는 파트장 입장에서 즐거운 코드 리뷰였습니다! 특히 몇 가지 지키면 좋을 것들(접근제어자 등)만 지켜주시고 2주 차 내용까지 잘 공부하시면 정말 빠르게 실력이 느실 것 같아요! 2주 차도 파이팅입니다!
|
|
||
| if (! checkDrawn(x,y)) { // If that point has been drawn | ||
| throw new IllegalArgumentException("Point (x,y) has already been drawn"); | ||
| } |
There was a problem hiding this comment.
검증 로직 잘 구현해 주셨군요! 검증 로직도 매우 중요하긴 하나 drawLine 이라는 메서드에 혼재되어 있으면 가독성이 떨어지게 됩니다. 검증 로직을 메서드로 추출해서 사용하시면 drawline 의 기능을 이해하기 좋을 것 같습니다!
|
|
||
| public static Ladder createLadder(NaturalNum row, NaturalNum numberOfPerson) { | ||
| Ladder ladder = new Ladder(new Matrix(row, numberOfPerson)); | ||
| return new Ladder(new Matrix(row, numberOfPerson)); |
There was a problem hiding this comment.
Ladder 의 생성 로직이 반복된건 실수겠죠? 👀
| public Ladder(Matrix matrix) { | ||
| // 사다리 생성하고 크기 지정해야 함 | ||
| this.matrix = new Matrix(matrix.getRow(),matrix.getNumberOfPerson()); | ||
| } |
There was a problem hiding this comment.
정적 팩토리 메서드를 통해 객체의 생성을 제어하고 있다면 생성자는 private 접근자로 제한해도 좋지 않을까요?
| // If that point is last col Line (only can draw to right) | ||
| // If that point has been drawn | ||
|
|
||
| int direction = 1; |
There was a problem hiding this comment.
1이라는 변하지 않는 내부 상수가 matrix.setDirection() 으로 전달되는 거라면 굳이 사용하지 않고 matrix 내부에서 제어하는 것이 좋지 않을까요?
| return false; | ||
|
|
||
| return true; | ||
| } |
There was a problem hiding this comment.
검증 메서드는 외부에서 사용하지 않는 것으로 보입니다. private 접근자로 제한하면 의도치 않은 사용을 막을 수 있어요!
| } | ||
|
|
||
| public int runLadder(NaturalNum x) { | ||
| int starter = x.getNaturalNum() - 1; |
There was a problem hiding this comment.
굳이 NaturalNum으로 값을 받아서 -1을 해줘야 한다면 NaturalNum 말고 다른 검증 패턴을 가진 클래스가 필요하지 않을까요?
| break; | ||
|
|
||
| default: | ||
| throw new IllegalArgumentException("There is unacceptable value in ladder"); |
There was a problem hiding this comment.
유효성 처리 이후 로직에 같은 예외를 발생하는 로직이 왜 필요할까요? 여기서 해당 예외가 발생하는 건 이미 유효성 검증에 실패한 코드라는 방증인 것 같습니다. 마치 유효성 검증을 끝낸 값을 디비에 저장하고 후에 조회할 때 다시 검증하는 것과 비슷해보입니다!
|
|
||
| case 1: // 일단 오른쪽으로 가면 다음은 무조건 내려가야 함 | ||
| starter = moveRight(starter); | ||
| currentRow = moveDown(currentRow); |
There was a problem hiding this comment.
moveRight 메서드 이후에 moveDown 동작이 반드시 후행되어야 한다면 해당 로직을 moveRight에 포함시키면 좋을 것 같습니다.
| int finalLine = 0; | ||
|
|
||
|
|
||
| while (currentRow < row.getNaturalNum()) { // 높이 만큼 반복 |
There was a problem hiding this comment.
fori 문으로 하면 가독성도 좋고 인덱스 값을 직관적으로 확인하기에도 좋을 것 같습니다.
| assertThat(ladder.run(NaturalNum.from(2))).isEqualTo(4); | ||
| // assertThat(ladder.run(NaturalNum.from(1))).isEqualTo(4); | ||
| } | ||
|
|
There was a problem hiding this comment.
유효성 검증에 대한 테스트가 없는 것이 아쉽습니다! 유효성 검증은 어떻게 보면 핵심 비지니스 로직 만큼이나 중요하기 때문에 테스트에 포함되는 것이 바람직합니다! 검증에 대한 테스트 케이스가 없다면 리팩토링 이후에 해당 코드가 예외 상황을 잘 컨트롤한다고 확신할 수 있을까요?
|
자세한 리뷰 감사합니다 ! |
2 participants
부족하지만 최선을 다했습니다...
감사합니다.