Refactor: comment refactor #221
Conversation
jeongns2611
requested review from
Eseas,
taekminKwon,
K-KY,
kdh10806 and
HiSeungmin
as code owners
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| String modifierIp | ||
| ) { | ||
| this.isActive = YesNo.NO; | ||
| this.content = content; | ||
| this.modifierIp = modifierIp; | ||
| } | ||
|
|
There was a problem hiding this comment.
코드 리뷰 결과는 다음과 같습니다:
-
불필요한 주석 제거:
코드 주석(isActive : 사용 유무 (Y, N))은 명확성과 가독성을 위해 원본 필드의 중복된 설명을 줄이기 위해 제거되었습니다. 다만, 삭제된isActive필드의 목적과 사용이 불분명해질 수 있으니, 해당 내용을 추가하기 위해 다른 설명 방식이 필요할 수 있습니다. -
클래스 구조의 변경:
postId와parentCommentId필드 이름이post와parentComment로 변경되었습니다. 이는 가독성을 향상시키지만, 이전 코드와의 호환성을 깨뜨릴 수 있는 위험이 있습니다. 데이터베이스에서 이 필드들을 참조하는 곳은 없는지 검토가 필요합니다. -
isActive필드 제거:
isActive필드가 제거되었는데, 이 필드가 필요한 비즈니스 로직에 영향을 주지 않는지 확인하는 것이 중요합니다. 이 필드가 댓글의 활성/비활성 상태를 관리하는 데 필요하다면 다른 방법으로 관리해야 할 것입니다. -
생성자 및 메서드 수정:
여러 필드를 생성자에서 제거하고,createComment메서드에서isActive를 하드코딩하여 설정했던 부분이 변경되었습니다. 이 부분 역시,isActive필드가 있는 경우에는 해당 관리 로직이 필요한지 검토가 필요합니다. -
가독성 향상:
메서드와 생성자의 인자 목록이 변경되어 코드가 더 깔끔해졌습니다. 그러나, 인자의 수가 줄어들면서 생성자의 역할이 명확해지고, 관리가 쉬워질 수 있습니다. 하지만 중요한 필드가 생략된 경우와 같은 코드의 의도와 유지 보수성에 관한 논의가 필요합니다. -
버그 리스크:
isActive와 같은 상태 필드 제거 후 관련된 비즈니스 로직이나 후속 처리 로직이 깨지지 않도록 확인해야 합니다.Comment객체의 불변성을 위해createComment에서 외부에서 주입받는 값들을 final로 설정하는 것도 좋은 선택일 수 있습니다.
-
트랜잭션 관리:
데이터베이스와의 연동에서 트랜잭션이 필요한 부분이 있을 경우, 이를 명시적으로 처리하는 부분이 필요합니다. 특히 댓글 생성이나 삭제와 같은 작업은 데이터의 일관성을 위해 트랜잭션으로 묶는 것이 좋습니다. -
추가적인 개선 사항:
modifierIp가 null로 초기화되는 로직 외에, 이 부분도 적절한 기본값을 설정하거나 필요한 경우 필드 초기화를 고려하는 것이 좋습니다.- 코드 전반에 걸쳐 null 체크 및 예외 처리를 추가하는 것도 필수적입니다.
이와 같은 변경 사항들을 고려하여 최종 구현을 검토하시기 바랍니다.
| } | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
코드 리뷰에 대해 의견을 드리겠습니다. 다음은 주요 포인트와 개선 사항입니다:
-
주석 추가: 클래스와 필드에 대한 설명 주석은 좋습니다. 코드의 의도를 쉽게 이해할 수 있게 도와줍니다. 그러나 필요하다면, 메서드에 대한 설명 주석도 추가하는 것이 좋습니다. 예를 들어,
createCommentHistory메서드의 목적과 인자에 대한 설명이 있으면 좋겠습니다. -
필드 유효성 검사: 현재 생성자에서 필드 유효성 검사(예: null 체크, 범위 체크 등)가 없습니다. 특히,
nullable = false로 설정된 필드는 생성 시 null이 될 수 없도록 체크하는 로직이 필요합니다. 예외를 발생시키는 방법으로 유효성을 검증하는 것이 좋습니다. -
생성자 접근 제어자: 생성자가
private로 설정되어 있어 외부에서 직접 인스턴스를 생성할 수 없습니다.createCommentHistory메서드가 유일한 생성 경로인 경우, 이 방식은 적절합니다. 그러나 이 경우 다른 생성 방법이 있을 경우를 대비하여, 생성자를protected또는public으로 변경하는 것도 고려해 볼 수 있습니다. -
IP 주소의 길이: IP 주소의 길이를 50자로 제한하고 있는데, 대부분의 경우 45자(IPv6 포함)로 충분합니다. 이 점 조정할 수 있습니다.
-
기타 Enum 처리:
HistoryActionEnum에 대한 검증이 필요할 수 있습니다. 사용자가 잘못된 액션을 제공할 경우를 대비하여 적절한 예외 처리 로직을 추가하는 것이 좋습니다. -
테스트 케이스: 이 클래스에 대한 단위 테스트 케이스가 필요합니다.
createCommentHistory메서드의 입력에 따른 다양한 상황에 대한 테스트를 추가하여, 다양한 시나리오에서의 동작을 확인할 수 있도록 해야 합니다. -
Nullable 주의:
parentCommentId,ref,refOrder,childCommentNum필드는 nullable로 설정되어 있습니다. 필요한 경우에만 null을 허용하는 것이 좋으며, 이를 문서화하거나 Nullable 어노테이션을 추가하여 코드 가독성을 높일 수 있습니다.
전반적으로 매우 잘 작성된 코드입니다. 몇 가지 개선점들에 신경 쓴다면 더 robust한 코드가 될 수 있을 것 같습니다.
| @@ -77,7 +77,7 @@ public class PostHistory extends BaseEntity { | |||
| private String modifierIp; // 수정자 IP | |||
|
|
|||
| // 게시글 이력 생성 | |||
| public static PostHistory createPostHistory(Post post, PostAction action, String ip) { | |||
| public static PostHistory createPostHistory(Post post, HistoryAction action, String ip) { | |||
| PostHistory postHistory = new PostHistory(); | |||
| postHistory.projectStepId = post.getProjectStep().getId(); | |||
| postHistory.parentPostId = postHistory.getParentPostId(); | |||
There was a problem hiding this comment.
코드 리뷰를 진행하겠습니다.
-
변경 사항 검토:
PostAction에서HistoryAction으로의 변경은 클래스의 목적에 맞는 좋은 수정으로 보입니다. 게시물의 이력을 처리하는 데 있어 적절한 열거형을 사용하는 것이 중요합니다.
-
주석 부분:
- 코드 주석 부분에서 불필요한 공백이나 구조가 잘못된 부분이 있으므로 주석을 더욱 명확하게 작성하는 것이 좋습니다. 예를 들어, 줄바꿈이나 내용을 정리하여 가독성을 높일 수 있습니다.
-
null 체크:
Post post와ip매개변수에 대한 null 체크 로직이 없으므로, 해당 값들이 null일 경우NullPointerException이 발생할 수 있습니다. 이를 방지하기 위해 입력 값에 대한 검증을 추가하는 것이 좋습니다.
-
DB 컬럼 관련:
registered_ip와modifier_ip의 길이에 대한 제약조건이 있는데, 이 값들에 대한 유효성 검사 로직이 필요할 수 있습니다. 특히 IP 형식을 검증하는 로직을 추가하는 것을 고려할 수 있습니다.
-
메서드 명명:
createPostHistory메서드는 잘 명명되었지만, 팩토리 메서드의 경우 가독성을 높이기 위해fromPost와 같이 명확한 형식을 사용할 수도 있습니다.
-
상수 사용 고려:
PostHistory클래스 내부에서 사용되는 문자열을 명시적으로 상수화하는 것이 좋습니다. 이는 코드의 유지보수성을 높이고 반복되는 문자열을 감소시킬 수 있습니다.
이 외에 도움이 필요하시거나 더 구체적인 부분이 궁금하시면 말씀해 주세요!
| @@ -64,7 +64,7 @@ public enum ErrorCode { | |||
| NOT_DELETE_PARENT_POST(2506, HttpStatus.BAD_REQUEST, "관련된 게시글이 존재하는 경우 해당 게시글을 삭제할 수 없습니다."), | |||
| NOT_DELETE_PARENT_COMMENT(2507, HttpStatus.BAD_REQUEST, "관련된 댓글이 존재하는 경우, 해당 댓글을 삭제할 수 없습니다."), | |||
| NOT_MATCH_PROJECTSTEPID(2508, HttpStatus.BAD_REQUEST, "게시글의 프로젝트 단계가 일치하지 않습니다."), | |||
| NOT_FOUND_COMMENT(2509, HttpStatus.NOT_FOUND, "해당 댓글(부모 댓글)을 찾을 수 없습니다."), | |||
| NOT_FOUND_COMMENT(2509, HttpStatus.NOT_FOUND, "해당 댓글을 찾을 수 없습니다."), | |||
| // 3000번대 코드 : DB 관련 | |||
| FILE_NOT_FOUND_ERROR(3000, HttpStatus.NOT_FOUND, "해당 파일을 찾을 수 없습니다."), | |||
| LINK_NOT_FOUND_ERROR(3001, HttpStatus.NOT_FOUND, "해당 링크를 찾을 수 없습니다."), | |||
There was a problem hiding this comment.
코드 패치에 대한 간단한 리뷰를 진행하겠습니다.
변경 사항
NOT_FOUND_COMMENT의 메시지가 "해당 댓글(부모 댓글)을 찾을 수 없습니다."에서 "해당 댓글을 찾을 수 없습니다."로 변경되었습니다.
리뷰 및 제안 사항
-
명료성:
- 이전 메시지에서는 '부모 댓글'이라는 용어가 포함되어 있었으나, 새로운 메시지에서는 그 정보가 사라져 있습니다. 만약 이 코드가 부모 댓글을 찾는 것이 명확히 요구되는 상황이라면, 이 변경이 혼란을 초래할 수 있습니다. 변경 후 메시지의 명확성을 확인하고, 필요한 경우 부모 댓글에 대한 언급을 유지하는 것이 좋습니다.
-
일관성:
- 다른 오류 메시지들과 비교할 때, '부모 댓글'의 언급이 사라지는 것이 일관성을 해칠 수 있습니다. 다른 오류 메시지들도 특별한 객체에 대한 정보를 포함하고 있으므로, 이 오류 메시지도 그에 맞춰 일관성을 유지할 필요가 있습니다.
-
향후 확장성:
- 이 에러 코드가 프로그램의 다른 부분에서 많이 사용될 가능성이 있다면, 에러 메시지를 좀 더 일반화하여 사용하는 것도 고려해볼 만합니다. 현재 메시지는 특정 상황에서만 사용될 수 있으므로 이를 염두에 두고 작성해야 합니다.
-
테스팅:
- 변경 사항이 동작에 영향이 없는지 확인하기 위해 관련 테스트 케이스를 점검하고, 필요한 경우 추가하는 것이 좋습니다.
결론
전체적으로 작은 수정이지만, 명확성과 일관성, 그리고 향후 유지보수성을 고려해 주의 깊게 검토하는 것이 필요합니다. 변경된 메시지가 논리적으로 적절한지 확인하고, 코드의 나머지 부분과 충분히 잘 어울리는지 확인하도록 하세요.
| @@ -43,7 +43,7 @@ public String getEmail(){ | |||
|
|
|||
| @Override | |||
| public String getUsername() { | |||
| return member.getUsername(); | |||
| return member.getName(); | |||
| } | |||
|
|
|||
| @Override | |||
There was a problem hiding this comment.
코드 패치에 대한 간단한 리뷰를 제공하겠습니다.
-
버그 위험:
getUsername()메서드가member.getUsername()에서member.getName()으로 변경되었습니다. 만약getName()메서드가 사용자 이름(Username)을 반환하지 않고, 다른 값을 반환한다면 예상치 못한 버그가 발생할 수 있습니다. 이 변경이 의도된 것인지,getName()메서드가 실제로 사용자 이름을 반환하는지 확인해야 합니다. -
명확성: 메서드 이름인
getUsername()가 반환하는 값이 사용자 이름이 아니라면, 메서드 이름을 변경하는 것이 좋습니다. 예를 들어getDisplayName()같은 이름으로 변경하면 더 명확한 의미를 전달할 수 있습니다. -
테스트 추가: 이 메서드에 대한 변경사항을 테스트하는 유닛 테스트가 있다면, 이를 업데이트하거나 새로운 테스트 케이스를 추가해 변경된 로직이 제대로 동작하는지 검증하는 것이 좋습니다.
-
문서화: 코드에 대한 주석이나 문서가 없으면 변경에 대한 이유를 이해하기 어려울 수 있습니다. 변경 사항에 대한 간단한 설명을 주석으로 추가하면 이후 유지보수에 도움이 될 것입니다.
변경 사항에 대한 추가 설명이나 코드의 다른 부분과의 연관성을 확인하는 것도 좋은 방법입니다.
| @@ -6,6 +6,8 @@ | |||
| import org.springframework.data.domain.Pageable; | |||
|
|
|||
| public interface PostReader { | |||
| void existPost(Long postId); | |||
|
|
|||
| Post getPost(Long postId); | |||
|
|
|||
| Page<Post> getPosts(Long projectStepId, String keyword, String repoFilter, Pageable pageable); | |||
There was a problem hiding this comment.
코드 패치를 검토한 결과는 다음과 같습니다:
-
메서드 이름:
existPost메서드는 존재 여부를 확인하는 것으로 보이지만, 메서드 이름이 그 목적을 완전히 반영하지 않는 것 같습니다. 일반적으로 "exists"라는 이름을 사용하는 것이 더 명확할 수 있습니다. 즉,existsPost(Long postId)로 변경하는 것이 좋습니다. -
반환 타입: 현재
existPost메서드는 반환 타입이 없습니다. 하지만 존재 여부를 확인하는 메서드라면 boolean 값을 반환하는 것이 더 적절할 것 같습니다. 예를 들어,boolean existsPost(Long postId)로 구현하면 더 직관적입니다. -
예외 처리:
postId가 유효하지 않거나 null일 경우에 대한 예외 처리가 필요할 수 있습니다. 이를 통해 예기치 않은 에러를 방지할 수 있습니다. 적절한 예외를 던지도록 메서드를 구현하는 것이 좋습니다. -
주석 및 문서화: 새로운 메서드에 대한 설명이 부족합니다. 코드의 가독성을 높이기 위해 javadoc과 같은 문서화를 추가하는 것이 좋습니다. 이 방법으로 다른 개발자들이 이 메서드의 목적과 사용 방식을 더 쉽게 이해할 수 있습니다.
-
테스트 케이스: 새로운 메서드를 추가하면서 테스트 케이스도 추가해야 합니다. 여러 시나리오를 테스트하여 예상대로 작동하는지 확인하는 것이 중요합니다.
이와 같은 점들을 고려하여 변경 및 개선하면 코드 품질이 향상될 것입니다.
| @@ -51,6 +58,7 @@ public Integer getRefOrder(Post parentPost) { | |||
| .orElseThrow(() -> new BusinessException(NOT_FOUND_POST)); | |||
| } | |||
|
|
|||
|
|
|||
| @Override | |||
| public ProjectStep getProjectStep(Long projectStepId) { | |||
| return projectStepRepository.findById(projectStepId) | |||
There was a problem hiding this comment.
코드 패치에 대한 간단한 리뷰를 진행하겠습니다.
-
버그 리스크:
existPost메서드에서postRepository가 정의되지 않은 것 같습니다. 해당 변수가 클래스 내에서 초기화되었는지 확인할 필요가 있습니다. 만약 그렇지 않다면,NullPointerException이 발생할 수 있습니다.
-
예외 처리:
BusinessException을 던질 때, 예외 메시지가 상수NOT_FOUND_POST로 지정되어 있습니다. 이 메시지가 사용자에게 충분한 정보를 제공하는지 확인해볼 필요가 있습니다. 필요 시, 사용자 친화적인 메시지로 변경하는 것도 고려해볼 수 있습니다.
-
메서드 명명:
existPost라는 메서드 이름은 조금 어색하게 느껴질 수 있습니다.postExists또는isPostExists와 같은 형태로 변경하면 더욱 명확하게 함수의 목적을 드러낼 수 있습니다.
-
문서화:
- 메서드에 대한 주석이 없다면 다른 개발자들이 이 메서드의 목적이나 사용 방법을 이해하기 어려울 수 있습니다. 각 메서드에 적절한 주석을 추가하는 것을 고려해보세요.
-
불필요한 공백:
- 코드 내에 불필요한 공백이 있습니다. 예를 들어
@Override앞 뒤로 공백이 추가되어 있는데, 이런 부분들은 코드 가독성에 악영향을 미칠 수 있으니 정리하는 것이 좋습니다.
- 코드 내에 불필요한 공백이 있습니다. 예를 들어
이러한 사항들을 고려하여 코드 품질을 향상시킬 수 있습니다. 추가적인 질문이 있으시면 말씀해 주세요!
| @@ -205,7 +206,7 @@ private Sort getSort(PostSort sortType) { | |||
| * 함수명 : savePostAndPostHistory() | |||
| * 함수 목적 : 게시글 및 게시글 이력 저장 메서드 | |||
| */ | |||
| private void savePostAndPostHistory(Post post, String ip, PostAction postAction) { | |||
| private void savePostAndPostHistory(Post post, String ip, HistoryAction postAction) { | |||
| postStore.storePost(post); | |||
| postStore.storePostHistory(post, postAction, ip); | |||
| } | |||
There was a problem hiding this comment.
코드 패치를 검토한 결과는 다음과 같습니다:
코드 리뷰 요약
-
상수 변경:
PostAction에서HistoryAction으로의 변경은 코드의 명확성을 높이고, 게시물 동작(Action)을 좀 더 직관적으로 나타내는 것 같습니다. 그에 따라 코드의 일관성이 향상됩니다.
-
댓글 목록 조회:
commentService.selectCommentList(post.getId())로 변경한 점은 좋습니다. 직관적이고 코드 가독성을 높입니다.
-
중복 코드 제거:
createPost메소드 내에서savePostAndPostHistory(post, registerIp, ...의 중복을 제거하고 항목을 통일한 점도 좋습니다.
-
댓글 삭제:
- 게시물 삭제 시 댓글을 일괄 삭제하는 로직을 추가한 점은 기능적인 개선이지만, 댓글을 삭제할 때 어떤 예외 처리가 필요한지 확인해야 합니다. 예를 들어, 댓글 삭제 과정에서 문제가 발생하면 전체 삭제 프로세스가 실패할 수 있습니다.
-
주석 정리:
- 주석을 정리하고 명확하게 작성한 부분이 인상적입니다. 이는 코드 가독성을 높이는 데 기여합니다.
버그 위험 요소
-
댓글 일괄 삭제:
commentService.deleteAllComments(post, deleterIp);에서, 댓글 삭제 과정에서 특정 조건을 확인하거나 롤백 처리가 이루어지지 않는데, 이로 인해 데이터 무결성이 손상될 수 있습니다. 적절한 예외 처리를 추가하는 것이 좋습니다.
-
IP 주소 변수 사용:
deleterIp변수가 어디서 정의되는지가 불명확합니다. 만약 이 변수가 정의되지 않거나 null인 경우, 댓글 삭제 과정에서 오류가 발생할 수 있습니다. 변수가 적절하게 초기화되었는지 확인할 필요가 있습니다.
-
트랜잭션 처리:
deletePost메소드는 여러 데이터베이스 연산을 수행하고 있습니다. 이 중 하나라도 실패할 경우, 성공적으로 완료된 작업과 분리되어 문제가 생길 수 있습니다. 트랜잭션을 활용하여 모든 작업이 성공적으로 수행되어야만 커밋되도록 하는 것이 좋습니다.
개선 제안
-
예외 핸들링 추가: 각 메소드에서 발생할 수 있는 예외에 대한 처리 로직을 추가하여 안정성을 높이는 것이 좋습니다.
-
단위 테스트 작성: 변경 사항에 대한 단위 테스트가 포함되어 있는지 확인하십시오. 특히 댓글 삭제 및 게시물 삭제 기능에 대한 테스트는 필수입니다.
-
정렬 기준 및 필터링 기능: 더 향상된 기능을 도입하기 위해,
getSort메소드에서 보다 정교한 정렬 기준이나 필터링 옵션을 추가해볼 수 있습니다.
위의 사항들을 참고하여 코드 품질을 개선하고, 버그를 예방하는 방향으로 진행하시면 좋겠습니다.
|
|
||
| public interface PostStore { | ||
| Post storePost(Post post); | ||
|
|
||
| PostHistory storePostHistory(Post post, PostAction postAction, String ip); | ||
| PostHistory storePostHistory(Post post, HistoryAction postAction, String ip); | ||
|
|
||
| void deletePost(Post post); | ||
| } |
There was a problem hiding this comment.
코드 패치를 검토해본 결과, 다음과 같은 사항들을 지적하고 개선할 점을 제안합니다.
-
명명 일관성:
PostAction이라는 기존 상수 이름을HistoryAction으로 변경한 것처럼 보이는데, 이 변경이 기존 코드의 일관성에 어떤 영향을 미치는지 확인해야 합니다. 인터페이스를 사용하는 다른 클래스에서 이 변경 사항이 영향을 줄 수 있으므로, 사용되는 모든 곳에서 일관되게 수정되었는지 확인해야 합니다. -
버그 리스크:
storePostHistory메소드에서PostAction을HistoryAction으로 변경한 것의 의미를 명확히 이해하고 있어야 합니다. 만약PostAction이 이전에 사용되던 중요 메소드라면, 이 변경으로 인한 기능 손실이 발생할 가능성이 있습니다. -
문서화:
storePostHistory메소드의 파라미터 변경 및 의미에 대한 문서화가 부족합니다. 메소드에 대한 주석을 추가하여 변경된 파라미터가 무엇을 의미하는지, 왜 변경되었는지를 명확히 하면 다른 개발자가 이해하는 데 도움이 됩니다. -
유효성 검사:
storePostHistory메소드의 파라미터로Post및String ip를 받는데, 이 값들이 null일 경우에 대한 처리 로직이 필요할 수 있습니다. 입력값에 대한 유효성을 검사하여 예외를 처리하는 방법을 고려해야 합니다.
이 사항들을 검토하고 수정하여 코드의 안전성과 가독성을 높이는 방향으로 진행하면 좋을 것 같습니다.
| @@ -20,7 +20,7 @@ public Post storePost(Post post) { | |||
| } | |||
|
|
|||
| @Override | |||
| public PostHistory storePostHistory(Post post, PostAction postAction, String ip) { | |||
| public PostHistory storePostHistory(Post post, HistoryAction postAction, String ip) { | |||
| return postHistoryRepository.save(PostHistory.createPostHistory(post, postAction, ip)); | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
코드 패치에 대한 간단한 코드 리뷰를 제공하겠습니다.
코드 리뷰
-
변수 이름 변경:
PostAction을HistoryAction으로 변경한 것은 기존의 의미를 정교하게 만들 수 있습니다. 다만, 코드의 일관성과 가독성을 위해 더 구체적인 이름 사용을 고려할 수 있습니다. 두 종류의 액션을 명확히 구분할 수 있다면, 문맥에 맞는 명칭이 좋습니다. -
의존성 주입:
@RequiredArgsConstructor를 사용하고 있지만, 클래스 생성자에서postHistoryRepository가 어떤 방식으로 주입되는지 확인이 필요합니다.requiredArgsConstructor를 올바르게 사용하기 위해 어노테이션을 상단에 명시해야 합니다. -
예외 처리:
postHistoryRepository.save()메서드는 데이터베이스 관련 작업이므로, 예외가 발생할 가능성이 있습니다. 예외를 처리할 수 있는 로직을 추가하여 안정성을 높이는 것이 좋습니다. 예를 들어, try-catch 블록을 사용하여 데이터 저장 과정에서 발생할 수 있는 예외를 처리하는 것이 좋습니다. -
메서드 문서화:
storePostHistory메서드에 대한 JavaDoc을 추가하는 것이 좋습니다. 이 메서드의 목적, 매개변수, 반환값 및 예외에 대한 설명을 포함하면, 코드 유지보수 시 도움이 됩니다. -
입력값 검증:
storePostHistory메서드에서post,postAction,ip와 같은 입력값에 대한 유효성 검사를 추가할 필요가 있습니다. null 체크 및 특정 규칙을 확인하여 잘못된 데이터가 저장되지 않도록 해야 합니다.
이러한 점들을 고려하여 성능 및 안정성을 개선할 수 있습니다.
| List<Comment> comments = commentRepository.getCommentList(postId); | ||
| List<GetCommentResponse> responses = new ArrayList<>(); | ||
| comments.stream() | ||
| .map(comment -> |
There was a problem hiding this comment.
이거 stream 빼고 forEach가 맞지 않나요..? comments가 쓰이지 않아서 여쭤봅니다..!
|
|
||
| // 작성자 일치 여부 확인 | ||
| matchCommentWriter(comment.getCreatedBy(), deleterId); |
There was a problem hiding this comment.
댓글 작성자만 삭제할 수 있는거 같은데 관리자나 원글 작성자가 삭제할 수 있게 해야하지 않을까요?
| } | ||
| // todo : 이력 추가 |
|
|
||
| List<GetCommentResponse> getIsActiveComments(Long postId); | ||
|
|
||
| Comment getComment(Long commentId); |
There was a problem hiding this comment.
코드 패치에 대한 간단한 리뷰입니다.
-
코드 변경 사항:
Member클래스를 import하고getMember(Long userId)메서드를 추가하였습니다.
-
버그 위험:
getMember(Long userId)메서드가userId를 받지만, 이를 어떻게 처리할지에 대한 로직이 없으므로, 사용자 ID가 잘못되었거나 존재하지 않는 경우에 대한 처리가 필요합니다. 실패 시null을 리턴하거나 예외를 던져야 할지 고민할 필요가 있습니다.
-
개선 제안:
- 메서드 이름이
getMember인데, 사용자 정보를 가져오는 것이라면findMemberById와 같은 이름으로 변경하여 더 명확하게 표현할 수 있습니다. getMaxRef()나getComment(Long commentId)와 같은 메서드들이 어떤 경우에 호출되는지 주석을 추가하면 이해에 도움이 될 것입니다.- 반환 자료형으로
Member객체가 문제가 될 수도 있으니,Optional<Member>를 반환하도록 수정하면 더 안전한 디자인이 될 수 있습니다. 이는 클라이언트 코드에서null체크를 수월하게 할 수 있게 해줍니다.
- 메서드 이름이
이러한 점들을 고려하여 코드를 수정하면 더 안정적이고 유지보수하기 좋은 코드를 만들 수 있을 것입니다.
| .map(GetCommentResponse::toDto) | ||
| .toList(); | ||
| public List<GetCommentResponse> selectCommentList(Long postId){ | ||
| return commentReader.getIsActiveComments(postId); | ||
| } |
There was a problem hiding this comment.
코드 패치에 대해 간단히 검토한 결과는 다음과 같습니다.
개선 사항 및 버그 위험
-
역할 확인 로직:
checkCommentEditPermission메서드에서는 작성자와 관리자를 비교하는 로직이 추가되었습니다. 하지만commentReader.getMember(modifierId)가null을 반환할 경우NullPointerException이 발생할 위험이 있습니다. 이를 방지하기 위해getMember호출 후,null체크를 추가하는 것이 좋습니다.Member member = commentReader.getMember(modifierId); if (member == null || !member.getRole().equals(Role.ADMIN)) { throw new BusinessException(NOT_MATCH_WRITER); }
-
상수 사용:
NOT_MATCH_WRITER와 같은 상수는 코드의 가독성과 유지 관리에 큰 도움이 됩니다. 그러나 이 상수가 어디서 정의되었는지 패치에서 확인할 수 없습니다. 만약 해당 상수가 적절한 위치에 정의되어 있지 않다면, 가독성을 위해 상수들을 정리하고 문서화할 필요가 있습니다. -
주석 및 문서화:
// todo : 이력 추가와 같은 주석은 개발자가 해야 할 일을 명확히 하긴 하지만, 패치에서 이 부분을 수정하지 않고 남겨두는 것은 다소 불필요할 수 있습니다. 이력 기능이 필요하다면 명확히 구현하고 주석을 제거하는 것이 좋습니다. -
전반적인 예외 처리:
BusinessException이 발생하는 경우 사용자에게 적절한 오류 메시지를 반환할 수 있도록 처리해야 합니다. 이를 위해 예외 처리 로직을 추가하여 API 사용자에게 명확한 이유를 전달하도록 고려해야 합니다. -
유닛 테스트: 이 변경사항이 다른 부분에 미치는 영향을 최소화하기 위해 유닛 테스트를 작성하는 것이 중요합니다. 특히 역할 비슷한 비교 로직이 포함된 경우, 다양한 시나리오에 대한 테스트를 수행해야 합니다.
결론
전반적으로 코드 변경은 이해하기 쉽게 잘 구성되어 있지만, 예외 처리를 추가하고, null 체크를 통해 안전성을 높이며, 주석을 개선하는 방향으로 변경하면 좋겠습니다. 이러한 점들을 고려하여 코드를 수정하면 더욱 안정적이고 가독성이 높은 코드가 될 것입니다.
| @@ -59,12 +59,12 @@ public enum ErrorCode { | |||
| NOT_FOUND_PROJECT_STEP(2501, HttpStatus.NOT_FOUND, "해당 프로젝트 단계를 찾을 수 없습니다."), | |||
| NOT_FOUND_MEMBER(2502, HttpStatus.NOT_FOUND, "해당 회원을 찾을 수 없습니다."), | |||
| NOT_FOUND_POST(2503, HttpStatus.NOT_FOUND, "해당 게시물을 찾을 수 없습니다."), | |||
| NOT_MATCH_WRITER(2504, HttpStatus.FORBIDDEN, "작성자만 수정 및 삭제할 수 있습니다."), | |||
| NOT_HAVE_EDIT_PERMISSION(2504, HttpStatus.FORBIDDEN, "수정 및 삭제 권한이 없습니다."), | |||
| NOT_FOUND_WRITER(2505, HttpStatus.NOT_FOUND, "작성자 정보를 찾을 수 없습니다."), | |||
| NOT_DELETE_PARENT_POST(2506, HttpStatus.BAD_REQUEST, "관련된 게시글이 존재하는 경우 해당 게시글을 삭제할 수 없습니다."), | |||
| NOT_DELETE_PARENT_COMMENT(2507, HttpStatus.BAD_REQUEST, "관련된 댓글이 존재하는 경우, 해당 댓글을 삭제할 수 없습니다."), | |||
There was a problem hiding this comment.
코드 패치에 대한 간단한 리뷰를 진행하겠습니다.
-
변수명 변경:
NOT_MATCH_WRITER를NOT_HAVE_EDIT_PERMISSION으로 변경한 것은 명확성과 가독성을 높이는 긍정적인 변화입니다. 수정 및 삭제 권한이 없음을 더 명확하게 표현하고 있습니다.
-
상태 코드:
HttpStatus.FORBIDDEN을 사용하는 것은 적절합니다. 사용자가 권한이 없을 때의 상황을 잘 나타내고 있습니다.
-
메시지 언어:
- 오류 메시지가 사용자 친화적인 한국어로 잘 작성되어 있습니다. 하지만 사용자의 이해를 돕기 위해, 메시지에 좀 더 구체적인 상황을 추가할 수 있습니다. 예를 들어, "작성자만 수정 및 삭제할 수 있습니다."라는 문구는 "로그인한 사용자가 작성자가 아닙니다." 같은 형태로 사용자에게 더 구체적인 피드백을 줄 수 있습니다.
-
정보 추가:
- 추가적으로, 사용자에게 필요한 경우 수정이나 삭제 권한을 어떻게 요청할 수 있는지에 대한 정보를 포함하는 것도 좋습니다. 예를 들어, 오류 메시지 안에 "권한 요청은 관리자에게 문의하십시오." 같은 도움말을 추가할 수 있습니다.
-
검증 및 테스트:
- 코드를 변경한 후, 이 부분에 대한 충분한 테스트를 수행해야 합니다. 특히 이 에러 코드가 사용되는 모든 흐름에서 제대로 작동하는지를 확인하는 것이 중요합니다.
결론적으로, 코드 변경은 전반적으로 긍정적이며 문구의 명확성을 높였습니다. 추가적인 사용자 피드백을 제공하는 방안을 고려하면 더욱 좋을 것 같습니다.
| import com.seveneleven.entity.board.Comment; | ||
| import com.seveneleven.entity.board.Post; | ||
| import com.seveneleven.entity.member.Member; | ||
| import com.seveneleven.entity.board.constant.HistoryAction; | ||
| import com.seveneleven.entity.member.constant.Role; | ||
| import com.seveneleven.exception.BusinessException; |
There was a problem hiding this comment.
코드 리뷰를 진행하겠습니다.
-
조건문 변경:
- 기존의 조건문에서는
!commentCreatedBy.equals(modifierId)또는!commentReader.getMember(modifierId).getRole().equals(Role.ADMIN)중 하나라도 참이면 예외를 발생시켰습니다. - 수정된 코드에서는 두 조건이 모두 거짓일 때만 예외를 발생시키고 있습니다. 이는 의도한 로직인지 확인이 필요합니다. 만약 작성자 또는 관리자 둘 중 하나라도 맞으면 예외가 발생하지 않도록 하려는 것이었다면, 변경된 코드가 맞습니다. 하지만 원래의 코드에서 의도한 바가 다르게 해석될 수 있으니 잘 검토해야 합니다.
- 기존의 조건문에서는
-
예외 메시지 변경:
- 예외 메시지가
NOT_MATCH_WRITER에서NOT_HAVE_EDIT_PERMISSION으로 변경되었습니다. 새로운 메시지가 더 직관적으로 보이지만, 이로 인해 기존 클라이언트 코드에서 예외 처리 로직이 변경될 수 있으므로 주의가 필요합니다.
- 예외 메시지가
-
null 체크:
commentCreatedBy와modifierId가 null일 경우NullPointerException을 발생시킬 수 있습니다. 두 변수를 비교하기 전에 null 체크를 추가하는 것이 좋습니다.
-
로깅 추가:
- 예외를 발생시키기 전에 로깅을 추가하여 누가 어떤 권한으로 접근했는지 기록하면, 문제 발생 시 디버깅에 도움이 될 수 있습니다.
-
주석 보강:
- 메서드와 조건문의 목적을 명확하게 하기 위해 주석을 추가하거나 개선하면 코드 가독성이 높아질 것입니다.
-
코드 스타일:
- 파일의 끝에 새 줄이 없다는 경고가 나타나고 있습니다. 코드 스타일 가이드를 따르기 위해 파일 끝에 새 줄을 추가하는 것이 좋습니다.
이와 같은 요소들을 고려해서 코드를 검토하시기 바랍니다. 전반적으로 변경된 로직이 의도한 바와 일치하는지 확인하는 것이 가장 중요합니다.
| @@ -1,7 +1,7 @@ | |||
| package com.seveneleven.board.service; | |||
|
|
|||
| import com.seveneleven.board.dto.*; | |||
| import com.seveneleven.entity.board.constant.PostAction; | |||
| import com.seveneleven.entity.board.constant.HistoryAction; | |||
| import com.seveneleven.entity.board.Post; | |||
| import com.seveneleven.entity.board.constant.PostFilter; | |||
| import com.seveneleven.entity.board.constant.PostSort; | |||
There was a problem hiding this comment.
코드 패치에 대해 간단한 리뷰를 하겠습니다.
-
예외 메시지 변경:
NOT_MATCH_WRITER에서NOT_HAVE_EDIT_PERMISSION으로 예외 메시지를 변경한 것은 기능적인 면에서 적절해 보입니다. 작성자와 수정자가 일치하지 않는 경우에 대한 보다 명확한 설명을 제공하는 것이 좋습니다. 하지만 예외 메시지의 변화를 충분히 문서화하여 다른 개발자들이 이해할 수 있도록 해야 합니다. -
Null 체크:
createdBy와modifierId는Long타입으로 정의되어 있습니다. null 값이 들어올 경우equals메서드를 호출할 때NullPointerException이 발생할 수 있습니다. 따라서 null 체크를 추가하는 것이 좋습니다.if (createdBy == null || modifierId == null || !createdBy.equals(modifierId)) { throw new BusinessException(NOT_HAVE_EDIT_PERMISSION); }
-
주석 추가: 이 메서드는 사용자가 수정 권한이 있는지를 판단하는 역할을 하고 있습니다. 그러므로 이를 명확히 설명하는 주석이 있으면 코드 가독성 및 유지보수성이 향상될 것입니다.
-
로깅: 예외가 발생할 때 그에 대한 로깅을 추가하면, 문제 발생 시 디버깅에 도움이 될 수 있습니다. 예외를 던지기 전에 로그를 남기는 것도 고려해 보세요.
이러한 점들을 고려하면 코드의 안정성과 가독성을 높일 수 있습니다.
| List<GetCommentResponse> responses = new ArrayList<>(); | ||
| comments.stream() | ||
| .map(comment -> | ||
| { | ||
| if(comment.getModifierIp() == null) { | ||
| responses.add(GetCommentResponse.toDto(comment, YesNo.NO)); | ||
| } else { | ||
| responses.add(GetCommentResponse.toDto(comment, YesNo.YES)); | ||
| } | ||
| return null; | ||
| } | ||
| ) | ||
| .toList(); | ||
| return responses; |
There was a problem hiding this comment.
return comments.stream()
.map(comment ->
{
if(comment.getModifierIp() == null) {
return GetCommentResponse.toDto(comment, YesNo.NO);
} else {
return GetCommentResponse.toDto(comment, YesNo.YES);
}
}
)
.toList();
| @@ -40,7 +40,7 @@ public ResponseEntity<APIResponse<SuccessCode>> updateComment(@AuthenticationPri | |||
| @RequestBody PatchCommentRequest patchCommentRequest, | |||
| HttpServletRequest request | |||
| ) { | |||
| commentService.updateComment(postId, commentId, patchCommentRequest, request, user.getMember().getId()); | |||
| commentService.updateComment(postId, commentId, patchCommentRequest, request, user); | |||
| return ResponseEntity.status(SuccessCode.UPDATED.getStatus()) | |||
| .body(APIResponse.success(SuccessCode.UPDATED)); | |||
| } | |||
There was a problem hiding this comment.
코드 패치에 대한 간단한 리뷰를 제공하겠습니다.
-
변경 사항:
user.getMember().getId()를 전달하던 부분이user객체를 직접 보내도록 변경되었습니다. -
장점:
user객체를 직접 전달함으로써 코드가 보다 명확해졌습니다. 호출하는 메서드의 정의가 간소화되어user에 대한 여러 정보를 사용할 수 있는 유연성이 생깁니다.- 나중에
user객체에서 추가 정보를 필요로 할 경우, 바로 접근이 가능해지는 장점이 있습니다.
-
버그 리스크:
commentService.updateComment()및commentService.deleteComment()메서드에서user객체를 사용하여 비즈니스 로직을 수행하는 경우, 해당 메서드들이user의 적절한 상태를 검사하고 있는지 확인해야 합니다. 만약user가 null이거나 유효하지 않은 상태라면 예외가 발생할 수 있습니다.
-
개선 사항:
- 메서드 인자에 대한 주석을 추가하여
user객체가 포함된 이유와 사용 방법을 설명하는 것도 좋습니다. 이는 코드 가독성을 높여줍니다. - 만약
user객체의 검증이 아직 구현되지 않았다면, 코드 내에서 null 체크를 추가하거나, 유효성 검사를 할 수 있도록 검토해야 합니다.
- 메서드 인자에 대한 주석을 추가하여
-
코드 스타일:
- 전반적으로 코드 스타일은 깔끔하며, 일관된 형식을 유지하고 있습니다. 하지만, 추가적인 로깅이나 예외 처리를 고려할 수 있습니다. 예를 들어, 비즈니스 로직 실행 시 오류 발생 시 적절한 로그 메시지를 기록하는 것이 유용할 수 있습니다.
이상의 사항을 고려하여 코드 리뷰를 진행하시기 바랍니다!
| @RequiredArgsConstructor | ||
| public class CommentReaderImpl implements CommentReader { | ||
| private final CommentRepository commentRepository; | ||
| private final MemberRepository memberRepository; |
There was a problem hiding this comment.
코드 패치를 살펴보면 몇 가지 개선사항과 잠재적인 버그 위험 요소가 있습니다.
개선사항 및 버그 위험
-
불필요한 반환 값 제거:
- 기존 코드에서는
responses리스트를 사용했으나, 이제는 Stream API를 사용하므로 리스트를 따로 만들 필요가 없습니다. 그러나map함수 내부에서 두 개의return문이 있기 때문에, 코드가 더 간결해질 수 있습니다. null을 반환하는 코드가 남아있지 않았는데,map에서 반환하는 값을 잘 처리하고 있음을 확인했습니다.
- 기존 코드에서는
-
단순화된 논리:
if-else문을 사용하여YesNo값에 따라GetCommentResponse를 반환하고 있습니다. 이를 삼항 연산자로 간소화할 수 있습니다. 예를 들어, 다음과 같이 쓸 수 있습니다.return comments.stream() .map(comment -> GetCommentResponse.toDto(comment, comment.getModifierIp() == null ? YesNo.NO : YesNo.YES)) .toList();
-
메서드 명확성:
getIsActiveComments라는 메서드 이름이 "활성 댓글"이라는 의미를 가지고 있지만, 어떤 조건에 의해 이를 판단하는지 주석이나 문서화가 필요할 수 있습니다. 다른 개발자가 코드를 이해하는 데 도움이 될 것입니다.
-
Null Safety:
comment.getModifierIp()가null인지 체크하는 부분은 괜찮지만,comments리스트 자체가null일 가능성도 고려해야 합니다. 이를 방지하기 위해Optional을 사용할 수도 있습니다.
-
성능 고려사항:
- 현재
toList()메서드를 호출하여 결과를 리스트로 변환하고 있지만, 이 과정이 필요 없는 경우 성능에 영향을 줄 수 있습니다. 예를 들어, 결과를 직접적으로 다른 곳에 사용한다면 리스트로 만드는 과정을 생략할 수 있습니다.
- 현재
결론
전반적으로 코드는 잘 작성되었고, Stream API를 활용하여 가독성을 높인 점은 긍정적입니다. 하지만 위에서 언급한 개선사항을 고려하여 코드를 더 깔끔하고 안전하게 만들 수 있습니다.
|
|
||
| @Transactional | ||
| void updateComment(Long postId, Long commentId, PatchCommentRequest patchCommentRequest, HttpServletRequest request, Long modifierId); |
There was a problem hiding this comment.
코드 패치에 대한 간단한 코드 리뷰를 진행하겠습니다.
-
입력 매개변수 변경:
updateComment와deleteComment메서드의 매개변수를Long유형의 ID에서CustomUserDetails로 변경했습니다. 이는 사용자 정보를 객체로 전달하여, 더 많은 사용자 정보를 쉽게 관리할 수 있게 하는 접근법으로 보입니다. 그러나 기존의 ID를 사용하는 방식과 혼합할 경우, 사용자 관련 로직이 비즈니스 로직과 혼잡해질 수 있습니다.CustomUserDetails를 어떻게 사용하는지 명확히 할 필요가 있습니다. -
트랜잭션 관리: 기존의
@Transactional어노테이션이updateComment와deleteComment메서드에서 제거되었습니다. 이는 트랜잭션 관리를 놓칠 수 있는 잠재적인 위험이 있으므로, 트랜잭션 처리가 필요한 경우 다시 추가하는 것이 좋습니다. 비즈니스 로직에서 데이터 무결성을 보장하기 위해 필수적입니다. -
메서드 간결성:
updateComment및deleteComment메서드가 두 개의 긴 매개변수를 받습니다. 매개변수를 좀 더 간결하게 유지하고, 객체로 묶어 전달하는 방법도 검토해볼 만합니다. 예를 들어, 요청 객체를 하나로 묶거나 DTO를 사용하는 방식을 고려할 수 있습니다. -
코드 일관성: 메서드 시그니처의 일관성을 유지하는 것이 중요합니다. 만약
createComment가HttpServletRequest를 유지하고 있다면,updateComment및deleteComment의 매개변수도 일관되게 유지하는 것이 좋을 수 있습니다. -
예외 처리: 코드에 예외 처리에 대한 내용이 보이지 않는데, 에러 발생 시 예외를 어떻게 처리할지도 검토해야 합니다. 예를 들어, 사용자가 존재하지 않거나 수정 권한이 없는 경우에 대한 처리가 필요할 수 있습니다.
이런 사항들을 고려하여 패치를 진행하시면, 코드의 안정성과 가독성을 높일 수 있을 것입니다.
| public List<GetCommentResponse> selectCommentList(Post post){ | ||
| return commentRepository.getCommentList(post.getId()) | ||
| .stream() | ||
| .map(GetCommentResponse::toDto) |
There was a problem hiding this comment.
코드 패치를 리뷰해 드리겠습니다. 아래는 코드에서 발견된 버그 위험 및 개선 사항입니다.
버그 위험
-
NullPointerException 위험:
customUserDetails가null일 경우user.getMember()호출 시NullPointerException이 발생할 수 있습니다. 메서드의 파라미터로 들어오는 객체가 유효한지 체크하는 로직이 필요합니다.
-
Role 체크 로직:
- 현재 관리자의 역할을 비교하는 로직에서
Role.ADMIN.equals(user.getMember().getRole())를 사용하고 있습니다. 만약user.getMember()가null인 경우, 같은 문제로 이어질 수 있습니다.
- 현재 관리자의 역할을 비교하는 로직에서
개선 사항
-
코드 가독성 개선:
checkCommentEditPermission메서드 내의 조건문을 메서드로 분리하여 가독성을 높일 수 있습니다. 예를 들어, 작성자와 관리자의 조건을 각각의 메서드로 나누어 관리하는 것이 좋습니다.
-
예외 메시지 개선:
BusinessException의 메시지를 좀 더 명확하게 하여, 어떤 경우에 예외가 발생했는지 쉽게 알 수 있게 하면 좋습니다. 예를 들어, "댓글 편집 권한이 없습니다."와 같이 더 구체적으로 작성할 수 있습니다.
-
기타 유효성 검사 추가:
postId,commentId, 및request가 유효한지 검증하는 로직을 추가하는 것이 좋습니다. 이로 인해 코드의 안정성을 향상시킬 수 있습니다.
결론
전체적으로 잘 작성된 코드이며, 구조도 명확합니다. 그러나 NullPointerException과 같은 예외 처리, 코드 가독성 개선 및 추가적인 유효성 검증을 통해 좀 더 안전하고 유지보수하기 쉬운 코드를 만들 수 있을 것입니다. 코드 품질을 높이기 위한 노력은 언제나 중요합니다.
바로 MERGE하지 마세요! 검토 후 MERGE해야합니다!
✅ 최근 작업 주제 (하나 이상의 주제를 선택해주세요.)
🏆 구현 목표
댓글 리팩토링
📋 구현 사항 설명 (작업한 내용을 상세하게 기록합니다.)
🔍 테스트 방법 (변경 사항을 확인하기 위한 테스트 방법을 기술합니다.)