Feat #73 post,notice, receipt 파일 업로드 수정 #73
Conversation
hyxklee
changed the title
| fileRepository.delete(file); | ||
| } | ||
|
|
||
| public void delete(List<File> files) { |
There was a problem hiding this comment.
메소드 이름을 deleteAll로 하는게 나아보여용
There was a problem hiding this comment.
단일 파일 삭제랑 구분하기 위해서 deleteAll이 더 직관적인거 같습니다 !
There was a problem hiding this comment.
메서드 명에 대한 추상화를 위해 메서드 오버로딩을 사용해 같은 메서드 명으로 사용했습니다!
메서드 명은 같지만 파라미터의 타입이 다르기 때문에 구분이 가능하고, 외부에서는 캡슐화를 통해 메서드의 자세한 로직은 숨기고, 다형성을 구현해 확장성과 유연성을 조금이나마 확보할 수 있다고 생각하는데 이런 관점에서는 어떻게 생각하시나요??
구분하는게 나을 것 같다면 수정 하겠습니다!
| @Service | ||
| @RequiredArgsConstructor | ||
| public class FileUpdateService { | ||
|
|
||
| public void update(File file, FileUpdateRequest dto) { | ||
| file.update(dto.fileName(), dto.fileUrl()); | ||
| } | ||
|
|
||
| public void updateFiles(List<File> fileList, List<FileUpdateRequest> dto) { | ||
| for (File file : fileList) { |
There was a problem hiding this comment.
혹시라도 둘중 하나라도 비어있을 수 있으니 exception을 추가하면 좋아보여요!
There was a problem hiding this comment.
저도 해당 부분에 서비스단에서 비어있을 경우의 예외처리를 던져주는 방식이 좋아보입니당
There was a problem hiding this comment.
파일의 경우 업로드가 될 수도 있고 안될 수도 있기 때문에 비어있는 경우에 예외를 날리기 보다는 저장된 파일이 있는 지 usecase에서 체크해서 해당 메서드를 호출하고, dto가 null이 아닌 경우에만 로직이 돌아가게 수정 하겠습니다.
그리고 입력된 fileId가 올바른지 아닌지 검증하는 로직을 추가하겠습니다
| return fileGetService.findAllByPost(postId); | ||
| } | ||
|
|
||
| private Post validateOwner(Long postId, Long userId) { |
There was a problem hiding this comment.
여기도 Notice의 validateOwner와 마찬가지 입니당
| return fileGetService.findAllByNotice(noticeId); | ||
| } | ||
|
|
||
| private Notice validateOwner(Long noticeId, Long userId) { |
There was a problem hiding this comment.
검증만을 위해서면 void형을 사용해서 더 명확하게 할수있을거같아여
There was a problem hiding this comment.
모든 메서드에
Notice notice = noticeFindService.find(noticeId); 가 포함되고 있어서 해당 코드 반복을 줄이고자 이 메서드에서 find에서 return 하는 것으로 구현 했어요!
| } | ||
|
|
||
| @Override | ||
| public void delete(Long noticeId, Long userId) throws UserNotMatchException { | ||
| @Transactional |
There was a problem hiding this comment.
혹시 여기에 @transactional(rollbackFor = UserNotMatchException.class)로 바뀌는게 더 나을까요?
There was a problem hiding this comment.
UserNotMatchException이 RuntimeException을 상속하는 BusinessLogicException을 상속하기 때문에 예외 발생시 롤백이 돼서 추가하지 않아도 될 것 같아요!
| @Service | ||
| @RequiredArgsConstructor | ||
| public class FileUpdateService { | ||
|
|
||
| public void update(File file, FileUpdateRequest dto) { | ||
| file.update(dto.fileName(), dto.fileUrl()); | ||
| } | ||
|
|
||
| public void updateFiles(List<File> fileList, List<FileUpdateRequest> dto) { | ||
| for (File file : fileList) { |
There was a problem hiding this comment.
저도 해당 부분에 서비스단에서 비어있을 경우의 예외처리를 던져주는 방식이 좋아보입니당
| @Mapping(target = "id", ignore = true) | ||
| @Mapping(target = "post", source = "post") | ||
| File toFile(String fileName, String fileUrl, Post post); | ||
|
|
||
| @Mapping(target = "id", ignore = true) | ||
| @Mapping(target = "notice", source = "notice") | ||
| File toFile(String fileName, String fileUrl, Notice notice); | ||
|
|
||
| @Mapping(target = "id", ignore = true) | ||
| @Mapping(target = "receipt", source = "receipt") | ||
| File toFile(String fileName, String fileUrl, Receipt receipt); |
There was a problem hiding this comment.
toFile의 변수명으로 post, notice, receipt보다는
linkedPost, linkedNotice, linkedReceipt로 더 의미를 명확하게 해주는건 어떨까요
There was a problem hiding this comment.
toFileWithPost 이런 식은 어떨까용?
| fileRepository.delete(file); | ||
| } | ||
|
|
||
| public void delete(List<File> files) { |
There was a problem hiding this comment.
단일 파일 삭제랑 구분하기 위해서 deleteAll이 더 직관적인거 같습니다 !
PR 내용
PR 세부사항
관련 스크린샷
주의사항
체크 리스트