[feat] 클론코딩 3번째!

[rlarlgnszx]

• closes [feat] 클론코딩 3번째! #18 (여기에는 이슈 넘버를 넣어주세요!)

이 주의 과제

1. 상품을 등록할 떄 사진을 함께 업로드할 수 있게 구현하기

2. 등록한 상품을 지우는 로직도 구현해줄게요! (deleteImage 활용)

• 상품을 등록할 떄 사진을 함께 업로드할 수 있게 구현하기
• 상품 지우는 로직 추가

요구사항 분석

• image 엔티티 추가 -> Blog랑 OneToOne 연결
  

  rlarlgnszx/carrot/src/main/java/server/sopt/carrot/entity/Image.java

  Lines 8 to 26 in 8e2dce2

  	@Entity
  	@Getter
  	@NoArgsConstructor
  	public class Image {
  	@Id
  	@GeneratedValue(strategy = GenerationType.IDENTITY)
  	private Long id;
  	private String imageUrl;
  	@OneToOne
  	Product product;
  	@Builder
  	public Image(String imageUrl, Product product) {
  	this.imageUrl = imageUrl;
  	this.product = product;
  	}
  	}

• Product Service Create 구현
  

  rlarlgnszx/carrot/src/main/java/server/sopt/carrot/service/ProductServiceImpl.java

  Lines 61 to 67 in 8e2dce2

  	productRepository.save(product);
  	String imageUrl = s3Service.uploadImage(BLOG_S3_UPLOAD_FOLER, req.getImage());
  	Image image = Image.builder()
  	.imageUrl(imageUrl)
  	.product(product).build();
  	imageRepository.save(image);
  	return ProductCreate.Response.fromEntity(product);

• Product Service Delete 구현
• Product아이디와 연결된 이미지 찾아서 삭제
  

  rlarlgnszx/carrot/src/main/java/server/sopt/carrot/service/ProductServiceImpl.java

  Lines 122 to 129 in 8e2dce2

  	@Override
  	public ResponseEntity<Void> deleteProduct(Long productId) {
  	Image image= imageRepository.findByProduct_Id(productId).orElseThrow(
  	() -> new BusinessException(ErrorMessage.PRODUCT_NOT_FOUND)
  	);
  	imageRepository.delete(image);
  	return ResponseEntity.ok().build();
  	}

구현 고민 사항

Product에 ImageUrl을 넣어서 Put이나 Patch로 구현 vs.Image엔티티를 따로 빼서 다른 도메인으로 구현해서 연관관계 설정 후 Post, Delete 메소드로 구현

• 왜 나는 엔티티를 빼서 구현을 했는가?
  • 확장된 후에 연결된 Image들을 찾을때 Image쪽을 Many로 구현해서 제거하고 싶은 로직을 구현하고 싶었습니다..또한 이전에 상품 스크랩하는 API를 만들었을때에 Post,Delete로 구현을 해서 이미지또한 비슷하게 Post,Delete로 구현하고 싶었습니다.

[jinsu4755]

안녕하세요 이번에 리뷰어로 참여하게된 명예OB 박진수입니다! 회사 일정이 바빠서 리뷰가 늦어졌습니다...죄송합니다.

코드리뷰에서 사용된 단계는 다음과 같습니다.

p0 : 최고 우선 순위, 빠르게 고쳐야할 치명적인 버그나 사이드 이팩트가 예상됨. 혹은 요구사항을 만족하지 못하는 경우
p1: 함수 혹은 객체가 요구사항을 만족 하는데 충분히 명확하지 않은 경우. 불필요한 코드 작성이 반복되거나 복잡도가 높은 경우, 일관성을 해치는 경우
p5~: 네이밍 부터 자잘한 수정 사항

구현 고민사항을 읽어보니 단순하게 image 를 상품에 추가하는 것 뿐 아니라 앞으로 확장성 까지 고려하신 것 같아서 많은 고민을 해주신것을 알 수 있었습니다.

실제로 Product 를 수정하거나 하는 상황에서 사진을 교체하거나 삭제하는 일이 실제로 발생할 수 있으니까요 ㅎㅎ 추가적으로 단순하게 추가, 삭제, 수정 뿐 아니라 만약 사용자가 사진의 순서까지 지정하고 싶다면 어떻게 해야할까? 고민해보시는 것도 좋을 것 같아요!

클라이언트에서는 사진의 순서를 정해서 보여주고 그 순서 역시 편집할 수 있어요.
데이터를 받을 때는 그 순서를 유지할 수 있지만 현재와 같이 연관 관계인 경우 어떻게 정렬하거나 처리해야 데이터를 내려줄 때 역시 그 순서를 유지할 수 있을까요?
그냥 간단히 생각했을때 postgrsql 을 사용하기 때문에 단순하게 DB 의 Array 타입을 이용한다거나, 혹은 일대다 관계에서 테이블에 어떤 조치를 취한다거나. 방법은 많겠지만
다양한 방식에서 각 방식 마다 어떤 장단점이 있는지 분석하고 도입해보시는 것도 좋을 것 같아요!

제게 조언 해주셨던 14년차 개발자 분 및 많은 시니어분들의 이야기를 종합해보자면
결국 어떤 고민을 하고 이를 해결하고자 할 때, 뭐든 다 될 것 같지만..내 생각의 편리함, 구현의 편리함 등 편리함에만 집중하면 본질이 흐려지기 때문에 공식문서나 베스트 케이스 등에서는 어떤 방식으로 처리하는지, 그게 내 답과 어떤 차이가 있고 만약 내 답이 좋다면 어느정도의 성능 향상이나 이점이 있는지 검토해보면 좋을 것 같아요.

또한 제가 경험했던 것을 바탕으로 혹은 실무에서 본것을 바탕으로 리뷰를 남겨드리고자 노력했지만, 기훈님이 리뷰를 읽어보시고 기훈님이 맞다고 생각하시는 부분이 있다면 제 리뷰에 반박해서 저를 설득해보시는 것도 좋을 것 같아요.

리뷰는 결국 코드를 보고 상호 소통하는 것이기 때문에 이런 소통이 더 나은 결과물을 만들 수 있어요. 또한 요구사항을 만족한다면 코드 작성 방법에는 절대적인 정답은 없다고 생각해요. 어떤 상황에서 A가 B를 설득한다면 그 상황에 맞는 방법은 A의 방법이 되는 것이지 반드시 제가 말한 리뷰여야만 한다는 그런 수학적 공식이나 답이 있는건 아니라고 생각합니다.

추가적인 궁금증이 있으시면 @jinsu4755 로 언급해주시고 만약 제가 확인을 못하는 것 같다면 언제든 플그 전화번호로 카톡 주셔도 됩니다 :)

[jinsu4755]

p5: implementation 으로 추가해주신 awsSDK BOM 은 어떤 일을 하는 라이브러리일까요?

해당 라이브러리를 왜 추가하는지 알아보시면 좋을 것 같아요.
예를 들어 지금 작성하신 파일에서 어떤 종속성에는 반드시 버전이 명시되어있고, 어떤 라이브러리는 버전이 명시되지 않아요.

특히 스프링은 어떤 이유에서 우리가 버전을 "직접" 작성하지 않아도 스프링의 라이브러리를 사용할 수 있을까요? 뭔가 라이브러리 버전을 한번에 관리해주는 친구가 있다면 그 친구의 버전으로 일괄성있게 종속성을 관리해주는게 좋지 않을까요?

참고로 해당 부분 공식 문서에는 다음과 같이 되어있습니다.

...
dependencies {    
    implementation(platform("software.amazon.awssdk:bom:2.21.1"))
    implementation("software.amazon.awssdk:s3")
 ...
}
...

[jinsu4755]

p0: RequestBody 를 사용하였기 때문에 전달 받은 데이터를 어떻게 처리 되는지 알아보면 좋을 것 같아요.

만약 이미지를 처리할 수 없다면 어떤 이유에서 이미지를 처리할 수 없는지, 혹은 있다면 어떻게 처리해야 되는지 알아보시면 좋을 것 같습니다.
물론 현재 변경사항이 없는 것을 미루어보아 단순하게 수정 사항 누락하신 것 같은 느낌도 들지만, 만약 바꿔야한다면 왜 그것을 사용해야하는지 예제는, 혹은 어떤 현업에서는 왜 그것을 사용했는지 아는 것 자체가 클론코딩을 통해 성장할 수 있는 가장 큰 지점이라고 생각하기 때문이에요

[jinsu4755]

p0: uploadImage 에서 validate 를 통해 현재 image 의 확장자를 "image/jpeg", "image/png", "image/jpg", "image/webp"로 받고있는것과 달리 이미지 파일에 .jpg 로 확장자로 모두 붙이는 이유가 있을까요?

반드시 jpg 만 들어오는 서비스라면 문제가 되지 않겠지만 그렇지 않는경우 문제가 발생할 수 있어보입니다.
단순하게 확장자를 바꾸어도 조회에 문제가 없는 경우 역시 존재하지만, 그렇지 않고 이미지가 보이지 않는 경우도 발생할 수 있을 것 같아요!

[jinsu4755]

p1: path 는 사용되는데 값을 받지 않고 해더에서 productID 를 받는 이유가 있을까요?

RequestHeader 는 header 에서 값을 받아오기만 하면 된다면 불필요하게 URL 에서 어떤 값을 읽을 것이라고 표현해둘 필요가 있을까요?

추가적으로 Valid 어노테이션이 단일 값일때도 해당 어노테이션이 작동하나요? 어떤 상황에서 사용하는 친구일까요?

[jinsu4755]

p5: BLOG_S3_UPLOAD_FOLER에 해당하는 폴더 정보를 ItemService 가 알고 처리하기보다 폴더나 파일 처리 모든 부분의 책임을 S3Service 에 위임하는것은 어떨까요?

만약 ItemService 가 아닌 다른 곳에서도 해당 주소에 이미지나 파일을 올려야한다면 해당 Service에도 같은 변수를 작성해야할 수 있어요.
그런 경우를 대비한다면 올려야하는 경로를 관리하는 무언가가 있으면 더 객체지향적이지 않을까요?

[jinsu4755]

comment: 현재 작성하신 부분이 아니지만, S3Service 는 별도의 Interface 를 구현하지 않고 ProductService 는 Interface 를 구현한 이유가 있을까요?

단순하게 필요하다 필요하지 않다를 넘어 어떤 의도를 가지고 만드신 것인지 궁금합니다.

또한 이러한 의도를 생각하면 저는 Impl 이라는 네이밍은 상당히 객체지향적이지 못하고, 확장성에 좋지 않은 영향을 주며 더 나아가 오히려 Service Interface 가 필요없다고 생각되는 이유가 될 수 있다고 생각합니다.

Interface 가 어떤 친구인지도 고민해보면 좋을 것 같아요!

[jinsu4755]

p1: 현재 설계적으로 Image 는 생성될 때 반드시 Product 가 필요할 것 같습니다.
하지만 빌더 패턴의 사용으로 객체 생성에서 가독성이나 편리함은 챙길 수 있었지만, 오히려 필수 파라미터가 초기화 되는지를 보장하지 못하고 있다고 생각해요.

실제로 일부 값을 넣지 않고 build() 를 호출하더라도 개발자의 실수이므로..라고 생각할 수 있겠지만 이런 개발자의 실수 까지 고려하고 설계하여 적절하게 예외를 만들거나 강제하여 사용이 유용하게 만드는 것 역시 초기 코드를 설계하는 사람의 실력이라고 생각합니다.

어떻게 해야 필수적으로 받아야할 부분을 받을 수 있을까요?

+) 여담이지만 실제로 빌더 패턴이 안티패턴이냐 아니냐로 되게 주제가 많이 올라온 적이 있습니다. 이러한 부분도 함께 고민해보셔도 좋을 것 같아요! 제 생각이 궁금하시면 이 역시 저를 언급해서 질문해주세요ㅎㅎ

[jinsu4755]

p5: 해당 함수 안에서 Product 를 만들어 저장하는 일과 Image 를 저장하는일이 일어나고 있어요
추상화 단계에 따라서 각각의 일을 분리하여 가독성을 높여주는 것도 좋을 것같아요.

만약 상품에 보다 복잡하게 연관 관계가 발생하면(태그 등) 해당 함수는 매우 길어지고 가독성이 나빠질 수 있어요. 또한 분리한 함수를 어느정도 재사용 가능하게 추상화 단계에 따라 함수가 하나의 일을 하도록 구성하면 이후에 재사용성 역시 챙길 수 있을 것 같아요