-
Notifications
You must be signed in to change notification settings - Fork 204
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
base: hyeonjiyeon
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NoSuchElementException은 RuntimeException으로 알고있는데 굳이 명시한 이유가 있을까요?
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dto라는 suffix는 없어도 될거같아요
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아래 validateTitleLength에서 null 검증하는데 굳이 또 작성해줘야 하나요?
There was a problem hiding this comment.
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 설정은 해줘야 한다고 생각했습니다!
There was a problem hiding this 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) { |
There was a problem hiding this comment.
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 해서 리펙토링해도 좋을것 같습니다!
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 예외 처리까지 하셨네요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exception을 그런데 받는 이유가 있나요? NoSuchElementException를 받아도 되지 않나요?
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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}$"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
email도 VO로 빼는게 어떨까요?
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이렇게 null 값도 줄 수 있군요 배워갑니다! 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
현지님 지연님 수고하셨습니다! 👍
@Embedded | ||
private Content content; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Embedded
를 사용할 때 주의사항이 뭐가 있을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
같은 타입을 사용하게 되면 @AttributeOverride
로 속성 명을 재정의 해줘야 하고 동등성 비교를 위해 equals
와 hashCode
를 재정의 해야 합니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수고하셨습니다! 👍
restdocs 까지 잘 작성해주셨네요!
다른 분들이 코멘트 잘 달아주셔서 바로 approve 하겠습니다
@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) | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ParameterizedTest 덕분에 깔끔해진 것 같습니다 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😎
public ResponseEntity<PostResponse> getOne(@PathVariable Long id) throws NoSuchElementException { | ||
PostResponse postResponse = new PostResponse(postService.getById(id)); | ||
|
||
return ResponseEntity.ok(postResponse); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NoSuchElementException
을 던지는 것을 명시해준 이유가 있을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저도 Exception을 달아둔 이유가 궁금하네요
There was a problem hiding this 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DynamicUpdate를 사용한 이유가 무엇인가요?
There was a problem hiding this comment.
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}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put이 아니라 Patch인 이유가 있을까요?
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public 생성자도 사용하고 builder도 사용하고있나오?
There was a problem hiding this 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jpa 공부하면서 컬럼 수가 굉장히 많은 경우가 아니라면 @DynamicUpdate 를 사용하는 것이 오히려 좋지 않을 수 있다는 것을 알게되었습니다. 해당 어노테이션 삭제하겠습니다!
📌 과제 설명
컨트롤러 구현 및 테스트(RestDocs 적용), 엔티티 VO 분리
👩💻 요구 사항과 구현 내용
✅ 피드백 반영사항
🤔 PR 포인트 & 궁금한 점
VO의 생성을 VO를 사용하는 객체(Post)내부에서 해줘야 할지, 객체 생성자의 파라미터로 VO를 만들어서 넣어줘야 할지 궁금합니다.