-
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차 과제 제출 #5
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.
아니 왜 다 잘하시죠..
private final PostService postService; | ||
|
||
// 엑셀 파일로부터 게시글 저장 API | ||
@Operation(summary = "엑셀 파일로부터 게시글 저장", description = "엑셀 파일로부터 게시글을 저장합니다.") |
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.
오 swagger 설명 명시한 거 좋네요
// 엑셀 파일로부터 게시글 저장 API | ||
@Operation(summary = "엑셀 파일로부터 게시글 저장", description = "엑셀 파일로부터 게시글을 저장합니다.") | ||
@PostMapping("/excel") | ||
public ResponseEntity<DataResponse<Void>> savePostsByExcel(@RequestBody SavePostsByExcelRequest request) { |
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에 Validation을 적용해봐도 좋을 거 같아요!
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.
리팩토링 하면서 수정하겠습니다!
|
||
// 단일 게시글 저장 API | ||
@Operation(summary = "게시글 저장", description = "단일 게시글을 저장합니다.") | ||
@PostMapping("/create") |
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.
혹시 url에 동사를 명시하신 이유가 있을까요?
restful api 규칙에 맞지 않아보여서요!
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 규칙에 대해서 명확히 알지 못해서 그랬던거 같습니다. api 규칙에 대해 알아보고 수정하도록 하겠습니다!
|
||
// 현재 페이지 | ||
private int currentPage; | ||
} |
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.
Request는 @Data
를 사용하셨던데 Response는 사용하지 않으신 이유가 있으실까요
그리고 Builder 패턴과 Allagrs를 둘 다 public 으로 열어두신 이유가궁금합니다!
Builder 패턴을 사용하시려고 명시하신거면 AllArgs는 private으로 닫아도 되지 않았을까 싶어서요!
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.
또한 NoArgs 또한 Json 기능 때문에 열어두신 거 같은데 Public 말고 private 사용해도 되지 않았을까 싶네요!
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.
Request는
@Data
를 사용하셨던데 Response는 사용하지 않으신 이유가 있으실까요
응답 DTO는 의도치 않은 데이터 변경 방지를 위해 @Setter를 포함하는 @DaTa 어노테이션을 사용하지 않은 것입니다. 요청 DTO에 대해서는 그 부분을 파악하지 못했네요 지금 생각해보니 요청 DTO도 @DaTa 어노테이션 사용을 지양하는게 좋을거 같습니다!
그리고 Builder 패턴과 Allagrs를 둘 다 public 으로 열어두신 이유가궁금합니다! Builder 패턴을 사용하시려고 명시하신거면 AllArgs는 private으로 닫아도 되지 않았을까 싶어서요!
그렇네요 빌더 패턴을 사용해서 객체 생성을 할려는 의도가 있기 때문에 다른 방식으로 객체 생성을 하는 것 막을 필요가 있겠네요!
} | ||
|
||
// 내용 설정 | ||
public void setContent(String 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.
이 부분은 필드 위에 @Setter
애노테이션 붙이면 그 필드만의 Setter가 만들어지는 거 아시나요?
저도 최근에 알았어서 공유드려요
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.
좋은 정보 감사합니다!
ps.setString(2, post.getContent()); | ||
ps.setString(3, post.getName()); | ||
ps.setLong(4, post.getViews()); | ||
}); |
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.
또 BulkInsert를..
// 로컬 파일 경로로부터 엑셀 파일을 읽어 Post 엔터티로 변환하고 저장 | ||
@Transactional | ||
public void saveEstatesByExcel(String filePath) { | ||
if (filePath == 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.
이 validation은 DTO에서 처리해보는 건 어떨까요?
나중에 테스트 코드를 작성할 때 filePath가 null이면 예외를 던진다를 테스트하려면
서비스를 호출해야 하잖아요?
그럼 부담이 크기에 DTO 생성에서 validation을 넣으면 빠르게 테스트할 수 있다는 장점이 있거든요!
@Transactional | ||
public void deletePost(Long id) { | ||
Post post = postRepository.findById(id).orElseThrow(() -> ApiException.from(POST_NOT_FOUND)); | ||
postRepository.delete(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.
혹시 이 부분 동일하게 동작하는 deleteById()를 사용하시면 좀 더 깔끔해질 수 있을 거 같아요!
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.
신경많이 써서 작성하신게 느껴지는 코드였습니다! 잘보구가요👍
import cotato.backend.domains.post.dto.response.GetPostResponse; | ||
|
||
@Component | ||
public class PostConverter { |
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.
오 찬민님은 Converter 클래스를 따로 만들어서 사용하시네요! 하나 배워갑니다
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.
Converter class를 따로 만드신 이유가 궁금해요!
// 단일 게시글 저장 요청 | ||
@Data | ||
@NoArgsConstructor(access = AccessLevel.PRIVATE) | ||
public class SavePostRequest { |
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.
Data 애노테이션은 지우는게 좋을 것 같아요! (Setter가 포함되어 있어서 dto에는 지양하는게 좋다구 하네요..)
저는 요즘 DTO 클래스만들때 record를 자주사용하는데 한번 찾아보셔도 좋을 것 같아요!
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.
record를 활용하는 방식이 코드도 깔끔하고 좋더라고요. record 방식을 활용해서 수정해보겠습니다!
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.
코드 잘 봤습니다! 궁금한 것 위주로 코멘트 달았습니다
수고많으셨어요!!
// 단일 게시글 조회 API | ||
@Operation(summary = "게시글 조회", description = "게시글을 조회합니다.") | ||
@GetMapping("/{id}") | ||
public ResponseEntity<DataResponse<GetPostResponse>> getPostById(@PathVariable Long 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.
경로 변수로 id 값을 받는데 서비스 로직 상 꼭 필요한 변수이니 Long 대신 원시 타입인 long
을 사용하면 의도가 명확히 전달될 것 같아요!
원시 타입을 사용하면 Null 값이 들어오는 것을 막고, 어디서 터질지 모르는 NPE를 방지할 수 있어요
|
||
// 단일 게시글 저장 | ||
@Transactional | ||
public void savePost(SavePostRequest request) { |
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.
클라이언트로부터 전달받은 SavePostRequest
DTO는 어떤 역할을 하고 어떤 책임이 있을까요? 왜 사용할까요?
저는 사용 이유를 계층간 분리라고 생각하고 있어요.
Client <-> Server 간 데이터 전송은 저희가 정한 규격인 API에 따라 SavePostRequest
DTO를 통해 값을 전달받고,
Controller <-> Service 간 데이터 전송은 서비스에서 정해진 비즈니스 로직에 따라 처리하기 위한 값들을 다른 DTO를 통해 전달받도록 분리하는 편이에요.
이렇게 분리하게 되면 서비스 계층에서 전달받는 값을 수정해도 클라이언트로부터 받는 값은 변경되지 않아 API 규격이 유지되는 장점이 있어요.
찬민님은 어떻게 생각하실까요!
public void savePost(SavePostRequest request) { | ||
if (request.getTitle() == null || request.getContent() == null || request.getName() == null) { | ||
throw ApiException.from(INVALID_PARAMETER); | ||
} |
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으로 컨트롤러 단에서 처리하는 편인데요,
혹시 이러한 값에 대한 검증은 어떤 계층에서 해야한다고 생각하실까요?
예를 들어 저처럼 컨트롤러 단에서 검증할 수도 있고 또는 찬민님처럼 서비스 계층에서 할 수 있어요. 그리고 엔티티 객체를 생성하는 과정에서 검증을 하게 되면 레포지토리 계층에서 하게 돼요.
제 결론은 컨트롤러 단에서 하는 것이었는데, 찬민님의 의견이 궁금해요.
레포지토리 계층의 경우 엔티티 클래스에 뷰 계층을 위한 @JsonIgnore
과 같은 기능이 추가되면 안되는 것처럼 비즈니스 요구사항에 대한 검증 로직도 들어가면 안된다고 생각했어요. 그리고 서비스 로직 같은 경우 맥락 상 검증 로직을 포함해도 되지만, 컨트롤러로부터 넘겨받은 값이 비즈니스 요구사항을 충족한다는 것을 보장한 상태로 처리하는 것이 구현에 집중할 수 있다고 생각했어요. 추가로 원래 컨트롤러 계층에서는 클라이언트가 전송하는 값을 약간 검증하는 책임도 있기도 했구요.
public GetPostPreviewListResponse getPostPreviewList(int page) { | ||
if (page < 0) { | ||
throw ApiException.from(INVALID_PARAMETER); | ||
} |
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.
컨트롤러에서 @PositiveOrZero
어노테이션을 사용해 0 또는 양수 값을 받도록 보장할 수 있어요!
근데 저도 이거 쓰는거 까먹었어요 ㅋㅋㅋ
Pageable pageable = Pageable.ofSize(10).withPage(page); | ||
Page<Post> postPage = postRepository.findAllByOrderByViewsDesc(pageable); | ||
|
||
List<GetPostPreviewResponse> postPreviewList = postPage.getContent() |
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.
GetPostPreviewResponse
도 위에서 말씀드린 것과 동일합니다!
@@ -0,0 +1,21 @@ | |||
spring: |
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 파일에서 application.*
등과 같이 작성해 해당 파일은 내리는 것이 좋아보여요!
보안 상의 이슈가 있거든요
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.
깔끔한 코드 잘 봤습니다! 감사합니다~
import cotato.backend.domains.post.dto.response.GetPostResponse; | ||
|
||
@Component | ||
public class PostConverter { |
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.
Converter class를 따로 만드신 이유가 궁금해요!
게시글 저장 기능 구현
메소드 내에서 파라미터에 대한 유효성 검사를 진행하였고, JpaRepository의 save 메소드를 활용하여서 저장하였습니다.
엑셀로부터 읽어온 데이터 저장 기능 구현
JpaRepository의 saveAll 메소드를 활용하여 데이터를 저장하려고 시도를 했으나, 게시글을 저장할 때마다 insert 쿼리가 발생하였고 데이터베이스 접근이 많이 일어나 성능이 저하가 되는 문제를 발견했습니다.
성능 문제를 해결하기 위해 jdbcTemplate을 활용해서 bulk insert 방식으로 변경을 했습니다. PostCumtomRepository에 bulk insert 방식을 구현하였고, 해당 인터페이스를 PostRepository 인터페이스가 상속을 받아서 사용할 수 있도록 했습니다.
게시글 조회 기능 구현
JpaRepository의 findById 메소드를 활용하여 구현했습니다.
게시글 전체 조회 페이징 기능 구현
일반적인 쿼리문으로 구현을 하였습니다. 해당 메소드는 모든 데이터를 조회수 기준으로 정렬을 하기 때문에 테이블의 데이터가 더욱 더 많아질수록 정렬 비용이 더 커지고 성능 문제가 발생될 것이라고 예측이 됩니다.
아직 이 문제에 대한 해결책을 찾지는 못하였고, 코드 리팩토링을 진행하면서 해당 문제에 대한 해결책을 고민해볼 예정입니다.
게시글 삭제 기능 구현
jpaRepository의 delete 메소드를 활용했습니다.
Converter 활용
빌더 패턴을 활용하여 객체를 생성할 수 있도록 하였고, 컨버터를 활용하여 엔터티와 DTO간의 변환이 용이할 수 있도록 구현했습니다
코드 리팩토링 예정사항