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

[14주차] 임현우 학습 PR 제출합니다. #2

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

woowal
Copy link

@woowal woowal commented Jan 2, 2024

Copy link
Member

@stophwan stophwan left a comment

Choose a reason for hiding this comment

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

전반적으로 너무 잘하셨습니다! 고생하셨습니다.

Comment on lines +11 to +13
private Long member;
private Long post;
private String content;
Copy link
Member

Choose a reason for hiding this comment

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

제약조건 걸어주기! 당장 member필드 post필드 content필드에 null이라 빈 값이 들어올 경우도 생각해주세요.
NotNull, NotEmpty, NotBlank의 차이점에 대해 공부해보면 좋습니다!

Comment on lines +25 to +30
@OneToMany(mappedBy = "member", cascade = CascadeType.ALL, orphanRemoval = true)
private List<Post> posts = new ArrayList<>();

@OneToMany(mappedBy = "member", cascade = CascadeType.ALL, orphanRemoval = true)
private List<Comment> comments = new ArrayList<>();

Copy link
Member

Choose a reason for hiding this comment

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

간단한 토이프로젝트라서 큰 상관은 없지만 양방향 매핑은 지양하는게 좋습니다!
왜 양방향 매핑을 지양해야 할까요? 특히 Jpa엔티티와 도메인이 같은 객체일 경우에는 일어날 수 있는 문제점이 정말 많습니다.
객체간 순환참조가 될 뿐만 아니라, 조금만 도메인이 많아져도 양방향 지옥에 빠져 어려움을 겪을 확률이 너무너무 큽니다... 관리도 안될 뿐더러 의도치 않는 쿼리가 나갈 수도 있고요.
정말 두 객체가 서로 알고 있어야하는지 고민해볼 필요가 있을 것 같아요.

@OneToMany(mappedBy = "member", cascade = CascadeType.ALL, orphanRemoval = true)
private List<Post> posts = new ArrayList<>();

@OneToMany(mappedBy = "member", cascade = CascadeType.ALL, orphanRemoval = true)
Copy link
Member

Choose a reason for hiding this comment

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

CascadeType.DELETE와 orphanRemoval 차이에 대해서도 학습하시면 좋을 것 같아요!

Comment on lines +39 to +42
List<String> comments = commentRepository.findByPostID(id)
.stream()
.map(Comment::getContent)
.collect(Collectors.toList());
Copy link
Member

Choose a reason for hiding this comment

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

메서드 분리하는게 좋아보여요!

Comment on lines +25 to +27
private final PostRepository postRepository;
private final MemberRepository memberRepository;
private final CommentRepository commentRepository;
Copy link
Member

@stophwan stophwan Jan 3, 2024

Choose a reason for hiding this comment

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

저는 개인적으로 PostRepository를 호출할 수 있는 것은 PostService라고 생각해요. 이 이야기는 뭐냐면 CommentRepository에 PostService가 직접 접근해 Comment를 가져와서 찾는 행위가 PostService에서 하는 것이 올바른 책임인가 생각이 듭니다.
CommentService에서 CommentRepository 사용하고 CommentService를 호출하는 것이 더 나아보입니다.
만약 다른 MemberService와 같이 많은 서비스에서 Comment가 필요하다면 그때마다 해당 도메인에 Service레이어가 CommentRepository를 의존해야하니까요.
물론 이런 경우에는 서비스 레이어간의 순환참조가 일어날 수도 있지 않느냐? 하는 의견이 있긴합니다만 저는 애당초 레이어간의 순환참조가 일어난다면 그건 애당초 잘못된 설계라고 생각합니다!

해당 내용에 대한 논의 글도 있으니 확인해보시면 좋을 것 같아요!
woowacourse/retrospective#15

Comment on lines +11 to +14
@Query("select new com.example.jpa.comment.domain.Comment(c.member, c.content) " +
"from Comment c " +
"where c.post.id = :id")
List<Comment> findByPostID(@Param("id") 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.

굳이 Query를 사용하신 이유가 있나요? 사용하지 않아도 될 것 같아요.

import java.util.List;

@RestController
@RequestMapping("api/posts")
Copy link
Member

Choose a reason for hiding this comment

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

이래도 작동은 하지만
"/api/posts" 이렇게 바꾸시는게 좋아요.

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.

2 participants