Skip to content
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

Develop #96

Merged
merged 4 commits into from
Jan 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion server/sql/231224_1450.sql
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
ALTER TABLE alcohol_type
ADD `display_order` bigint DEFAULT NULL
ADD `display_order` int DEFAULT NULL
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 코드 패치는 alcohol_type 테이블에 display_order라는 열을 추가하는 ALTER TABLE 문입니다. 변경된 부분은 데이터 형식입니다. bigint 대신 int로 변경되었습니다.

이러한 코드 리뷰에서 주의해야 할 사항은 다음과 같습니다:

  • 데이터베이스 구조와 관련된 변경은 신중히 검토해야 합니다. 이 변경이 다른 테이블 또는 쿼리에 영향을 줄 수 있는지 확인해야 합니다.
  • 기존 코드에 영향을 주는 변경일 경우, 변경된 코드를 사용하는 모든 위치를 확인하여 호환성을 검증해야 합니다.
  • 데이터 형식 변경 시 데이터 손실이 발생할 수 있으므로, 데이터베이스에 저장된 값들의 유효성을 검사하는 것이 중요합니다.
  • 코드에 새로운 버그가 없는지 확인하기 위해 테스트를 진행해야 합니다.

개선 제안:

  • 코드 스타일을 맞추기 위해 각 줄의 들여쓰기를 확인하고 일관된 스타일을 사용하세요.
  • "no newline at end of file" 경고를 해결하기 위해 파일의 끝에 새로운 줄을 추가해주세요.

위의 정보를 참고하여 코드 패치를 검토해보세요.

Original file line number Diff line number Diff line change
@@ -1,17 +1,24 @@
package com.bside.bside_311.component;

import com.bside.bside_311.dto.AlcoholSearchCondition;
import com.bside.bside_311.entity.Alcohol;
import com.bside.bside_311.entity.AlcoholType;
import com.bside.bside_311.entity.Post;
import com.bside.bside_311.entity.PostAlcohol;
import com.bside.bside_311.entity.YesOrNo;
import com.bside.bside_311.repository.AlcoholRepository;
import com.bside.bside_311.repository.AlcoholTypeRepository;
import lombok.RequiredArgsConstructor;
import org.apache.commons.lang3.ObjectUtils;
import org.springframework.data.domain.Page;
import org.springframework.data.domain.Pageable;
import org.springframework.stereotype.Component;

@Component
@RequiredArgsConstructor
public class AlcoholManager {
private final AlcoholRepository alcoholRepository;
private final AlcoholTypeRepository alcoholTypeRepository;

public void connectAlcoholWithPost(Long alcoholNo, String alcoholFeature, Post post) {
Alcohol alcohol = alcoholRepository.findByIdAndDelYnIs(alcoholNo, YesOrNo.N).orElseThrow(
Expand All @@ -20,4 +27,22 @@ public void connectAlcoholWithPost(Long alcoholNo, String alcoholFeature, Post p
post.addPostAlcohol(postAlcohol);
alcohol.addPostAlcohol(postAlcohol);
}

public Page<Alcohol> searchAlcohol(Pageable pageable, String searchKeyword,
Long alcoholType) {
if (ObjectUtils.isNotEmpty(alcoholType)) {
AlcoholType alcoholType1 = alcoholTypeRepository.findByIdAndDelYnIs(alcoholType, YesOrNo.N)
.orElseThrow(
() -> new IllegalArgumentException(
"술 종류가 존재하지 않습니다."));
}
// 술 종류 fetch join
return alcoholRepository.searchAlcoholPage(AlcoholSearchCondition.builder()
.searchKeyword(
searchKeyword)
.alcoholType(
alcoholType)
.build(),
pageable);
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

위의 코드 패치를 간단히 검토해 드리겠습니다.

  1. AlcoholManager 클래스는 AlcoholRepositoryAlcoholTypeRepository에 대한 주입을 받도록 되어 있습니다.
  2. connectAlcoholWithPost 메서드는 알코올 번호(alcoholNo), 알코올 특징(alcoholFeature), 게시물(post)을 매개변수로 받아 알코올과 게시물을 연결하는 기능을 수행합니다. 주어진 알코올 번호를 사용하여 Alcohol 객체를 찾고, 해당 게시물에 PostAlcohol을 추가하고, 알코올에도 PostAlcohol을 추가합니다. 이 부분은 문제가 없어 보입니다.
  3. searchAlcohol 메서드는 페이지네이션(pageable), 검색 키워드(searchKeyword), 알코올 유형(alcoholType)을 매개변수로 받아 알코올을 검색하는 기능을 수행합니다. 알코올 유형이 주어진 경우 해당 유형이 유효한지 확인하기 위해 AlcoholTypeRepository를 사용하는데, 유효하지 않은 알코올 유형인 경우 IllegalArgumentException을 던지고 있습니다. 이 부분은 문제가 없어 보입니다. 그러나, 알코올 유형에 따라 Alcohol을 검색하는 부분이 누락되어 있습니다. 해당 부분을 확인하고 추가해야 합니다.

개선 제안:

  • searchAlcohol 메서드에서 alcoholType1 변수를 사용하지 않고, 알코올 유형이 유효한 경우 해당 유형에 따라 Alcohol을 검색하도록 수정해야 합니다.
  • searchAlcohol 메서드에서 검색 조건(AlcoholSearchCondition)을 생성할 때, searchKeywordalcoholType을 사용하여 조건을 설정하도록 해야 합니다.

수정된 searchAlcohol 메서드의 예시:

public Page<Alcohol> searchAlcohol(Pageable pageable, String searchKeyword, Long alcoholType) {
  if (ObjectUtils.isNotEmpty(alcoholType)) {
    AlcoholType alcoholTypeEntity = alcoholTypeRepository.findByIdAndDelYnIs(alcoholType, YesOrNo.N)
                                                        .orElseThrow(() -> new IllegalArgumentException("술 종류가 존재하지 않습니다."));

    // 알코올 유형이 유효한 경우 해당 유형에 따라 검색결과 반환
    return alcoholRepository.searchAlcoholPage(AlcoholSearchCondition.builder()
                                                                     .searchKeyword(searchKeyword)
                                                                     .alcoholType(alcoholTypeEntity)
                                                                     .build(),
                                                pageable);
  } else {
    // 알코올 유형이 주어지지 않은 경우 모든 알코올 검색결과 반환
    return alcoholRepository.searchAlcoholPage(AlcoholSearchCondition.builder()
                                                                     .searchKeyword(searchKeyword)
                                                                     .build(),
                                                pageable);
  }
}

추가적으로, 코드의 다른 부분은 검토 결과 문제를 발견하지 못했습니다. 이 내용을 바탕으로 코드 패치를 수정하실 수 있을 것입니다.

Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package com.bside.bside_311.component;

import com.bside.bside_311.dto.AttachDto;
import com.bside.bside_311.entity.Attach;
import com.bside.bside_311.entity.AttachType;
import com.bside.bside_311.entity.YesOrNo;
Expand All @@ -8,7 +9,10 @@
import lombok.RequiredArgsConstructor;
import org.springframework.stereotype.Component;

import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;

import static com.bside.bside_311.util.ValidateUtil.resourceChangeableCheckByThisRequestToken;

Expand Down Expand Up @@ -50,4 +54,32 @@ public void deleteAttachesByRefNoAndAttachType(Long refNo, AttachType attachType
imageStorage.delete(attach.getPublicId());
}
}

public Map<Long, List<AttachDto>> getAttachInfoMapBykeysAndType(List<Long> keys,
AttachType attachType) {
List<Attach> attachList =
attachRepository.findByRefNoInAndAttachTypeIsAndDelYnIs(keys, attachType,
YesOrNo.N);
Map<Long, List<AttachDto>> kToAMap = new HashMap<>();
for (Attach attach : attachList) {
if (!kToAMap.containsKey(attach.getRefNo())) {
kToAMap.put(attach.getRefNo(), new ArrayList<>());
}
List<AttachDto> attachDtos = kToAMap.get(attach.getRefNo());
attachDtos.add(AttachDto.of(attach));
}
return kToAMap;
}

public List<AttachDto> getAttachListBykeyAndType(Long key,
AttachType attachType) {
List<Attach> attachList =
attachRepository.findByRefNoInAndAttachTypeIsAndDelYnIs(List.of(key), attachType,
YesOrNo.N);
List<AttachDto> attachDtos = new ArrayList<>();
for (Attach attach : attachList) {
attachDtos.add(AttachDto.of(attach));
}
return attachDtos;
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

다음은 코드 패치입니다. 버그 위험이나 개선 제안 사항을 알려주세요:

@@ -1,5 +1,6 @@
 package com.bside.bside_311.component;

+import com.bside.bside_311.dto.AttachDto;
 import com.bside.bside_311.entity.Attach;
 import com.bside.bside_311.entity.AttachType;
 import com.bside.bside_311.entity.YesOrNo;
@@ -8,7 +9,10 @@
 import lombok.RequiredArgsConstructor;
 import org.springframework.stereotype.Component;

+import java.util.ArrayList;
+import java.util.HashMap;
 import java.util.List;
+import java.util.Map;

 import static com.bside.bside_311.util.ValidateUtil.resourceChangeableCheckByThisRequestToken;

@@ -50,4 +54,32 @@ public void deleteAttachesByRefNoAndAttachType(Long refNo, AttachType attachType
       imageStorage.delete(attach.getPublicId());
     }
   }

+  public Map<Long, List<AttachDto>> getAttachInfoMapBykeysAndType(List<Long> keys,
+                                                                  AttachType attachType) {
+    List<Attach> attachList =
+        attachRepository.findByRefNoInAndAttachTypeIsAndDelYnIs(keys, attachType,
+            YesOrNo.N);
+    Map<Long, List<AttachDto>> kToAMap = new HashMap<>();
+    for (Attach attach : attachList) {
+      if (!kToAMap.containsKey(attach.getRefNo())) {
+        kToAMap.put(attach.getRefNo(), new ArrayList<>());
+      }
+      List<AttachDto> attachDtos = kToAMap.get(attach.getRefNo());
+      attachDtos.add(AttachDto.of(attach));
+    }
+    return kToAMap;
+  }
+
+  public List<AttachDto> getAttachListBykeyAndType(Long key,
+                                                   AttachType attachType) {
+    List<Attach> attachList =
+        attachRepository.findByRefNoInAndAttachTypeIsAndDelYnIs(List.of(key), attachType,
+            YesOrNo.N);
+    List<AttachDto> attachDtos = new ArrayList<>();
+    for (Attach attach : attachList) {
+      attachDtos.add(AttachDto.of(attach));
+    }
+    return attachDtos;
+  }
 }

이 코드 패치에는 다음과 같은 기능이 포함되어 있습니다:

  1. com.bside.bside_311.dto.AttachDto를 import 합니다.
  2. getAttachInfoMapBykeysAndType 메서드를 추가합니다. 이 메서드는 keysattachType을 기반으로 Attach를 찾고, 해당하는 AttachDto 리스트를 키-값 쌍의 맵 형태로 반환합니다.
  3. getAttachListBykeyAndType 메서드를 추가합니다. 이 메서드는 keyattachType을 기반으로 Attach를 찾고, 해당하는 AttachDto 리스트를 반환합니다.

개선 제안:

  • 사용하지 않는 import 문을 정리하세요 (java.util.ArrayList, java.util.HashMap).
  • getAttachInfoMapBykeysAndTypegetAttachListBykeyAndType 메서드 내에서 AttachDto.of(attach) 대신 생성자를 직접 호출하는 것이 코드를 더 명확하게 만들 수 있습니다.

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
package com.bside.bside_311.component;

import com.bside.bside_311.dto.GetPostsToOneMvo;
import com.bside.bside_311.dto.PostSearchCondition;
import com.bside.bside_311.entity.Post;
import com.bside.bside_311.entity.YesOrNo;
import com.bside.bside_311.repository.PostMybatisRepository;
import com.bside.bside_311.repository.PostRepository;
import com.bside.bside_311.util.AuthUtil;
import com.bside.bside_311.util.MessageUtil;
import lombok.RequiredArgsConstructor;
import org.apache.commons.collections4.CollectionUtils;
import org.springframework.data.domain.Page;
import org.springframework.data.domain.Pageable;
import org.springframework.stereotype.Component;

import java.util.HashMap;
import java.util.List;
import java.util.Map;

@Component
@RequiredArgsConstructor
public class PostService {
private final PostRepository postRepository;
private final PostMybatisRepository postMybatisRepository;

public void savePost(Post post) {
postRepository.save(post);
}

public Post findPost(Long postNo) {
return postRepository.findByIdAndDelYnIs(postNo, YesOrNo.N).orElseThrow(
() -> new IllegalArgumentException(MessageUtil.POST_NOT_FOUND_MSG));
}

public void deletePost(Post post) {
post.setDelYn(YesOrNo.Y);
postRepository.save(post);
}

public Page<Post> getPostListCommon(Pageable pageable, String searchKeyword,
List<Long> searchUserNoList,
Boolean isLikedByMe, Boolean isCommentedByMe,
List<Long> searchAlcoholNoList) {
return postRepository.searchPageSimple(
PostSearchCondition.builder().searchKeyword(searchKeyword)
.searchUserNoList(searchUserNoList)
.isLikedByMe(isLikedByMe)
.isCommentedByMe(isCommentedByMe)
.myUserNo(
AuthUtil.getUserNoFromAuthentication())
.searchAlcoholNoList(searchAlcoholNoList)
.build(), pageable);
}

public Page<Post> getPostListPopular(Long page, Long size) {
return postRepository.searchPagePopular(page, size);
}

public Map<Long, GetPostsToOneMvo> getGetPostsToOneMvoMap(List<Long> postNos) {
List<GetPostsToOneMvo> postsToOneList =
CollectionUtils.isNotEmpty(postNos) ? postMybatisRepository.getPostsToOne(postNos) :
List.of();
Map<Long, GetPostsToOneMvo> postsToOneMap = new HashMap<>();
for (GetPostsToOneMvo getPostsToOneMvo : postsToOneList) {
postsToOneMap.put(getPostsToOneMvo.getPostNo(), getPostsToOneMvo);
}
return postsToOneMap;
}


}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 코드 패치는 PostService 클래스에 대한 변경 사항입니다. 아래는 코드 리뷰입니다:

  • PostService 클래스는 Component 애너테이션이 지정되어 있습니다.
  • RequiredArgsConstructor 애너테이션을 사용하여 필요한 의존성 주입을 자동으로 생성합니다.
  • PostRepositoryPostMybatisRepository 인터페이스를 사용하여 데이터 액세스 작업을 수행합니다.
  • savePost, findPost, deletePost 메서드는 각각 게시물을 저장, 조회, 삭제하는 기능을 합니다.
  • getPostListCommon 메서드는 페이징 및 검색 조건으로 게시물 목록을 반환합니다.
  • getPostListPopular 메서드는 인기 게시물 목록을 반환합니다.
  • getGetPostsToOneMvoMap 메서드는 게시물 번호 목록을 기반으로 게시물과 관련된 추가 정보를 가져옵니다.

개선 제안:

  • 누락된 주석: 코드의 목적과 각 메서드의 역할에 대한 주석을 추가하는 것이 좋습니다.
  • 예외 처리: findPost 메서드에서 게시물이 없는 경우 IllegalArgumentException을 던지고 있습니다. 이 예외의 메시지도 확인해보는 것이 좋습니다.
  • 네이밍: 메서드 이름과 변수 이름은 직관적이고 명확하게 작성되어 있는 것 같습니다. 변수의 유형과 용도에 대한 추가 설명을 제공할 수 있으면 좋습니다.

잠재적인 버그 위험 요소나 이외의 개선 사항은 제시된 코드에서 확인하기 어렵습니다. 전체 애플리케이션의 아키텍처와 다른 모듈들과의 상호 작용을 고려하여 추가적인 코드 리뷰가 필요할 것입니다.

Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
package com.bside.bside_311.component;

import com.bside.bside_311.entity.User;
import com.bside.bside_311.entity.YesOrNo;
import com.bside.bside_311.repository.UserRepository;
import com.bside.bside_311.util.MessageUtil;
import lombok.RequiredArgsConstructor;
import lombok.extern.slf4j.Slf4j;
import org.springframework.stereotype.Component;
import org.springframework.util.ObjectUtils;

@Slf4j
@Component
@RequiredArgsConstructor
public class UserManager {
private final UserRepository userRepository;

public User getUser(Long userNo) {
if (ObjectUtils.isEmpty(userNo)) {
return null;
}
return userRepository.findByIdAndDelYnIs(userNo, YesOrNo.N).orElseThrow(
() -> new IllegalArgumentException(MessageUtil.USER_NOT_FOUND_MSG));
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

코드 리뷰를 진행하겠습니다:

  1. UserManagercom.bside.bside_311.component 패키지에 속하는 컴포넌트입니다.
  2. 필요한 의존성 주입을 위해 lombok.RequiredArgsConstructor를 사용합니다.
  3. getUser 메서드는 userNo 매개변수를 받아 해당 userNo에 해당하는 User 객체를 찾아 반환합니다.
  4. 만약 userNo가 비어있다면, null을 반환합니다.
  5. userRepository에서 findByIdAndDelYnIs 메서드를 호출하여 userNo와 삭제 여부(YesOrNo.N)로 User를 찾고 없을 경우 예외를 던집니다.
  6. 예외 처리 시 IllegalArgumentException이 발생하며, 해당 예외에 대한 메시지는 MessageUtil.USER_NOT_FOUND_MSG로 정의되어 있습니다.

개선 제안:

  1. ObjectUtils.isEmpty(userNo)를 대신하여 userNo == null을 사용할 수 있습니다.
  2. 예외 처리의 내용에 대해 좀 더 구체적인 설명을 추가할 수 있습니다.

위 내용은 단순한 코드 리뷰입니다. 실제 애플리케이션에 적합한지는 전제 조건과 요구사항에 따라 다를 수 있습니다.

Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import io.swagger.v3.oas.annotations.media.Schema;
import io.swagger.v3.oas.annotations.tags.Tag;
import jakarta.validation.Valid;
import jakarta.validation.constraints.Positive;
import lombok.RequiredArgsConstructor;
import lombok.extern.slf4j.Slf4j;
import org.springframework.data.domain.Page;
Expand Down Expand Up @@ -102,9 +103,13 @@ public Page<AlcoholResponseDto> getAlcoholV2(
Pageable pageable,
@RequestParam(required = false, name = "searchKeyword")
@Schema(description = "키워드", example = "키워드")
String searchKeyword) {
String searchKeyword,
@RequestParam(required = false, name = "alcoholType")
@Schema(description = "술 타입(선택)", example = "1")
@Positive(message = "술 타입은 1이상의 숫자만 가능합니다.")
Long alcoholType) {
log.info(">>> AlcoholController.getAlcohol");
return alcoholService.getAlcoholV2(pageable, searchKeyword);
return alcoholService.getAlcoholV2(pageable, searchKeyword, alcoholType);
}

@Operation(summary = "[o]술 상세 조회", description = "술 상세 조회 API")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 코드 패치는 다음과 같은 변경 사항을 포함하고 있습니다:

  1. jakarta.validation.constraints.Positive 어노테이션이 추가되었습니다.
  2. alcoholType 파라미터가 추가되었으며, @RequestParam@Schema 어노테이션으로 정의되었습니다.
  3. alcoholType 파라미터에 대한 유효성 검사를 위해 @Positive 어노테이션을 사용하고 있습니다.

개선 및 버그 위험 사항에 대한 제안:

  1. 코드의 가독성을 높이기 위해 주석을 영문으로 작성하는 것이 좋습니다.
  2. String searchKeywordLong alcoholType 파라미터들의 이름을 좀 더 명확하게 지정하는 것이 좋습니다. 이러한 이름들은 개발자들이 해당 메소드를 사용할 때 도움이 될 수 있습니다.
  3. 코드에서 AlcoholController.getAlcohol 로깅 메시지를 ">>> AlcoholController.getAlcohol" 에서 보다 명확한 메시지로 변경하는 것이 좋습니다.
  4. 나머지 코드의 내용과 로직에 대해서는 별다른 문제나 버그 위험이 보이지 않습니다.

위의 사항들을 고려하여 코드를 개선할 수 있습니다.

Expand Down
Loading