Feature/#2 map letter 기능 구현 #27
Conversation
|
|
||
| Mockito.verify(paperRepository, Mockito.times(1)).findAll(); | ||
| } | ||
| } |
| private String label; | ||
| private String content; | ||
| private boolean isBlocked; | ||
| private boolean isDeleted; |
There was a problem hiding this comment.
isDeleted로 삭제됐는지 확인하는 방식으로 soft delete를 적용하는 건가요?? (안 해봐서 잘 모름)
There was a problem hiding this comment.
넹 일단 그렇게 했어요(저도 안해봐서 잘 모름) 그래서 편지 불러올 때 isDeleted가 false인것만 불러오고 그래요!!
| ReplyMapLetterEntity letter = replyMapLetterJpaRepository.findById(letterId) | ||
| .orElseThrow(() -> new MapLetterNotFoundException("해당 편지를 찾을 수 없습니다.")); | ||
|
|
||
| ReplyMapLetterEntity replyMapLetter = em.find(ReplyMapLetterEntity.class, letterId); |
There was a problem hiding this comment.
EntityManager에서 가져오는 이유가 궁금해요!!
There was a problem hiding this comment.
아래에서 updateDelete true로 해서 변경 감지로 수정하려고 em 사용했습니닷
| if (mapLetter.getType() == MapLetterType.PRIVATE && (!mapLetter.getTargetUserId().equals(userId) | ||
| && !mapLetter.getCreateUserId().equals(userId))) { | ||
| throw new CommonForbiddenException("편지를 볼 수 있는 권한이 없습니다."); | ||
| } |
There was a problem hiding this comment.
이 부분을 MapLetter 자체가 검증해도될 것 같아요!
if (mapLetter.isPrivate() && mapLetter.isTarget(userId)) {}이런식으로 Service에서 MapLetter의 필드를 가져와서 검증하는 것보다 MapLetter에게 맡기는 것도 좋은 것 같습니다! 아니면 mapLetter.validate~~()식의 메서드 내에서 바로 예외를 던져도 될 듯 해요!
There was a problem hiding this comment.
나중에 바꿔볼게요~(이해 못함)
| OneLetterResponseDTO oneLetterResponseDTO = MapLetter.toOneLetterResponse(mapLetter); | ||
| String profileImg = ""; //user 서비스 메서드 불러서 받기 | ||
| return OneLetterResponseDTO.builder() |
There was a problem hiding this comment.
MapLetter의 toOneLetterResponse 메서드가 MapLetter를 DTO로 변환해주는 정적 메서드라면, 어차피 자기 자신이 DTO로 변하는 거니까 정적 메서드로 안 괜찮을 것 같아요! 그리고 로직 상 만들고 이미지 가져온다음 다시만드는 것보다, 처음 만들 때 이미지를 가져온걸 가지고 만드는 건 어떨까요?
| if (!findMapLetter.getCreateUserId().equals(userId)) { | ||
| throw new CommonForbiddenException("편지를 삭제 할 권한이 없습니다. 편지 삭제에 실패하였습니다."); | ||
| } |
There was a problem hiding this comment.
이 부분도 MapLetter에게 작성자인지 검증하는 책임을 맡겨도 괜찮을 것 같아요!
| List<MapLetterAndDistance> letters = mapLetterRepository.findLettersByUserLocation(latitude, longitude, userId); | ||
|
|
||
| return letters.stream() | ||
| .map(letterWithDistance -> FindNearbyLettersResponseDTO.builder() |
There was a problem hiding this comment.
FindNearbyLettersResponseDTO.from(letterWithDistance)로 분리하는건 어떨까요?
| MapLetterEntity letter = mapLetterJpaRepository.findById(letterId) | ||
| .orElseThrow(() -> new MapLetterNotFoundException("해당 편지를 찾을 수 없습니다.")); | ||
|
|
||
| MapLetterEntity mapLetter = em.find(MapLetterEntity.class, letterId); |
There was a problem hiding this comment.
위에서 findById를 통해서 가져온 객체를 두 번 조회하게 되는게 아닐까요??
| public ApiResponse<?> createTargetLetter( | ||
| @Valid @RequestBody CreateTargetMapLetterRequestDTO createTargetMapLetterRequestDTO, | ||
| BindingResult bindingResult, Long userId) { | ||
| if (bindingResult.hasErrors()) { |
There was a problem hiding this comment.
어차피 bindingResult를 넘기니까
위의 처리랑 묶어서 메서드 처리 가능할거 같아요
validateMapLetterRequest(bindingResult) {
if(bindingResult.hasError()) {
bindingResult.getFiedlErrors().forEach(error -> {
switch(error.getField()) {
case "title":
thorw~
default:
throw~ Ilegal~
}
});
}
}
| public ApiResponse<?> createReplyMapLetter( | ||
| @Valid @RequestBody CreateReplyMapLetterRequestDTO createReplyMapLetterRequestDTO, | ||
| BindingResult bindingResult, Long userId) { | ||
| if (bindingResult.hasErrors()) { |
There was a problem hiding this comment.
위의 메서드를 쓰면 이것도 줄일 수 있을 것 같아요
| Long paperId; | ||
| String paperUrl; | ||
|
|
||
| public static PaperDTO toPaperDTO(Paper paper) { |
There was a problem hiding this comment.
저는 이 부분을 DTO에 from으로 넣어서 스스로 DTO를 만들도록 했는데 어떤 방식이 좋은지 몰라서
의견 나눠보고 싶어요!
| private LocalDateTime createdAt; | ||
| private LocalDateTime updatedAt; | ||
|
|
||
| public static ReplyMapLetter createReplyMapLetter(CreateReplyMapLetterRequestDTO createReplyMapLetterRequestDTO, |
There was a problem hiding this comment.
마찬가지로 약간 저는 최대한 DTO 관련한 메서드를 넣지말자 생각해서 RequestDTO가 Domain을 생성하도록 만들었는데
이게 좋다가 아니라 어떤게 더 나을지 의견 나눠보고 싶습니다!
There was a problem hiding this comment.
도메인 로직에 서비스 로직을 넣어야 한다고 생각해서 이렇게 하기는 했는데... 사실 잘 모르겠습니다ㅎㅎ
| } | ||
|
|
||
| @Override | ||
| public void softDelete(Long letterId) { |
There was a problem hiding this comment.
저도 softDelet를 구현해야하니까 생각했던건 update 쿼리를 jpa에 작성해서 id에 대한 deleted를 true로 바꿔주는건데
보통 softDelete 처리를 할 때 해당 방식대로 하는건지 궁금합니다
전에 팀원분이 하신거 보니까 Entity에 @SQLDelete를 붙여서 delete처리가 해당 어노테이션에 들어간 쿼리로 돌아가게끔 하는 방식도 있는거 같아요
There was a problem hiding this comment.
오... 저는 변경감지로 updateDelete 이거 해서 수정했습니닷!
|
|
||
| OneLetterResponseDTO oneLetterResponseDTO = MapLetter.toOneLetterResponse(mapLetter); | ||
| String profileImg = ""; //user 서비스 메서드 불러서 받기 | ||
| return OneLetterResponseDTO.builder() |
There was a problem hiding this comment.
여기부터 아래 있는 builder들 각 DTO에 from으로 위임해주는거 어떨까요
public static OneLetterResponseDTO from(mapLetter) {
return new OneLetterResponseDTO(mapletter~~~);
}
그러면 이 부분이
return OneLetterResponseDTO.from(mapLetter)
가 될거 같아요
| .senderNickname(senderNickname) | ||
| .type("target") | ||
| .build(); | ||
| result.add(dto); |
There was a problem hiding this comment.
메서드가 반환해주는 역할이니까 result.add는 이걸 받는 메서드에서 List 반환받고 저장하는게 나을거 같아요!
| @Transactional(readOnly = true) | ||
| public List<FindAllReplyMapLettersResponseDTO> findAllReplyMapLetter(Long letterId, Long userId) { | ||
| MapLetter sourceLetter = mapLetterRepository.findById(letterId); | ||
| if (!sourceLetter.getCreateUserId().equals(userId)) { |
There was a problem hiding this comment.
검증들도 분리할 방법이 있으면 분리하면 좋지 않을까 생각이 들었습니다!
|
📢 기능 설명
연결된 issue
연결된 issue를 자동을 닫기 위해 아래 {이슈넘버}를 입력해주세요.
close #2
✅ 체크리스트
ETC
천천히 봐주세요...ㅎㅎ 그리고 그 트랜잭션 범위가 너무 헷갈려요;; 일단 1차로 구현했고 기능 리팩토링 하겠습니닷