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기 박현지, 최지연] 컨트롤러 구현 및 테스트(RestDocs 적용), 엔티티 VO 분리 #263

Open
wants to merge 19 commits into
base: hyeonjiyeon
Choose a base branch
from

Conversation

hyeon-z
Copy link

@hyeon-z hyeon-z commented Aug 9, 2023

📌 과제 설명

컨트롤러 구현 및 테스트(RestDocs 적용), 엔티티 VO 분리

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

  • 게시글 컨트롤러 구현
  • 게시글 컨트롤러 테스트
  • 게시글 Title, Content VO로 분리

✅ 피드백 반영사항

  • 게시글 Title, Content VO로 분리

🤔 PR 포인트 & 궁금한 점

VO의 생성을 VO를 사용하는 객체(Post)내부에서 해줘야 할지, 객체 생성자의 파라미터로 VO를 만들어서 넣어줘야 할지 궁금합니다.

public Post(String title, String content, Long userId) {
    this.title = new Title(title);
    this.content = new Content(content);
    this.userId = userId;
}
public Post(Title title, Content content, Long userId) {
    this.title = title;
    this.content = content;
    this.userId = userId;
}

@hyeon-z hyeon-z self-assigned this Aug 9, 2023
@hyeon-z hyeon-z changed the title feat: 컨트롤러 구현 및 테스트(RestDocs 적용), 엔티티 VO 분리 [4기 박현지, 최지연] 컨트롤러 구현 및 테스트(RestDocs 적용), 엔티티 VO 분리 Aug 9, 2023
Copy link
Member

@Juhongseok Juhongseok left a comment

Choose a reason for hiding this comment

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

👍

}

@GetMapping("/{id}")
public ResponseEntity<PostResponseDto> getOne(@PathVariable Long id) throws NoSuchElementException {
Copy link
Member

Choose a reason for hiding this comment

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

NoSuchElementException은 RuntimeException으로 알고있는데 굳이 명시한 이유가 있을까요?

Copy link
Author

Choose a reason for hiding this comment

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

좀 더 명확히 exception이 발생하는 의미를 담고 싶어서 NoSuchElementException을 사용했습니다!

import org.prgms.boardservice.domain.post.Post;
import org.prgms.boardservice.domain.post.Title;

public record PostCreateRequestDto(String title, String content, Long userId) {
Copy link
Member

Choose a reason for hiding this comment

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

Dto라는 suffix는 없어도 될거같아요

Copy link
Author

Choose a reason for hiding this comment

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

public class Title {

@Column(length = 20)
@NotNull
Copy link
Member

Choose a reason for hiding this comment

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

아래 validateTitleLength에서 null 검증하는데 굳이 또 작성해줘야 하나요?

Copy link
Author

Choose a reason for hiding this comment

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

해당 부분은 db의 설정이기 때문에 저희가 validate메서드로 검사하더라도 db에서의 not null 설정은 해줘야 한다고 생각했습니다!

Copy link

@juno-junho juno-junho left a comment

Choose a reason for hiding this comment

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

많은 시간을 쏟으신게 보입니다!
고생하셨습니다 👍

}

private void validateContentLength(String value) {
if (!hasText(value) || value.length() > 500) {

Choose a reason for hiding this comment

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

if 문의 조건들을 boolean type의 method으로 extract 해서 리펙토링해도 좋을것 같습니다!

Copy link
Author

Choose a reason for hiding this comment

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

이미 해당 조건들은 validateContentLength라는 메서드로 분리했기 때문에 조건을 또 한 번의 메서드로 분리하게 되면 오히려 한눈에 파악하기 어렵다고 생각했습니다..!

this.postService = postService;
}

@ExceptionHandler(NoSuchElementException.class)

Choose a reason for hiding this comment

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

👍 예외 처리까지 하셨네요

Choose a reason for hiding this comment

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

Exception을 그런데 받는 이유가 있나요? NoSuchElementException를 받아도 되지 않나요?

Copy link
Author

Choose a reason for hiding this comment

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

}

private void validateTitleLength(String value) {
if (!hasText(value) || value.length() > 20) {

Choose a reason for hiding this comment

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

Assert.hasText(value, message) 사용 고려해봐도 괜찮을것같아요

import java.util.regex.Pattern;

import static org.prgms.boardservice.util.ErrorMessage.*;

@Getter
@Entity
@NoArgsConstructor(access = AccessLevel.PROTECTED)
@EqualsAndHashCode(of = "id", callSuper = false)
public class User extends BaseTime {

private static final String EMAIL_REGEX = "^[\\w-\\.]+@([\\w-]+\\.)+[\\w-]{2,4}$";

Choose a reason for hiding this comment

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

email도 VO로 빼는게 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

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

private final String titleMoreThenTwentyLength = "TitleMoreThenTwentyLength";
private final String contentMoreThanFiveHundredLength = "sit amet cursus sit amet dictum sit amet justo donec enim diam vulputate ut pharetra sit amet aliquam id diam maecenas ultricies mi eget mauris pharetra et ultrices neque ornare aenean euismod elementum nisi quis eleifend quam adipiscing vitae proin sagittis nisl rhoncus mattis rhoncus urna neque viverra justo nec ultrices dui sapien eget mi proin sed libero enim sed faucibus turpis in eu mi bibendum neque egestas congue quisque egestas diam in arcu cursus euismod quis viverra nibh cras pulvinar mattis nunc sed blandit libero volutpat sed cras ornare arcu dui vivamus arcu felis bibendum ut tristique et egestas";
private static final String titleMoreThenTwentyLength = "TitleMoreThenTwentyLength";
private static final String contentMoreThanFiveHundredLength = "sit amet cursus sit amet dictum sit amet justo donec enim diam vulputate ut pharetra sit amet aliquam id diam maecenas ultricies mi eget mauris pharetra et ultrices neque ornare aenean euismod elementum nisi quis eleifend quam adipiscing vitae proin sagittis nisl rhoncus mattis rhoncus urna neque viverra justo nec ultrices dui sapien eget mi proin sed libero enim sed faucibus turpis in eu mi bibendum neque egestas congue quisque egestas diam in arcu cursus euismod quis viverra nibh cras pulvinar mattis nunc sed blandit libero volutpat sed cras ornare arcu dui vivamus arcu felis bibendum ut tristique et egestas";

@ParameterizedTest(name = "test with value [{0}]")
@NullAndEmptySource

Choose a reason for hiding this comment

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

이렇게 null 값도 줄 수 있군요 배워갑니다! 👍

Copy link

@beomukim beomukim 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 +25 to +26
@Embedded
private Content content;
Copy link

Choose a reason for hiding this comment

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

@Embedded를 사용할 때 주의사항이 뭐가 있을까요?

Copy link
Author

Choose a reason for hiding this comment

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

같은 타입을 사용하게 되면 @AttributeOverride로 속성 명을 재정의 해줘야 하고 동등성 비교를 위해 equalshashCode를 재정의 해야 합니다!

Copy link

@choi5798 choi5798 left a comment

Choose a reason for hiding this comment

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

수고하셨습니다! 👍
restdocs 까지 잘 작성해주셨네요!
다른 분들이 코멘트 잘 달아주셔서 바로 approve 하겠습니다

Comment on lines +56 to +73
@ParameterizedTest(name = "title: {0}, content: {1}")
@MethodSource("postInvalidInfo")
@DisplayName("게시글을 유효하지 않은 제목, 내용으로 변경할 수 없다.")
void fail_Change_Post_With_Invalid_Title(String title, String content) {
Post post = new Post(new Title("title"), new Content("content"), userId);

assertThatThrownBy(() -> post.changeTitle(title))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage(INVALID_POST_TITLE.getMessage());
assertThatThrownBy(() -> post.update(new Title(title), new Content(content)))
.isInstanceOf(IllegalArgumentException.class);
}

@ParameterizedTest(name = "test with value [{0}]")
@NullAndEmptySource
@ValueSource(strings = {" ", contentMoreThanFiveHundredLength})
@DisplayName("게시글을 유효하지 않은 내용으로 변경할 수 없다.")
void fail_Change_Post_With_Invalid_Content(String content) {
Post post = new Post("title", "content", userId);

assertThatThrownBy(() -> post.changeContent(content))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage(INVALID_POST_CONTENT.getMessage());
static Stream<Arguments> postInvalidInfo() {
return Stream.of(
Arguments.arguments(null, null),
Arguments.arguments("", ""),
Arguments.arguments(" ", " "),
Arguments.arguments(titleMoreThenTwentyLength, "content"),
Arguments.arguments("title", contentMoreThanFiveHundredLength)
);
Copy link

@choi5798 choi5798 Aug 10, 2023

Choose a reason for hiding this comment

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

ParameterizedTest 덕분에 깔끔해진 것 같습니다 👍

Copy link

@ddongpuri ddongpuri 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 +41 to +44
public ResponseEntity<PostResponse> getOne(@PathVariable Long id) throws NoSuchElementException {
PostResponse postResponse = new PostResponse(postService.getById(id));

return ResponseEntity.ok(postResponse);
Copy link
Member

Choose a reason for hiding this comment

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

NoSuchElementException을 던지는 것을 명시해준 이유가 있을까요?

Copy link

@ICCHOI ICCHOI Aug 16, 2023

Choose a reason for hiding this comment

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

저도 Exception을 달아둔 이유가 궁금하네요

Copy link
Member

@Juhongseok Juhongseok left a comment

Choose a reason for hiding this comment

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

👍

@Getter
@Entity
@NoArgsConstructor(access = AccessLevel.PROTECTED)
@AllArgsConstructor
@DynamicUpdate
Copy link

Choose a reason for hiding this comment

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

@DynamicUpdate를 사용한 이유가 무엇인가요?

Choose a reason for hiding this comment

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

jpa 공부하면서 컬럼 수가 굉장히 많은 경우가 아니라면 @DynamicUpdate 를 사용하는 것이 오히려 좋지 않을 수 있다는 것을 알게되었습니다. 해당 어노테이션 삭제하겠습니다!

return ResponseEntity.ok(new PageResponse<>(postResponseDtoPage));
}

@PatchMapping("/{id}")
Copy link

Choose a reason for hiding this comment

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

Put이 아니라 Patch인 이유가 있을까요?

Choose a reason for hiding this comment

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

이미 존재하는 자원에 대해서 부분적인 수정만 이루어지는 것을 의도해서 Patch 가 더 적합하다고 생각했습니다.

}

private void validateTitleLength(String value) {
if (!hasText(value) || value.length() > 20) {
Copy link

Choose a reason for hiding this comment

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

여기의 20과 title의 lenght, db의 제약조건 등, 타이틀 길이를 변경하려면 해야하는 일이 너무 많아지는 것 같다는 생각이 들어요.

import static org.prgms.boardservice.util.ErrorMessage.INVALID_POST_CONTENT;
import static org.prgms.boardservice.util.ErrorMessage.INVALID_POST_TITLE;

@Builder

Choose a reason for hiding this comment

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

public 생성자도 사용하고 builder도 사용하고있나오?

Copy link

@ddongpuri ddongpuri left a comment

Choose a reason for hiding this comment

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

@Getter
@Entity
@NoArgsConstructor(access = AccessLevel.PROTECTED)
@AllArgsConstructor
@DynamicUpdate

Choose a reason for hiding this comment

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

jpa 공부하면서 컬럼 수가 굉장히 많은 경우가 아니라면 @DynamicUpdate 를 사용하는 것이 오히려 좋지 않을 수 있다는 것을 알게되었습니다. 해당 어노테이션 삭제하겠습니다!

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.

None yet

9 participants