board image update #30
Conversation
| @@ -47,8 +47,7 @@ public void nickValidation( | |||
| public CommonIdResponse loginMember( | |||
| @RequestBody @Valid LoginRequest loginRequest, | |||
| HttpServletRequest request) { | |||
| Member member = memberService.login(loginRequest); | |||
| return memberService.provideSession(member, request); | |||
| return memberService.provideSession(memberService.login(loginRequest), request); | |||
There was a problem hiding this comment.
이제보니
memberService.login(loginRequest);
memberService.provideSession();
이 두개를 하나로 합쳐도 될것같은데요.
사용하는 곳이 이곳밖에 없는데 나뉘어져 있어서요.
혹시 나눈 이유가 있을까요?
There was a problem hiding this comment.
나눈이유는 딱히 없었습니다! 하나로 합치는게 좋겠네요!
| @@ -26,7 +28,7 @@ public ContentListResponse<BoardResponse> myBoardList( | |||
| @RequestParam Long page, | |||
| @SessionAttribute Long memberId | |||
| ) { | |||
| return boardService.findBoards(size, page, memberId); | |||
| return boardService.findBoards(size, page, Optional.ofNullable(memberId)); | |||
There was a problem hiding this comment.
저는 개인적으로 Optional이 남용되기 쉬워서 사용을 최대한 자제하는 편인데요.
현재 Optional은 memberId가 null 일때 익셉션을 Throw 하는 용도로만 사용하고 있습니다.
아래 글에서 16번 Item을 읽어보면 좋을것같습니다.
https://dzone.com/articles/using-optional-correctly-is-not-optional
Optional 코드 안에 보면
* @apiNote
{@code Optional} is primarily intended for use as a method return type where
there is a clear need to represent "no result," and where using {@code null}
is likely to cause errors. A variable whose type is {@code Optional} should
never itself be {@code null}; it should always point to an {@code Optional}
instance.
=> 메서드가 반환할 결과값이 ‘없음’을 명백하게 표현할 필요가 있고, null을 반환하면 에러를 유발할 가능성이 높은 상황에서 메서드의 반환 타입으로 Optional을 사용하자는 것이 Optional을 만든 주된 목적
라고 적혀있는데 이부분에 대한 고민을 해보면 좋을듯합니다.
| if (!imgOriginalName.substring(imgOriginalName.lastIndexOf(".")).matches("(.png|.jpg|.jpeg)$")) { | ||
| public ImageInfoResponse saveImageToS3(MultipartFile image) { | ||
| imageValidation(image); | ||
| String uuid = s3Service.saveFile(image, "tmp"); |
There was a problem hiding this comment.
"tmp"가 여러군데에서 사용되면 상수로 관리하는것도 좋아보입니다.
| return new ImageInfoResponse(uuid, image.getSize(), image.getOriginalFilename(), imgSrc); | ||
| } | ||
|
|
||
| private static void imageValidation(MultipartFile image) { |
There was a problem hiding this comment.
static 이어야 할 이유가 있을까요?
There was a problem hiding this comment.
메서드 추출을 이용했는데 static이 붙어버렸네요... 주의해야겠네요!
| log.info("memberId = {}"); | ||
| throw new BoardException(BoardStatus.INVALID_MEMBER_ID); | ||
| } | ||
| public ContentListResponse<BoardResponse> findBoards(Long size, Long requestPage, Optional<Long> memberId) { |
There was a problem hiding this comment.
위에 findBoards를 호출하는 부분에서 드린 리뷰를 참고하면 좋을듯합니다.
| // 기존 이미지 정보 | ||
| List<String> savedImageUuid = imageMapper.findImageUuid(boardId); | ||
| // 이미지가 있는 게시글일 경우 check | ||
| if (!savedImageUuid.isEmpty()) { |
There was a problem hiding this comment.
중첩 if문이 있으면 로직을 파악하는데 어려움이 있습니다.
별도 메소드로 분리하고 행위를 메서드 이름으로 잘 표현하면 좋을것같네요
| // 이미지 정보 저장 | ||
| List<ImageInfoRequest> imageInfoRequests = updateBoard.getImageInfoList(); | ||
| // DB에 이미지 정보 저장 | ||
| if (!imageInfoRequests.isEmpty()) { |
There was a problem hiding this comment.
이건 개개인의 차이긴하지만
저같은 경우는
!imageInfoRequests.isEmpty() 이면
isNotEmpty() 까지 봤다가 맨 앞의 부정문을 봐야해서 읽는 순서가 선형적이지 않아 불편합니다.
그래서 Apache Collections의 써서
CollectionUtils.isNotEmpty(imageInfoRequests) 형태로 표현하곤 합니다.
코틀린 같은 다른언어들은 이게 기본으로 장착되어있는 경우도 있습니다. ㅎㅎ
https://kotlinlang.org/api/latest/jvm/stdlib/kotlin.collections/is-not-empty.html
There was a problem hiding this comment.
오! 이런 좋은 유틸클래스가 있었군요. 적용해보겠습니다!
| @@ -65,23 +67,21 @@ public CommonIdResponse logout(HttpServletRequest request) { | |||
| } | |||
|
|
|||
| // 세션 발급 | |||
| public CommonIdResponse provideSession(Member member, HttpServletRequest request) { | |||
| public CommonIdResponse provideSession(Optional<Member> member, HttpServletRequest request) { | |||
There was a problem hiding this comment.
이부분도 optional 피드백 드린 댓글을 보면 좋을것같네요
| public List<String> checkDeletedImage(List<String> originalUuid) { | ||
| List<String> requestUuid = extractImageUuid(); | ||
| return originalUuid.stream() | ||
| .filter(o -> requestUuid.stream().noneMatch(Predicate.isEqual(o))) |
There was a problem hiding this comment.
stream안에 stream이 있으면 어떤 로직인지 판단하기 어렵습니다
함수로 분리해도 좋을것같네요
No description provided.