Test(#24): 테스트 코드 작성, 댓글 목록 조회 전체 데이터 반환 구현 #46
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -57,7 +59,7 @@ public void setValues(PreparedStatement ps, int i) throws SQLException { | |||
| ps.setString(2, post.getContent()); | |||
| ps.setLong(3, post.getUser().getId()); | |||
| ps.setBoolean(4, false); | |||
| ps.setTimestamp(5, Timestamp.valueOf(LocalDateTime.now())); | |||
| ps.setTimestamp(5, Timestamp.valueOf(LocalDateTime.now().minusHours(random.nextInt(48)))); | |||
| } | |||
|
|
|||
| @Override | |||
There was a problem hiding this comment.
지금 코드 리뷰를 진행하겠습니다:
-
Random클래스에 대한 객체random을 사용하여 랜덤 날짜 값을 생성하는 것은 프로그램의 예측 가능성을 줄이고 테스트가 어렵게 만들 수 있습니다. 다시 실행할 때 매번 결과가 달라질 수 있어서 추적 및 디버깅이 어려워집니다. -
현재 코드에서는
post를 처리하는 루프에서 랜덤 값을 사용하므로 데이터베이스에 저장되는 날짜 값이 테스트마다 다를 수 있습니다. 이는 테스트의 일관성을 해칠 수 있습니다. -
ps.setBoolean(4, false);코드의 경우is_done필드에 고정값으로false를 설정하고 있습니다. 이러한 경우 해당 필드는 의미 있는 상황에서만 이용해야 합니다. 그렇지 않으면 이 필드 자체를 제거하는 것이 나을 수도 있습니다. -
더 나은 가독성과 유지보수성을 위해 SQL 쿼리를 문자열로 하드 코딩하는 대신에,
NamedParameterJdbcTemplate를 사용하여 named parameter를 활용하는 것이 좋을 수 있습니다. -
LocalDateTime.now().minusHours(random.nextInt(48))부분은 현재 시간에서 최대 48시간 전의 랜덤 값을 생성하는데, 이는 기능적인 목적에 따라 다를 수 있습니다. 그럼에도 불구하고 이러한 동작이 필요하다면 해당 주석을 추가하여 이유를 명확히 남기는 것이 좋습니다. -
코드에 주석이 거의 없으며, 메소드와 변수 이름이 충분히 설명적이지 않습니다. 좀 더 명확하게 목적을 설명하는 이름을 사용하는 것이 좋을 것입니다.
-
작성된 코드와 함께 단위 테스트가 포함되어 있다면 더욱 안정적인 코드가 될 수 있습니다. 테스트 코드의 작성을 고려해 보세요.
-
보안 측면에서 SQL Injection을 방지하기 위해 Prepared Statement나 Parameterized Query를 사용하는 것이 좋습니다.
이러한 수정 사항을 고려하여 코드를 개선할 수 있을 것입니다.
| Post post = Post.from(createPostDto, users.get(randomUserId), null); | ||
| RequestPostDto requestPostDto = new RequestPostDto("title" + random.nextInt(99999), "content" + random.nextInt(99999)); | ||
| Post post = Post.from(requestPostDto, users.get(randomUserId), null); | ||
|
|
||
| posts.add(post); | ||
| } | ||
| bulkRepository.savePosts(posts); |
There was a problem hiding this comment.
이 코드 패치를 살펴보겠습니다.
-
CreatePostDto가RequestPostDto로 이름이 변경되었습니다. 이 유형 변경은 영향을 미칠 수 있으므로 API에서도 같이 업데이트해야 합니다. -
bulkPosts()메소드 내에서 1000번의 반복문을 실행하며 무작위 게시물을 생성하고 저장합니다. 이로 인해 대량의 데이터가 생성될 수 있으므로 성능과 테스트 가능성에 영향을 미칠 수 있습니다. 이를 개선하기 위해 실제 필요한 양에 맞게 조정하는 것이 좋습니다. -
Post.from()메소드는 별도의 클래스 메서드인 듯 하지만, 해당 구현이 어떻게 이루어졌는지 살펴볼 필요가 있습니다. 이 부분이 안정적이고 기능적인지 확인이 필요합니다. -
랜덤으로 사용자 ID를 만드는 부분에서
users.size()와randomUserId간의 일관성이 있는지, 그리고users목록이 비어 있는지 확인이 필요합니다. -
마지막으로
bulkRepository.savePosts(posts);와 같이 대량 저장 작업을 수행하는 경우, 에러 처리 및 롤백 메커니즘도 함께 고려해야 합니다.
개선 제안:
- 데이터베이스 호출 수를 최소화하고 성능을 향상시키기 위해 배치 삽입을 사용할 수 있습니다.
- 코드를 단위 테스트하고 예외 처리를 추가하여 안정성을 높일 수 있습니다.
- 주석을 추가하여 코드의 의도를 명확히 할 수 있습니다.
| @SwaggerOK(summary = "댓글 전체 목록 조회") | ||
| public ResponseEntity<List<CommentDto>> readComments(@PathVariable("postId") final Long postId) { | ||
| final List<CommentDto> commentDtos = commentService.readCommentsByPostId(postId); | ||
| return ResponseEntity.ok(commentDtos); | ||
| } | ||
|
|
||
| @PatchMapping |
There was a problem hiding this comment.
패치된 코드를 확인한 결과, 몇 가지 사소한 개선 포인트와 잠재적인 버그 위험 요소가 있습니다:
-
개선 제안:
readComments메서드에서는 댓글 전체 목록을 반환하므로, 메서드 이름을 더 명확하게 지어주는 것이 좋습니다. 예를 들어,getAllCommentsByPostId등으로 변경할 수 있습니다.- 커밋 메시지에 변화한 로직에 대한 간결한 설명을 추가하는 것이 좋습니다.
-
버그 위험 요소:
- 주석(
//)을 활용하여 메서드나 특정 코드 조각의 목적을 간단히 설명할 수 있습니다. 이렇게 하면 코드의 가독성을 향상시킬 수 있습니다.
- 주석(
-
추가 개선점:
- Swagger 문서에 있는 API 엔드포인트들을 실제 사용 상황과 일치하도록 수정하는 것이 좋습니다.
- 데이터 유효성 검사 부분에서 세부적인 실패 시 처리 로직 또는 오류 메시지를 추가함으로써 사용자에게 더 친절한 경험을 제공할 수 있습니다.
이러한 조언을 따라 코드를 개선하면 보다 견고하고 사용하기 쉬운 소프트웨어를 만들 수 있을 것입니다.
| @RequestPart(required = false) final MultipartFile thumbnail) { | ||
| postService.updatePost(postId, updatePostDto, thumbnail); | ||
| postService.updatePost(postId, requestPostDto, thumbnail); | ||
| } | ||
|
|
||
| @DeleteMapping("{postId}") |
There was a problem hiding this comment.
-
RequestPostDto클래스가CreatePostDto와UpdatePostDto를 대체하도록 변경되었습니다. 코드 전체에서 일관성을 유지하기 위해RequestPostDto클래스명 변경이 일관성 있는 선택입니다. -
createPost및updatePost메서드에서UpdatePostDto를 사용하는 대신 동일한RequestPostDto를 사용하여 더 일관성 있게 처리할 수 있습니다. -
createPost및updatePost메서드 내부에서 파라미터와 관련된 주석이 제공되면 이해하기 쉬울 것입니다. -
Swagger annotations과
@RequestParam등의 annotation 사이 빈 줄을 추가하여 가독성을 향상시킬 수 있습니다. -
예외 처리와 validation에 대한 추가적인 구현이 필요한지 확인하십시오.
-
코드 수정 후 테스트 케이스를 실행하여 모든 요청 및 응답이 기대한 대로 동작하는지 확인하는 것이 좋습니다.
| @@ -55,7 +54,7 @@ public void done() { | |||
| this.isDone = true; | |||
| } | |||
|
|
|||
| public void updatePost(final UpdatePostDto postDto, final String thumbnail) { | |||
| public void updatePost(final RequestPostDto postDto, final String thumbnail) { | |||
| this.title = postDto.title(); | |||
| this.content = postDto.content(); | |||
| this.thumbnail = thumbnail; | |||
There was a problem hiding this comment.
RequestPostDto와CreatePostDto,UpdatePostDto의 구분이 명확하지 않습니다. 적절한 이름을 선택하여 코드를 개선할 필요가 있습니다.updatePost메소드 내 매개변수명인postDto의 네이밍이 일관되지 않습니다. 다른 메소드와 통일성 있게 변경이 필요합니다.from메소드는 생성자 대신 static 팩토리 메서드로 변환될 수 있습니다.isDone불리언 대신 enum 혹은 상수를 사용하면 더 명시적일 수 있습니다.- JPA 설정에서 FetchType.LAZY보다 명시적인 fetch 전략 배치를 고려할 가치가 있습니다.
위 내용들을 고려하여, 코드 품질 및 유지 보수성을 향상해 볼 수 있습니다.
| private LocalDateTime updatedAt; | ||
| @LastModifiedDate | ||
| @Column(name = "updated_at") | ||
| private LocalDateTime updatedAt; | ||
| } |
There was a problem hiding this comment.
이 코드 패치의 리뷰를 진행하겠습니다.
버그 위험:
- 코드 중의 공백 문제: 들여쓰기가 규칙적이지 않음.
import선언에서 Star Import(*)가 사용되지 않았지만, 별도로 임포트되는 클래스들이 여전히 많습니다. 이것은 불필요한 인터페이스 의존을 낳을 수 있습니다.
개선 제안:
- Lombok 애노테이션을 좀 더 일관성 있게 정렬하여 가독성을 높일 수 있습니다.
LocalDateTime대신Instant나ZoneDateTime을 이용하여 보다 정확한 타임스탬프를 나타낼 수 있습니다.- Naming 컨벤션을 따라 변수명을 더 명확하고 통일성 있게 수정할 수 있습니다.
- 어노테이션에
@Override및 주석 처리된 부분들을 활용해 다른 개발자가 해당 클래스의 사용 방법과 역할을 더 쉽게 이해할 수 있도록 추가 설명을 덧붙일 수 있습니다.
이상입니다.
| @@ -12,4 +13,6 @@ public interface PostRepository extends JpaRepository<Post, Long> { | |||
| Optional<Post> findById(final Long id); | |||
|
|
|||
| Optional<Post> findByIdAndUserId(final Long postId, final Long userId); | |||
|
|
|||
| Optional<Post> findTopByOrderByIdAsc(); | |||
| } | |||
There was a problem hiding this comment.
이 코드 패치는 새로운 User 클래스를 추가하여 주요 변경 사항 중 하나입니다. 또한 findTopByOrderByIdAsc() 메서드를 추가하여 가장 작은 ID를 갖는 Post를 반환합니다.
개선 제안:
- 존재하는 메서드들의 JavaDoc 주석을 추가하면 좋습니다.
findTopByOrderByIdAsc()메서드가 명확하지 않을 수 있으므로, 해당 메서드를 호출하는 부분에서 사용 목적을 명시하거나 메서드 이름을 더 명확하게 수정하는 것이 좋을 수 있습니다.
버그 리스크:
- 새로 추가된
findTopByOrderByIdAsc()메서드의 동작을 테스트하여 예기치 않은 결과가 발생하지 않는지 확인해야 합니다. - 코드베이스 다른 부분과의 호환성 및 영향을 주의 깊게 검토하고 통합해야 합니다.
| final List<Comment> comments = commentRepository.findAllByPostId(postId); | ||
| return CommentDto.from(comments); | ||
| } | ||
|
|
||
| @Transactional | ||
| public void updateComment(final Long commentId, final String content) { | ||
| final Comment comment = commentRepository.findByIdAndUserId(commentId, securityUtil.getCustomUserId()) |
There was a problem hiding this comment.
코드 리뷰:
-
버그 위험:
updateComment메서드에서commentRepository.findByIdAndUserId반환 값을 확인하지 않으면NullPointerException이 발생할 수 있습니다.
-
개선 제안:
readAllCommentsByPostId및readCommentsByPostId메서드에 대한 예외 처리를 추가하는 것이 좋습니다.readCommentsByPostId메서드는findById메서드 대신findAllByPostId메서드를 사용함으로써 여러 개의 댓글을 반환하므로 의도를 명확히 할 수 있습니다.CommentDto.from(comments)가 List를 반환하는 것으로 보이므로 변수명을commentDtos등으로 변경하여 더욱 명확하게 할 수 있습니다.
-
추가적인 고려 사항:
- 입력 매개변수 유효성을 검증하는 부분이 누락되어 있을 수 있으니 이를 고려해야 합니다.
CommentDto클래스의 정교성이 코드에서 어떻게 다루는지에 대한 검토도 필요합니다.
| @@ -71,7 +74,7 @@ public PostPaginationDto readAllPosts(final PostPaginationParam paginationParam) | |||
| } | |||
|
|
|||
| @Transactional | |||
| public void updatePost(final Long postId, final UpdatePostDto postDto, final MultipartFile file) { | |||
| public void updatePost(final Long postId, final RequestPostDto postDto, final MultipartFile file) { | |||
| Post post = this.findById(postId); | |||
| String thumbnail = file != null ? s3Service.changeFile(post.getThumbnail(), file, S3path.THUMBNAIL) : post.getThumbnail(); | |||
| post.updatePost(postDto, thumbnail); | |||
There was a problem hiding this comment.
코드 리뷰 피드백:
-
kimandhong.oxox.dto.post.RequestPostDto대신RequestPostDto를 사용해야합니다. 임포트 선언 내의 국가 도메인 부분(kimandhong)을 제거할 것을 권장합니다. -
주석이 없는 생성자나 메서드 명시적인 목적 파라미터로 변경함으로써 가독성을 높일 수 있습니다.
-
createPost,updatePost메소드에서 예외가 캐치되었을 때, 어떤 예외가 던져지는지를 명시하는 것이 좋습니다. -
PostService의 메소드들은 인자로 받은 DTO를 Post 엔티티로 변환하는 작업을 하고 있는데, 이 로직을 별도의 클래스에서 처리하고 service 계층에 두면 역할 분리가 더 잘 이루어집니다.
-
try-catch블록 안에서 예외 처리 로직과 비즈니스 로직이 혼재되어 있습니다. 이를 분리하여 가독성과 유지보수성을 향상시키는 것이 좋습니다. -
코드에 대한 주석 처리를 추가하여 각 메소드와 기능에 대한 설명을 추가하면 이해하기 쉬워집니다.
-
PostPaginationDto 반환 값이 적절한 지 확인하고 필요 시 읽기 전용 DTO를 따로 만들어 사용하세요.
-
UpdatePostDto 대신 PostDto로 이름을 일치시켜 코드 일관성을 높입니다.
-
파일 처리 관련 로직에 대한 테스트 케이스 작성을 고려해보세요.
전반적으로 코드는 깔끔하고 보기 좋습니다. 개선 방법과 주의 사항을 위의 사항을 참고하여 살펴볼 것이 좋습니다.
|
|
||
| @BeforeEach | ||
| public void setUp() { | ||
| user = userRepository.findTopByOrderByIdAsc().orElseThrow(RuntimeException::new); | ||
| User user = userRepository.findTopByOrderByIdAsc().orElseThrow(RuntimeException::new); | ||
| token = "Bearer " + jwtUtil.createAccessToken(user); | ||
| } | ||
| } |
There was a problem hiding this comment.
-
코드 리뷰:
- 변경된 내용:
import구문이 추가 및user변수를 메소드 내에서 직접 선언. - 장점: 필요한 패키지 import 추가, 잘 정리된 코드.
- 주의해야 할 점:
user변수가 필드 레벨 변수에서 지역 변수로 변경되어 테스트 전체에서 사용할 수 없음.
- 변경된 내용:
-
개선 제안:
user필드를 다시 클래스 레벨 변수로 변경해서 모든 메소드에서 해당 객체에 접근 가능하게 수정.user를 private 필드로 선언하고 접근자(getter/setter)를 통해 접근하는 방식 고려.- 상수처럼 사용되는 문자열 "Bearer " 에 대한 상수로 추출하여 중복 제거.
- 예외 처리를 더 구체적으로 하여, 왜 해당 예외가 발생하는지 명확히 전달하는 것이 좋을 수 있음.
-
버그 리스크:
User user가 메소드 내부에 지역 변수로 선언되었으므로 해당 인스턴스는setUp()메소드 안에서만 유효합니다. 이후에user를 사용하는 메소드에서 문제가 발생할 수 있습니다.
| User user = userRepository.findTopByOrderByIdAsc().orElseThrow(RuntimeException::new); | ||
|
|
||
| RequestPostDto requestPostDto = new RequestPostDto("title" + random.nextInt(9999), "content" + random.nextInt(9999)); | ||
| Post post = Post.from(requestPostDto, user, null); | ||
| return postRepository.save(post); | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
코드 리뷰 요약:
- PostControllerTest 클래스에서 여러 중첩된 테스트들이 구현되어 있습니다.
- 테스트 메소드가 잘 그룹화되어 있고, 각 테스트 메소드는 적합한 이름과 기능을 가지고 있습니다.
- 썸네일이 있는 경우와 없는 경우에 대해 따로 테스트하는 방법으로 해결됩니다.
- 이미지 업로드 및 테스트 케이스의 JSON 응답을 확인하기 위해 mockMvc를 사용하는 것은 바람직합니다.
- 게시글 관련 조건에 따른 목록 조회 및 정렬에 대한 테스트가 적합하게 구현되었습니다.
- 수정 및 삭제에 대한 테스트 케이스도 적절히 포함되어 있습니다.
- 누락된 긴 설명이나 주석이야 하는 부분은 없는 것으로 보입니다.
개선사항:
getPostRepository.findTopByOrderByIdAsc()를 호출할 때에, 해당 결과가 null일 경우에 대한 처리가 필요할 수 있습니다.- 특정 동작에 대한 부수적인 설명이나 기대되는 결과를 주석으로 추가하여 코드 이해를 돕는 것이 좋습니다.
- 몇몇 네이밍 개선이 필요할 수 있고, 각 메소드나 변수의 역할을 더 명확히 드러내기 위해 이름 변경을 고려할 수 있습니다.
| ) | ||
| .andExpect(status().isOk()) | ||
| .andExpect(jsonPath("$.nickname").value(newNickname)); | ||
| } | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
-
개요:
- 테스트 코드에서 중첩 테스트를 사용하여 프로필 수정 시 사진이 있는 경우와 없는 경우를 각각 테스트하고 있습니다.
-
버그 위험:
- 주요한 버그 리스크가 보이지 않습니다.
-
개선 제안:
updateProfile메서드의 이름은 좀 더 구체적으로 정할 수 있습니다.- 중복되는 코드 부분을 줄여서 유지보수성을 높일 수 있습니다.
- 테스트 케이스에 실패 가능한 시나리오(예를 들어, 잘못된 파일 형식 등)를 추가하여 테스트 커버리지를 높일 수 있습니다.
- 예외 상황 처리에 대한 테스트도 추가하는 것이 좋습니다.
updateProfile클래스의 네임스페이싱을 더 명확하게 지정할 수 있습니다.
-
일반적인 추천사항:
- 모든 테스트 케이스에 대한 완전한 문구 설명 추가.
- 주석을 추가하여 비즈니스 로직에 대한 설명 및 기대 결과 명시.
-
종합:
- 현재 상태에서 큰 문제는 없어 보입니다. 특정 동작 변경이나 성능 개선을 위해 코드 리팩터링이나 추가 테스트 시나리오 탐색이 필요할 수 있습니다.
| @@ -24,8 +22,6 @@ | |||
|
|
|||
| class UserControllerTest extends AbstractTest { | |||
| private final String END_POINT = "/api/users/"; | |||
| @Autowired | |||
| UserRepository userRepository; | |||
| @MockBean | |||
| S3Service s3Service; | |||
|
|
|||
There was a problem hiding this comment.
코드 리뷰 및 개선 제안:
-
UserRepository에 대한 Autowired 어노테이션이 삭제되어 있습니다. 이 경우 UserRepository 객체가 주입되지 않아 NullPointerException 등의 예외가 발생할 수 있습니다.
-
JoinDto, LoginDto, 그리고 User 클래스를 import하여 사용하는 것을 확인할 수 있습니다.
-
MockBean 어노테이션을 통해 S3Service객체를 목 객체(mock object)로 주입하고 있습니다.
개선 제안:
-
UserRepository에 대한 의존성을 관리하기 위해 Autowired 어노테이션을 다시 추가하거나, Mockito나 다른 적절한 방법으로 UserRepository를 모의(mock)화 할 수 있습니다.
-
END_POINT 상수는 private final 로 선언되어 있으며 기본적인 URL 패스를 저장하고 있습니다. 변하지 않는 상수값이므로 static final 으로 변경하는 것이 좋습니다.
-
테스트 케이스 이름이 명확하지 않습니다. 좀 더 구체적이고 명확한 이름을 사용하여 가독성을 향상시키는 것이 좋습니다.
-
테스트 코드에서 필요없는 import 문이 보입니다. 사용하지 않는 import 문은 정리해주는 것이 좋습니다.
-
단위 테스트 작성 시, 외부 서비스에 대한 의존성을 줄이기 위해 Mockito와 같은 mocking 프레임워크를 사용하여 외부 서비스 호출을 대체하는 것이 권장됩니다.
이는 주어진 코드 조각의 현재 상태에 대한 간략한 리뷰와 개선 제안이었습니다.
#24