-
Notifications
You must be signed in to change notification settings - Fork 0
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
Clone coding#3 클론 코딩 과제 3 #14
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요 ~ 클론코딩 과제를 리뷰하게 된 명예오비 최윤한이라고 합니다..!
쉽지 않은 내용인데도 코드를 잘 작성해주신 것 같습니다 ..~!
저도 부족하지만 궁금한 점이나 아니면 제 생각에 대해서 리뷰 해봤습니다!! 잘 부탁드립니다 ㅎㅎ
궁금한점에 남겨주신 부분도 아래 커멘트로 남기겠습니다.
1. 현재는 나눔 상품과 판매 상품 delete를 같은 url 하나에서 처리하고 있습니다. 이때, 나눔 상품과 판매 상품의 delete 로직을 나눠서 작성하는 것이 좋을지, 따로 작성하는 것이 좋을지 궁금합니다.
-> 해당 질문은 서버 설계와도 관련이 있는 것 같습니다. 현재는 나눔상품과 판매상품을 다른 Entity로 정의하셨기 때문에, service에서 isinstanceof method로 검증을 하고 처리하고 있는 것으로 보입니다..! 따라서 현재는 로직이 크게 복잡하지 않기 때문에 따로 분리하지 않아도 메소드 길이도 크게 안 길어지고 복잡도도 그렇게 높지는 않아보여요. 하지만 이후에 로직이 복잡해지면 각각의 상황마다 달라지는 로직이 그대로 한 개의 API에 다 녹아 들어기 때문에 API의 복잡도가 올라가고 한 개의 API가 너무 많은 역할을 하게 되는 상황이 생길수도 있습니다!
물론 이게 잘못된 것이라고 할 수는 없지만, 저는 기본적으로 API가 단순하면 좋다고 생각해서 그래서 분리해서 개발하는 방법도 고려해봤을 것 같습니다!
아니면 나눔상품과 판매상품을 상품이라는 Entity로 정의하고, 나눔 상품인지 판매 상품인지를 enum으로 만드는 방법을 사용해서 구분하는 방법도 있을 것 같습니다..! 다양하게 고민해보면 좋을 것 같습니다 ~
2. 이미지 유무에 상관없이 상품을 등록하는 로직을 더 좋은 방식으로 작성할 수 있을지 궁금합니다.
- 우선 상품 등록 로직이 크게 복잡하지 않기 때문에, 현재 method 구현이 크게 잘못된 것 없이 잘 작성된 것 같습니다...! 위에서 리뷰에 언급했던 예외 부분만 바꿔봐도 좋을 것 같습니다!
- 이건 로직보다는 네이밍 관련인데 상품 등록 로직을 createSellingProduct라는 method를 통해서 작성하셨는데요! 상품 등록이라는 의미가 더 와닿게 method 명을 register같은 것으로 바꿔봐도 좋을 것 같아요!
@@ -29,6 +29,10 @@ dependencies { | |||
implementation group: 'org.postgresql', name: 'postgresql', version: '42.7.3' | |||
implementation 'org.springframework.boot:spring-boot-starter-data-jpa' | |||
testImplementation 'io.rest-assured:rest-assured' | |||
|
|||
//Multipart file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
//Multipart file 보다는 s3 혹은 aws 라고 표현해도 좋을 것 같습니다 !
return ResponseEntity.status(HttpStatus.CREATED) | ||
.header("Location", productService.createSharingProduct(sharingProductCreateDto)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ResponseEntity.created()
로 한번에 처리할 수 있을 것 같습니다 ~
@@ -32,7 +34,7 @@ public class ProductController { | |||
|
|||
@PostMapping("/selling") | |||
public ResponseEntity<SuccessStatusResponse> createSellingProduct( | |||
@RequestBody SellingProductCreateDto sellingProductCreateDto | |||
@ModelAttribute SellingProductCreateDto sellingProductCreateDto |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RequestPart
어노테이션으로도 요청을 처리할 수 있는데 알아두시면 좋을 것 같습니다 ~
@Bean | ||
public SystemPropertyCredentialsProvider systemPropertyCredentialsProvider() { | ||
System.setProperty(AWS_ACCESS_KEY_ID, accessKey); | ||
System.setProperty(AWS_SECRET_ACCESS_KEY, secretKey); | ||
return SystemPropertyCredentialsProvider.create(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AWS에서 credential을 setting하는 방법이 있는데, 해당 방법 말고도 다른 방법이 어떤 방법이 있는지 알아봐도 좋을 것 같습니다 ~
.bucket(bucketName) | ||
.key(key) | ||
.contentType(image.getContentType()) | ||
.contentDisposition("inline") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
contentDisposition을 inline으로 해두신 이유가 궁금합니다 ~
|
||
private void validateFileSize(MultipartFile image) { | ||
if (image.getSize() > MAX_FILE_SIZE) { | ||
throw new RuntimeException("이미지 사이즈는 5MB를 넘을 수 없습니다."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RuntimeException
을 던지고 있는데, 해당 예외보다 개발하고 있는 프로젝트에서 사용하는 적절한 커스텀 Exception을 정의하여 그 Exception을 RuntimeException
대신에 사용해도 좋을 것 같아요! RuntimeException은 Java API 에서 정의된 예외이기 때문에, 예외 상황 추적이나 의도하지 않은 예외가 발생할 수도 있기 때문입니다 ~!
@@ -28,34 +30,54 @@ public class ProductService { | |||
private final SellingProcuctRepository sellingProductRepository; | |||
private final SharingProductRepository sharingProductRepository; | |||
private final MemberService memberService; | |||
private final S3Service s3Service; | |||
private static final String PRODUCT_S3_UPLOAD_FOLER = "product/"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PRODUCT_S3_UPLOAD_FOLER
오타가 난것 같습니다 ㅎㅎ
sellingProductRepository.save(sellingProduct); | ||
return sellingProduct.getId().toString(); | ||
try { | ||
String imageUrl = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
null로 imageUrl을 선언하고 업데이트 하시는 이유가 있으실까요 ~? 궁금합니다 ㅎㅎ
public void deleteProduct(Long productId){ | ||
Product product = findProductById(productId); | ||
if (product instanceof SellingProduct){ | ||
try { | ||
SellingProduct sellingProduct = (SellingProduct) product; | ||
if (sellingProduct.getImageUrl() != null){ | ||
s3Service.deleteImage(sellingProduct.getImageUrl()); | ||
} | ||
sellingProductRepository.delete(sellingProduct); | ||
} catch (RuntimeException | IOException e){ | ||
throw new RuntimeException((e.getMessage())); | ||
} | ||
} | ||
else if (product instanceof SharingProduct){ | ||
try { | ||
SharingProduct sharingProduct = (SharingProduct) product; | ||
if (sharingProduct.getImageUrl() != null){ | ||
s3Service.deleteImage(sharingProduct.getImageUrl()); | ||
} | ||
sharingProductRepository.delete(sharingProduct); | ||
} catch (RuntimeException | IOException e){ | ||
throw new RuntimeException((e.getMessage())); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
로직은 잘 짜신 것 같은데 메소드내에 중복된 코드들이 많이 보여서, 리팩토링을 통해 조금 더 단순화 시킬 수 있을 것 같아요! private method 사용 등의 방법으로 고민해보셔도 좋을 것 같습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
catch에서 throw new RuntimeException
을 하고 있는데, 위에서 리뷰한 것 처럼 적절한 커스텀 예외를 만들어서 해당 예외를 던져봐도 좋을 것 같습니다!
@@ -10,8 +11,9 @@ public class SellingProductCreateDto extends ProductCreateDto { | |||
|
|||
public SellingProductCreateDto(String title, Long sellerId, Double price, String description, | |||
TransactionPlace transactionPlace, | |||
Boolean negotiable) { | |||
super(title, sellerId, description, transactionPlace); | |||
Boolean negotiable, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
negotiable이란 변수를 Boolean으로 선언하셨는데, 반드시 null을 사용해야 하는 상황이 아니고, true, false를 나타내는 것이 맞다면 primitive type으로 null에 대한 보장을 해줘도 좋을 것 같습니다
이 주의 과제 📕
사진 업로드를 포함한 상품 등록 api
AwsConfig
elive7/cloneCoding/src/main/java/org/sopt/cloneCoding/external/AwsConfig.java
Lines 10 to 48 in cb8b013
S3Service
elive7/cloneCoding/src/main/java/org/sopt/cloneCoding/external/S3Service.java
Lines 15 to 79 in cb8b013
aws의 s3 서비스를 활용하여 사진을 업로드 및 삭제할 수 있도록 위와 같이 코드를 작성해주었습니다.
Product
elive7/cloneCoding/src/main/java/org/sopt/cloneCoding/domain/Product.java
Lines 40 to 49 in 5575b7b
SellingProduct
elive7/cloneCoding/src/main/java/org/sopt/cloneCoding/domain/SellingProduct.java
Lines 17 to 21 in 5575b7b
SharingProduct
elive7/cloneCoding/src/main/java/org/sopt/cloneCoding/domain/SharingProduct.java
Lines 15 to 18 in 5575b7b
SellingProdcuct와 SharingProduct의 부모 클래스인 Product에 imageUrl 속성을 추가하고, 이에 맞게 Product와 SellingProdcuct, SharingProduct의 생성자도 수정했습니다.
ProductController
elive7/cloneCoding/src/main/java/org/sopt/cloneCoding/controller/ProductController.java
Lines 35 to 51 in cb8b013
상품 등록 정보를 request body가 아닌 ModelAttribute로 받도록 수정했습니다.
ProductCreateDto
elive7/cloneCoding/src/main/java/org/sopt/cloneCoding/service/dto/ProductCreateDto.java
Lines 10 to 18 in cb8b013
SellingProductCreateDto
elive7/cloneCoding/src/main/java/org/sopt/cloneCoding/service/dto/SellingProductCreateDto.java
Lines 7 to 20 in cb8b013
SharingProductCreateDto
elive7/cloneCoding/src/main/java/org/sopt/cloneCoding/service/dto/SharingProductCreateDto.java
Lines 7 to 18 in cb8b013
ProductCreateDto에 MultipartFile image 속성을 추가하고 그게 맞게 SellingProductCreateDto와 SharingProductCreateDto도 수정해주었습니다.
ProductService
elive7/cloneCoding/src/main/java/org/sopt/cloneCoding/service/ProductService.java
Lines 36 to 81 in 3a6387b
sellingProductCreateDto.getImage()를 확인해 null이 아니라면, s3에 해당 이미지를 업로드하고, sellingProduct에 imageUrl를 세팅하여 저장하고, null이라면 sellingProduct에 imageUrl로 null을 세팅하여 저장해주었습니다. sharingProduct 등록도 같은 로직을 적용했습니다.
사진 업로드를 포함한 상품 삭제 api
ProductController
elive7/cloneCoding/src/main/java/org/sopt/cloneCoding/controller/ProductController.java
Lines 63 to 67 in 66aca06
PathVariable로 productId를 받아서 상품을 삭제하는 로직을 실행할 수 있도록 다음과 같이 코드를 작성해주었습니다. 삭제시 noContent 204를 반환하도록 해주었습니다.
ProductService
elive7/cloneCoding/src/main/java/org/sopt/cloneCoding/service/ProductService.java
Lines 93 to 117 in 66aca06
해당 product가 SellingProduct/SharingProduct 중 어느 타입인지 확인해, 맞는 타입으로 다운캐스팅 한 후, imageUrl에 값이 있다면 s3의 사진을 지우는 로직을 수행한 후 실제 판매상품/나눔상품도 삭제하는 로직을 작성했습니다.
요구사항 분석 📙
구현 고민 사항 📗
질문있어요! 📘
API 명세서 📔
https://regular-cow-aa9.notion.site/6-API-4dd4004677a04e259e2658a8d3988a24?pvs=4