Skip to content
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

[김동영] 3차 과제 제출 #10

Open
wants to merge 14 commits into
base: develop
Choose a base branch
from

Conversation

goalSetter09
Copy link
Member

고민한 점

게시글 삭제 기능

JpaRepository의 기본 구현체인 SimpleJpaRepository의 deleteById 매서드는 다음과 같이 구현되어 있습니다.
Screenshot 2024-11-21 at 6 59 15 PM
id가 null이 아님만을 체크할 뿐, 막상 findById 매서드에서는 값이 없어도 별다른 처리없이 매서드가 끝납니다.

따라서 existsById 매서드를 통해 값이 존재하는지 체크하여 예외 처리를 하였습니다.
existsById는 SELECT COUNT(1) FROM post WHERE id = 1; 같이 엔터티의 존재 여부만 확인하는 쿼리를 사용하여 findById보다 효율적으로 동작합니다.
Screenshot 2024-11-21 at 7 18 56 PM

게시글 단일 조회시 조회수 증가

현재 방식은 읽기와 쓰기가 같은 트랜잭션 내에서 처리되어 문제가 있을 것 같습니다.. 다음 과제 때 리팩토링 하겠습니다.

엑셀 파일로 저장 기능

이 기능도 리팩토링해보겠습니다.

Copy link
Member

@yooooonshine yooooonshine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

역시 동영님~
이븐한 코드 잘 봤습니다!

동영님은 다음 과제 및 다다음 과제에서
동시성, 캐시를 통한 조회 성능 최적화, 인덱싱 쪽으로 파보시면 좋을 거 같아요!

if (!postRepository.existsById(postId)) {
throw ApiException.from(POST_NOT_FOUND);
}
postRepository.deleteById(postId);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pr 잘 읽었습니다!
existBy와 findBy까지 고려는 안해봤었는데 깊은 고민을 하신 게 느껴져요!
다만 지금 코드에서 봤을 때는

  1. 존재 확인 쿼리
  2. deleteById 에서 find 쿼리
  3. delete 쿼리
    로 기존 deleteById나 findById를 통해 delete를 호출하는 것보다 쿼리문이 1번 더 나가는 거 같은데
    제 이해가 맞을까요??.. 제 이해대로면 성능이 더 안좋아진거 같아서요!

description = "전체 게시글을 조회수 순으로 정렬한 후 페이징하여 조회합니다.(기본 페이지: 0, 페이지 별 기본 게시글 수: 10)"
)
public ResponseEntity<DataResponse<PostListFindResponse>> findPostListSortByViews(
@PageableDefault(page = 0, size = 10) Pageable pageable) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

와 이렇게 바로 pageable을 바로 받을 수 있군요!
첨 알았습니다..
유용하게 써먹을게요 ㅎㅎ

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

덕분에 @PageableDefault 어노테이션을 사용해서 페이지네이션 관련 인자 값들을 할당해 줄 수 있다는 것을 배웠습니다. 감사합니다!

Post post = postRepository.findById(postId)
.orElseThrow(() -> ApiException.from(POST_NOT_FOUND));

post.increaseViews();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 부분 추후 동시성 문제를 고려해보시면 좋을 거 같아요!

@@ -0,0 +1,8 @@
package cotato.backend.domains.post.dto.request;

public record SinglePostCreateRequest(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

validation을 추가해보면 좋을 거 같아요!

return Post.builder()
.title(title)
.content(content)
.name(name)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

혹시 보니까 Post 엔티티에서 views에 대한 디폴트 값을 따로 안 두셨던데
그럼 이 경우 views가 null로 들어가지 않나요??
추후 이 post조회하면 null예외가 터질 거 같은데..

}
@Id
@GeneratedValue
private Long id;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ID 생성 전략을 명시하지 않은 이유가 궁금합니다!

private String name;

@Column
private Long views;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

views 가 Long으로 설정되어 있는데 이럴 경우 기본적으로 Null 값으로 설정된다고 알고 있습니다. 이러한 상태에서 조회수를 증가시키는 로직을 사용하게 될 경우 null 예외가 발생할 위험이 있다고 생각됩니다!

@chanmin-00
Copy link
Member

existById에 대해서는 알지 못했는데 findById와의 명확한 차이점을 이해하기 쉽게 잘 알려주셔서 감사합니다! 깔끔하고 잘 정돈된 코드 잘 읽고 가요!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants