-
Notifications
You must be signed in to change notification settings - Fork 12
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차 과제 제출 #9
base: develop
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.
고생하셨습니다 규진님!
점점 많이 성장하시는 게 느껴져요!
이번 과제에서 약간! 아쉬운점이 보였어요
다른 분들 코드 잘 짜신 거 많으니까 꼭 여러 코드 봐보시고 배우셨으면 좋겠습니다!
@Column(nullable = false) | ||
private String name; | ||
|
||
@ColumnDefault("0") |
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.
오 이렇게 디폴트값을 지정할 수 있군요 배워갑니다!
@@ -23,4 +33,47 @@ public ResponseEntity<DataResponse<Void>> savePostsByExcel(@RequestBody SavePost | |||
|
|||
return ResponseEntity.ok(DataResponse.ok()); | |||
} | |||
|
|||
@PostMapping("/single") | |||
public ResponseEntity<DataResponse<Void>> savePostBySingle(@RequestBody PostDTO postDTO){ |
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.
validation도 추가해보시면 좋을 거 같아요!
return ResponseEntity.ok(DataResponse.ok()); | ||
} | ||
|
||
@GetMapping("/read/{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.
RESTful API 맞지 않는 url 같습니다!
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.
- 동사를 사용하지 않는다
posts/{id}/어찌고저찌고
순서가 더 맞지 않을까 싶습니다.
response.put("posts", filteredPosts); // 게시글 리스트 | ||
response.put("currentPage", postPage.getNumber()); // 현재 페이지 | ||
response.put("totalPages", postPage.getTotalPages()); | ||
|
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.
변환을 controller에서 하셨군요!
혹시 servcie에서 하지 않고 controller에서 하신 이유가 있으실까요?
return ResponseEntity.ok(response); | ||
} | ||
|
||
@DeleteMapping("/delete/{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.
/delete는 따로 필요없을거같아요!
method: delete
/api/posts/{id}
위 정보만으로도 충분해보입니다!
return content; | ||
} | ||
|
||
public String getName() { |
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를 직접 만드셨군요!
class 위에 @Getter
를 붙이면 모든 Getter 메서드가 자동으로 만들어진답니다! 놀랍지 않나요
} | ||
|
||
public Post readPostBySingle(Long id) { | ||
Optional<Post> optionalPost = postRepository.findById(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.
이 부분은 조금 잘 못 된 거 같습니다!
1번, 만약 id에 해당하는 Post가 없으면 Optional에는 null이 들어옵니다.
그럼 get을 할 경우 NullpointException이 나게 됩니다.
그래서 보통
postRepository.findById(id).orElseThrow(() -> Exception 객체))
식으로 예외처리를 해줍니다.
아마 예외 처리에 익숙하지 않으신 거 같은데 이쪽으로 공부해보시면 좋을 거 같습니다!
스프링에서의 전역 예외 처리 방법에 대해 정리해서 다음 과제 pr에 첨부해주세요!
|
||
return post; | ||
} | ||
|
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.
@Transactional(readOnly=true)
와 @Transactional
에는 성능차이가 있다는 것을 아시나요?
유진님 코드에 관련 리뷰있으니까 참고해보시면 좋을 거 같아요!
잘 읽었습니다! |
public Post readPostBySingle(Long id) { | ||
Optional<Post> optionalPost = postRepository.findById(id); | ||
Post post = optionalPost.get(); | ||
post.setViews(post.getViews() + 1); |
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.
Entity의 직접적인 Setter 사용은 값의 일관성을 해치는 결과가 나올수도 있어서 생성자@Builder
나 메소드를 통해서 별도로 값을 세팅하는 것을 추천드려요!
spring.datasource.username= root | ||
spring.datasource.password = 0000 |
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.
데이터베이스 키값이 외부로 노출되고 있어요! 보안상 문제가 발생할 수도 있으니 gitignore을 이용해서 해당 파일을 숨기는 것을 권장해 드려요
No description provided.