[E팀] 백엔드파트 메인서버 코드리뷰용 PR #35
Conversation
| * DATE AUTHOR NOTE | ||
| * ----------------------------------------------------------- | ||
| * 2024.10.17 이 건 최초 생성 | ||
| */ |
There was a problem hiding this comment.
옹 이런건 직접 만드시는건가요?
사실, git을 사용하면, 라인별로 누가 수정했는지 보이기 때문에, 불필요하고 관리포인트가 늘어나는 주석이 될수도 있을거 같습니다.
| private Member member; | ||
|
|
||
| // TickerReview 일대다 양방향 매핑 | ||
| @Builder.Default | ||
| @OneToMany(mappedBy = "clacoBook", orphanRemoval = true, fetch = FetchType.LAZY, cascade = CascadeType.ALL) | ||
| private Set<TicketReview> ticketReviews = new HashSet<>(); | ||
|
|
There was a problem hiding this comment.
clacoBook 도메인에서 member 도메인의 Member과 review 도메인에 정의되어있는 TicketReview를 직접 참조하고 있는것 같은데요.
엔티티를 직접참조 하게되면, 프로젝트가 커졋을때, 다른 도메인의 값을 변경할 수 있어서, 디버깅이 어려워지고, 불필요한 Join이 발생할 수 있어서 피하는게 좋습니다.
직접참조와 간접참조의 장단점이 있는데요. 이런경우에는 간접참조를 하고, 다른 도메인의 서비스에 변경 요청을 하거나 조회가 필요하면 트랜잭션 밖에서 조회해와서 사용하는건 어떨까요?
| * ----------------------------------------------------------- | ||
| * 2024.10.24 이 건 최초 생성 | ||
| */ | ||
| public interface ClacoBookService { |
There was a problem hiding this comment.
이 interface는 무슨 장점이 있을까요?
구현채와 인터페이스가 같은 레이어 위치해있고, 의존성 역전도 필요해보이지 않고, 다른 구현채로 변경할 일도 없어보이는데요. 단순히 코드량만 늘리는 인터페이스가 아닐까요?
추상화는 소스코드 트래킹을 힘들게 하기 때문에, 알맞게 사용하는게 좋습니다.
| // 접근 사용자의 ClacoBook 생성 | ||
| Member member = memberRepository.findMemberByIdWithClacoBook( | ||
| securityContextUtil.getContextMemberInfo().getMemberId()).stream() | ||
| .findAny() | ||
| .orElseThrow(() -> new BusinessException(ApiStatus.MEMBER_NOT_FOUND)); |
There was a problem hiding this comment.
주석보다는 함수를 이용해서 분리하는것은 어떨까요?
주석은 소스코드의 관리포인트를 늘리며, 추후 주석의 내용과 소스코드의 구현이 달라지면 의미없는 (오히려 코드를 읽는데 방해되는) 것이 되기도 합니다.
| @Override | ||
| public List<ClacoBookResponse> readClacoBooks() { | ||
| // 접근 사용자의 ClacoBook 조회 | ||
| Member member = memberRepository.findMemberByIdWithClacoBook( | ||
| securityContextUtil.getContextMemberInfo().getMemberId()).stream() | ||
| .findAny() | ||
| .orElseThrow(() -> new BusinessException(ApiStatus.MEMBER_NOT_FOUND)); | ||
|
|
||
| return member.getClacoBooks().stream() | ||
| .map(ClacoBookResponse::fromEntity) | ||
| .toList(); | ||
| } |
There was a problem hiding this comment.
조회로직만 하는경우에는 @Transactional(readOnly=true) 로 트랜잭션을 열어주시면 성능향상을 할 수 있습니다.
또한, jpa의 기본 구현체에는 트랜잭션 readOnly=true가 붙어있어서 트랜잭션을 열어주지 않아도 자동으로 적용됩니다.
| private List<T> listPageResponse = new ArrayList<>(); | ||
|
|
||
| private Long totalCount; | ||
|
|
||
| private Integer size; |
There was a problem hiding this comment.
필드를 final로 정의할 수 있는경우, final로 만들어 주시는게 좋습니다.
| public ApiResponse<Void> checkNicknameDuplicate(@RequestParam("nickname") String nickname) { | ||
| memberService.checkNicknameValid(nickname); | ||
|
|
||
| return ApiResponse.ok(); |
There was a problem hiding this comment.
@ResponseStatus(HttpStatus.OK) 어노테이션을 붙여주면 성공했을때의 상태코드 응답을 조금 더 명시적으로 표현할 수 있습니다.
| @AllArgsConstructor | ||
| public enum Gender { | ||
|
|
||
| MALE("MALE"), FEMALE("FEMALE"); |
There was a problem hiding this comment.
아래와 같이 트레일링 컴마를 붙이면 추후 코드변경이 발생했을때, 변경되는 라인이 적어서 코드리뷰하기 용이합니다.
| MALE("MALE"), FEMALE("FEMALE"); | |
| MALE("MALE"), | |
| FEMALE("FEMALE"), | |
| ; |
| @Builder | ||
| @AllArgsConstructor | ||
| @NoArgsConstructor(access = AccessLevel.PROTECTED) | ||
| @SQLDelete(sql = "UPDATE member SET active_status = 'DELETED' WHERE member_id = ?") |
There was a problem hiding this comment.
전반적으로 소프트딜리트를 사용하시는거 같은데요. 소프트딜리트는 아래와 같은 단점이 있기 때문에, 적절한 tradeoff를 해주는게 좋습니다.
- 실제로 데이터가 삭제되지 않기 때문에 제약조건 관리가 어려움
- DB에 데이터가 남아있기 때문에, 데이터를 옮겨주지 않으면 데이터가 쌓였을때 성능이 내려갑니다.
- 조회쿼리와 애플리케이션 로직이 복잡해집니다.
| public MemberInfoResponse updateMemberInfo(String updateNickname, MultipartFile updateImage) throws IOException { | ||
| Member member = memberRepository.findById(securityContextUtil.getContextMemberInfo().getMemberId()).stream() | ||
| .findAny() | ||
| .orElseThrow(() -> new BusinessException(ApiStatus.MEMBER_NOT_FOUND)); | ||
|
|
||
| member.updateNickname(updateNickname); | ||
|
|
||
| String profileImageLocation = "member/profile-image/" + member.getId(); | ||
|
|
||
| if (updateImage != null || !updateImage.isEmpty()) { | ||
| String url = s3Util.uploadImage(updateImage, profileImageLocation); | ||
| member.updateProfileImage(url); | ||
| } | ||
|
|
||
| return MemberInfoResponse.fromEntity(member); | ||
| } |
There was a problem hiding this comment.
트랜잭션 안에서 블로킹 I/O작업을 하고 있는것 같은데요. (S3 업로드)
데이터 정합성, 성능 측면에서 좋지 않기 때문에 최대한 지양하는게 좋습니다.
[Feature]#57 feature 추천시스템 로직
[Feature] 유저기반 클라코북 추천
…cookie refactor: refactor refresh
…cookie refactor: 리프레시 토큰 부활
bug: CORS 헤더 명시
[Requirements]: 리뷰 공연 둘러보기 쿼리 변경
feat: 환경 변수에 따른 쿠키 체크
hotfix: 파일 사이즈 설정
[hotfix] 리뷰 공연 둘러보기 전체 보여주기
Feature: 리프레시 쿠키 발급 메서드 추가
hotfix: refresh URI 체크 수정
hotfix: Clacobook 없을경우 예외 처리
hotfix: 쿠키 도메인 추가
hotifx: revert
📌 요약
📝 상세 내용
🗣️ 질문 및 이외 사항
☑️ 누구에게 리뷰를 요청할까요?