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

[4기 전선희] Spring Boot JPA 게시판 구현 미션 제출합니다! #220

Open
wants to merge 7 commits into
base: funnysunny08
Choose a base branch
from

Conversation

funnysunny08
Copy link

@funnysunny08 funnysunny08 commented Jul 31, 2023

📌 과제 설명

JPA 강의를 통해 배운 JPA, rest-docs 등을 적용하여 간단한 게시판을 구현하였습니다!

👩‍💻 요구 사항과 구현 내용

SpringDataJPA 를 설정한다.

  • datasource : mysql

엔티티를 구성한다

  • 회원(User)

  • 게시글(Post)

  • 회원과 게시글에 대한 연관관계를 설정한다.

    • 회원과 게시글은 1:N 관계이다.
  • 게시글 Repository를 구현한다. (PostRepository)
    image
    API를 구현한다.

  • 게시글 조회

    • 페이징 조회 (GET "/posts")

      image
    • 단건 조회 (GET "/posts/{id}")
      image
  • 게시글 작성 (POST "/posts")
    image

  • 게시글 수정 (POST "/posts/{id}")

    image

  • REST-DOCS를 이용해서 문서화한다.

Copy link

@hseong3243 hseong3243 left a comment

Choose a reason for hiding this comment

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

고생하셨습니다, 선희님! 코멘트 남겨봤습니다!

.andExpect(status().isOk())
.andDo(print())
.andDo(document("get-all-post",
responseFields(

Choose a reason for hiding this comment

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

쿼리 파라미터에 대해서도 문서화를 해주면 좋을 것 같아요!

UpdatePostRequest updatePostRequest = new UpdatePostRequest("바뀐 제목", null);

mockMvc.perform(patch("/posts/{postId}", post1.getPostId())
.header("userId", post1.getUser().getUserId())

Choose a reason for hiding this comment

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

경로 변수나 헤더에 대해서도 문서화를 해주면 좋을 것 같아요!


@ManyToOne(fetch = FetchType.EAGER, cascade = CascadeType.ALL)
@JoinColumn(name = "user_id")
private User user;

Choose a reason for hiding this comment

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

페치 타입은 eager, cascade 타입은 ALL로 설정하신 이유가 무엇인가요?

Copy link
Member

@bjo6300 bjo6300 left a comment

Choose a reason for hiding this comment

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

고생하셨습니다~

String title,
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.

CreatePostRequest와 UpdatePostRequest가 같다면 하나로 통합해서 사용하면 어떠신가요??


@Column
@Setter
private String hobby;
Copy link
Member

Choose a reason for hiding this comment

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

hobby만 Setter로 설정하신 이유가 궁금합니다!

Copy link
Member

Choose a reason for hiding this comment

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

Setter 보다 비즈니스적으로 의미있는 네이밍으로 변경 메서드를 제공하는 것은 어떨까요?

Copy link
Member

@JoosungKwon JoosungKwon 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 +4 to +5
String title,
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.

값의 대한 검증이 추가되면 어떨까요? Spring Validation에 대해 공부해보시면 좋을 것 같아요~

private final PostRepository postRepository;
private final UserRepository userRepository;

private static final int PAGE_SIZE = 10;
Copy link
Member

Choose a reason for hiding this comment

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

PAGE_SIZE를 서버에서 고정시키는 이유가 궁금합니다~ 매개 변수로 받을 수 있는 편이 좀 더 유연하지 않을까요?

Comment on lines +20 to +24
@RestControllerAdvice
@Component
@RequiredArgsConstructor
public class ControllerExceptionAdvice {
/**
Copy link
Member

Choose a reason for hiding this comment

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

@Component, @RequiredArgsConstructor 불필요한 것 같습니다~


@Column
@Setter
private String hobby;
Copy link
Member

Choose a reason for hiding this comment

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

Setter 보다 비즈니스적으로 의미있는 네이밍으로 변경 메서드를 제공하는 것은 어떨까요?

.content(request.content())
.user(user)
.build();
post.setCreatedBy(user.getName());
Copy link
Member

Choose a reason for hiding this comment

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

JPA Auditing을 사용하셨는데 이를 활용해도 되지 않나요?

String createdBy,
LocalDateTime createdAt
) {
public static PostResponse of(Long postId, String title, String content, String createdBy, LocalDateTime createdAt) {
Copy link
Member

Choose a reason for hiding this comment

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

Post를 매개변수로 받는 것도 고려해보셨나요?

Choose a reason for hiding this comment

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

주성님 말씀처럼 Post를 통해 받는 것이 파라미터 수를 줄일 수 있을 것 같네요 ㅎ

Comment on lines +24 to +29
public ApiResponse<PostsResponse> getAllPost(
@RequestParam int page
) {
if (page < 1) {
return ApiResponse.error(Error.PAGE_REQUEST_VALIDATION_EXCEPTION, Error.PAGE_REQUEST_VALIDATION_EXCEPTION.getMessage());
}
Copy link
Member

Choose a reason for hiding this comment

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

0은 포함하지 않나요..? Pageable을 파라미터로 전달받지 않으시는 이유가 궁금합니다~

@Getter
@RequiredArgsConstructor(access = AccessLevel.PRIVATE)
@AllArgsConstructor(access = AccessLevel.PRIVATE)
public class ApiResponse<T> {
Copy link
Member

Choose a reason for hiding this comment

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

ApiResponse를 따로 만드시는 이유가 궁금합니다~ code의 경우 HTTP Code와 동일한데 굳이 필요한가요..?

Copy link

@kakao-gray-great kakao-gray-great left a comment

Choose a reason for hiding this comment

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

선희님 미션하느라 고생 많으셨습니다 👍

몇 가지 코멘트 남겨놨으니 확인 부탁드려요!
추가로 도메인 계층에 대한 단위 테스트도 작성해보면 좋을 것 같아요.
저희의 주요 로직은 도메인에서 시작되는 것이 대부분인데 도메인에 대한 테스트가 없다면 프로젝트에 대한 확장 및 변경에 두려움이 생길 수 있을 것 같네요

return ApiResponse.error(Error.PAGE_REQUEST_VALIDATION_EXCEPTION, Error.PAGE_REQUEST_VALIDATION_EXCEPTION.getMessage());
}

return ApiResponse.success(Success.GET_POST_LIST_SUCCESS, postService.getAllPost(page));

Choose a reason for hiding this comment

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

다음과 같이 postService.getAllPost(page)에서 반환되는 값이 한줄로 표현되는 경우는 오히려 가독성을 해칠 수 있어요.
반환 값을 먼저 뺀 다음에 success의 파라미터로 넣어두는게 좋을 것 같아요.

}

@GetMapping("/{postId}")
@ResponseStatus(HttpStatus.OK)

Choose a reason for hiding this comment

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

Success.GET_POST_SUCCESS를 통해서 status를 반환하고 있는데, @ResponseStatus를 붙여줄 필요가 있나요?

String createdBy,
LocalDateTime createdAt
) {
public static PostResponse of(Long postId, String title, String content, String createdBy, LocalDateTime createdAt) {

Choose a reason for hiding this comment

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

주성님 말씀처럼 Post를 통해 받는 것이 파라미터 수를 줄일 수 있을 것 같네요 ㅎ

@@ -0,0 +1,23 @@
package com.board.server.domain.post.entity;

Choose a reason for hiding this comment

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

repository가 entity 패키지에 들어있는 이유는 무엇인가요?

Comment on lines +11 to +21
// CREATE

// READ
Page<Post> findAllByOrderByCreatedAtDesc(Pageable pageable);

@Query("select p from Post p join fetch p.user where p.postId = :postId")
Optional<Post> findById(Long postId);

// UPDATE

// DELETE

Choose a reason for hiding this comment

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

불필요한 주석은 혼란만 야기할 수 있어요 ㅎ
커밋하기 전에 확인하고 지워주는 습관을 가지는게 좋슴다

Comment on lines +5 to +9
public class NotFoundException extends CustomException {
public NotFoundException(Error error, String message) {
super(error, message);
}
}

Choose a reason for hiding this comment

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

커스텀 예외를 만드셨는데, 어떠한 이유로 만드시게 되었나요?

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

Successfully merging this pull request may close these issues.

5 participants