6차 세미나 클론 코딩 과제 (#14) #16
Conversation
sjk4618
requested review from
tkdwns414,
sohyundoh,
PgmJun,
yummygyudon,
Parkjyun and
bo-ram-bo-ram
There was a problem hiding this comment.
안녕하세요 이번에 리뷰어로 참여하게된 명예OB 박진수입니다! 회사 일정이 바빠서 리뷰가 늦어졌습니다...죄송합니다.
코드리뷰에서 사용된 단계는 다음과 같습니다.
p0 : 최고 우선 순위, 빠르게 고쳐야할 치명적인 버그나 사이드 이팩트가 예상됨. 혹은 요구사항을 만족하지 못하는 경우
p1: 함수 혹은 객체가 요구사항을 만족 하는데 충분히 명확하지 않은 경우. 불필요한 코드 작성이 반복되거나 복잡도가 높은 경우, 일관성을 해치는 경우
p5~: 네이밍 부터 자잘한 수정 사항
정적 팩토리 메서드라던가, GlobalExceptionHandler, Util 등 패키지나 다양한 곳에서 어떤 고민을 하셨을지 보여서 쏠쏠하게 읽으며 리뷰 남겼습니다
코드를 보며 느낀점은 뭔가 S3Service 는 세미나 코드인것 같은데 해당 코드 역시 고민을 더 해보셔도 될 것 같다는 생각이 들었습니다.
코드가 틀렸다 보다는 저 역시 파트장을 했기 때문에
세미나의 코드는 역대 모든 파트장들이 어떻게 하면 아무것도 모르는 사람도 이해할 수 있을까? 고민한 결과물이기 때문에 오히려 왜 이런 구조를 띄게 되었을까? 고민해보시면 좋을 것 같아요!
그리고 더 나은 구조 역시 고민해보시면 좋을 것 같아요. 저는 파트장을 하면서도 이야기 했지만, 결국 설명과 이해를 위한 코드이므로 이상적이고 좋은 코드와는 거리가 먼 경우가 분명 발생하곤 합니다.(객체지향적으로 혹은 더 좋은 아키택처로 하면 처음하는 사람들은 절대 따라오지 못하니까요...)
그럼 여기서 역대 파트장들의 노력이 주는 과제는 "왜 여기서 코드가 이렇게 작성되었지?", "만약 더 좋게 만든다면 어떻게 해야할까?" 고민하는 것이라 생각합니다
또한 제가 경험했던 것을 바탕으로 혹은 실무에서 본것을 바탕으로 리뷰를 남겨드리고자 노력했지만, 성준님이 리뷰를 읽어보시고 성준님이 맞다고 생각하시는 부분이 있다면 제 리뷰에 반박해서 저를 설득해보시는 것도 좋을 것 같아요.
리뷰는 결국 코드를 보고 상호 소통하는 것이기 때문에 이런 소통이 더 나은 결과물을 만들 수 있어요. 또한 요구사항을 만족한다면 코드 작성 방법에는 절대적인 정답은 없다고 생각해요. 어떤 상황에서 A가 B를 설득한다면 그 상황에 맞는 방법은 A의 방법이 되는 것이지 반드시 제가 말한 리뷰여야만 한다는 그런 수학적 공식이나 답이 있는건 아니라고 생각합니다.
추가적인 궁금증이 있으시면 @jinsu4755 로 언급해주시고 만약 제가 확인을 못하는 것 같다면 언제든 플그 전화번호로 카톡 주셔도 됩니다 :)
carrotMarket/build.gradle
Outdated
| //Multipart file | ||
| implementation("software.amazon.awssdk:bom:2.21.0") | ||
| implementation("software.amazon.awssdk:s3:2.21.0") |
There was a problem hiding this comment.
p5: implementation 으로 추가해주신 awsSDK BOM 은 어떤 일을 하는 라이브러리일까요?
해당 라이브러리를 왜 추가하는지 알아보시면 좋을 것 같아요.
예를 들어 지금 작성하신 파일에서 어떤 종속성에는 반드시 버전이 명시되어있고, 어떤 라이브러리는 버전이 명시되지 않아요.
특히 스프링은 어떤 이유에서 우리가 버전을 "직접" 작성하지 않아도 스프링의 라이브러리를 사용할 수 있을까요? 뭔가 라이브러리 버전을 한번에 관리해주는 친구가 있다면 그 친구의 버전으로 일괄성있게 종속성을 관리해주는게 좋지 않을까요?
참고로 해당 부분 공식 문서에는 다음과 같이 되어있습니다.
...
dependencies {
implementation(platform("software.amazon.awssdk:bom:2.21.1"))
implementation("software.amazon.awssdk:s3")
...
}
...
추가적인 이야기지만 주석의 경우는 개인적으로는 Mutipart 보다는 AWS 정도가 좋을 것 같습니다.
물론 Multipart file 이라는 주석을 통해 해당 라이브러리가 쓰이는 용도로 구분하는 것 역시 나쁘진 않지만, 추후 AWS SDK 가 추가로 사용되는 경우 해당 주석은 충분히 혼란을 줄 수 있을 것 같습니다. 물론 개인적인 생각이에요 :)
There was a problem hiding this comment.
오 aswSDK BOM이 무슨 라이브러린지 몰랐는데, 처음 알게 되었습니다!!
버전을 일괄적으로, 명시적으로 사용할수 있군요!
주석 또한 진수님의 의견에 동의해서 바꾸도록 하겠습니다!
| @ModelAttribute final RegisterItemDTO registerItemDTO) { | ||
| RegisterItemResponseDTO registerItemResponseDTO = itemService.registerItem(memberId, registerItemDTO); | ||
|
|
||
| return ApiResponseUtil.success(SuccessMessage.ITEM_REGISTER_SUCCESS, registerItemResponseDTO); |
There was a problem hiding this comment.
p1: ApiResponseUtil 를 별도로 작성하신 이유가 있을까요?
현재 ApiResponseUtil 에 작성해주신 내용 모두 ResponseEntity 객체 내부에 static method 로 보다 풍부하게 지원해주는 함수가 있습니다! 확인해보시면 좋을 것 같아요!
There was a problem hiding this comment.
제 현재 생각으로는 success와 failure를 따로 커스텀해서 보낼 수 있어서 위와 같이 했습니다!
확인하도록 하겠습니다
| @@ -8,6 +8,7 @@ | |||
| @AllArgsConstructor | |||
| public enum ErrorMessage { | |||
| MEMBER_NOT_FOUND(HttpStatus.NOT_FOUND.value(), "MemberID에 해당하는 멤버가 없습니다."), | |||
| ITEM_NOT_FOUND(HttpStatus.NOT_FOUND.value(), "해당하는 아이템이 없습니다"), | |||
There was a problem hiding this comment.
p5: 큰 문제는 없지만 혹시 따로 value 만 받아서 전달하시는 이유가 있을까요?
static <T> ResponseEntity<BaseResponse<?>> success(SuccessMessage successMessage, T data) {
return ResponseEntity
.status(successMessage.getStatus())
.body(BaseResponse.of(successMessage, data));
}결국 해당 함수에서 그대로 가져와서 사용할 뿐이라면 HttpStatus.NOT_FOUND 여도 동일하게 작동하는데 별도로 value 만 받아오는 의도가 있으셨나요?
| @RequestBody final RegisterItemDTO registerItemDTO) { | ||
| itemService.registerItem(memberId, registerItemDTO); | ||
| return ApiResponseUtil.success(SuccessMessage.ITEM_REGISTER_SUCCESS); | ||
| @ModelAttribute final RegisterItemDTO registerItemDTO) { |
There was a problem hiding this comment.
comment: 과제를 살펴보니 모든 분들이 ModelAttibute 를 사용하셨는데 사용하신 이유가 뭘까요?
만약 그 이유가 다른 어떤 수단들 보다 좋다고 생각해서 사용하시는 것인지, 아니면 단순하게 따라하신 것인지 궁금합니다.
만약 전자와 같이 설득할 수 있는 이유가 없이 단순하게 사용만 하신 것이라면 왜 이것을 사용해야만 했는지 그 이유를 찾아보시는 것 역시 좋을 것 같아요.
과제는 "클론"코딩이기 때문에 따라했다 역시 사용할 줄 안다는 측면에서는 배움이 될 수 있어요.
하지만 남들과 다르게 성장하기 위해서는 클론코딩 과정에서 유명한 프로젝트는, 예제는, 혹은 어떤 현업에서는 왜 그것을 사용했는지 아는 것 자체가 클론코딩을 통해 성장할 수 있는 가장 큰 지점이라고 생각하기 때문이에요
There was a problem hiding this comment.
세미나에서 사용해서 그냥 사용했는데, 지금 바로 공부해보겠습니다!
| @@ -51,9 +54,10 @@ private Item(Member member, String title, int price, boolean isReceived, String | |||
| } | |||
|
|
|||
| //정적팩토리메서드(빌더패턴이용) | |||
| public static Item register(Member member, String title, int price, boolean isReceived, String detailInfo, Location hopeTradeSpot) { | |||
| public static Item register(Member member, String imageUrl, String title, int price, boolean isReceived, String detailInfo, Location hopeTradeSpot) { | |||
| private String generateImageFileName() { | ||
| return UUID.randomUUID() + ".jpg"; | ||
| } |
There was a problem hiding this comment.
p0: uploadImage 에서 validate 를 통해 현재 image 의 확장자를 "image/jpeg", "image/png", "image/jpg", "image/webp"로 받고있는것과 달리 이미지 파일에 .jpg 로 확장자로 모두 붙이는 이유가 있을까요?
반드시 jpg 만 들어오는 서비스라면 문제가 되지 않겠지만 그렇지 않는경우 문제가 발생할 수 있어보입니다.
단순하게 확장자를 바꾸어도 조회에 문제가 없는 경우 역시 존재하지만, 그렇지 않고 이미지가 보이지 않는 경우도 발생할 수 있을 것 같아요!
There was a problem hiding this comment.
오 .jpg를 지워겠네요!!
그렇지 않는 경우를 생각하지 못했던 것 같습니다
| @@ -27,22 +30,31 @@ public class ItemService { | |||
| private final ItemLikesRepository itemLikesRepository; | |||
| private final ItemLikesService itemLikesService; | |||
|
|
|||
| private final S3Service s3Service; | |||
| private static final String BLOG_S3_UPLOAD_FOLER = "blog/"; | |||
There was a problem hiding this comment.
p5: BLOG_S3_UPLOAD_FOLER에 해당하는 폴더 정보를 ItemService 가 알고 처리하기보다 폴더나 파일 처리 모든 부분의 책임을 S3Service 에 위임하는것은 어떨까요?
만약 ItemService 가 아닌 다른 곳에서도 해당 주소에 이미지나 파일을 올려야한다면 해당 Service에도 같은 변수를 작성해야할 수 있어요.
그런 경우를 대비한다면 올려야하는 경로를 관리하는 무언가가 있으면 더 객체지향적이지 않을까요?
| } catch (RuntimeException | IOException e) { | ||
| throw new RuntimeException(e.getMessage()); | ||
| } |
There was a problem hiding this comment.
p5: 발생하는 Exception 을 모두 하나로 처리하는 이유가 있나요?
현재 RuntimeException 과 IOException이 모두 사실상 같은 RuntimeException 으로 처리되게 되는것 같아요.
예외라는 것은 무엇일까요? 에러와는 어떤 차이가 있을까요?
참고로 RuntimeException 과 IOExcpetion 은 모두 Exception 을 상속하고있어요. 그렇다면 두 에러를 Exception 으로 catch 하면 되는걸지 고민해보면 좋을 것 같아요.
자바는 왜 Exception 을 여러가지로 나누고 상세하게 나누었을지 고민하면 우리가 예외를 처리할 때 역시 어떻게 해야하는지 보일것 같아요.
이런 이유를 알고 더 나아간다면 내 서비스 도메인에 맞는 로직을 처리하기 위한 Exception 을 Custom 하는 것 역시도 생각해볼 수 있을 거고 추후 도입하실지 모르겠지만 ControllerAdvice 등 보다 유연하게 에러를 처리하여 클라이언트에게 표시하는 방법도 고민해보면 좋을 것 같아요!
만약 위 이야기에서 자신만의 답을 찾으셨다면 또 다른 부분에서 고민해보실 수 있는 자료도 첨부해봅니다.
해당 링크는 꼭 위 이야기에 자신만의 답을 찾고 읽어주세요!
https://tecoble.techcourse.co.kr/post/2020-08-17-custom-exception/
There was a problem hiding this comment.
추가적으로 현재 GlobalExceptionHandler 라는 CotrollerAdvice 를 사용하고 계신데 이와 같이 처리한다면 추후 실제로 의도하지 않은 예외가 발생하는 것에 처리를 할 수 없고, 예외 상황 추적에서도 문제가 발생할 수 있습니다
| @@ -24,14 +25,15 @@ public record GetAllItemsInfoResponseDTO( | |||
| //지역별로 받아올 때는, 내가 좋아요를 누른지가 필요하지 않아서 builder를 이용했다. | |||
|
|
|||
| @Builder //빌더패턴 | |||
| public GetAllItemsInfoResponseDTO(Long itemId, String title, int price, | |||
| public GetAllItemsInfoResponseDTO(Long itemId, String title, String imageUrl,int price, | |||
There was a problem hiding this comment.
p5: 오히려 entity 보다 지금같은 경우에서 정적 팩토리 메서드를 사용하는것이 더 좋다고 생각합니다.
해당 DTO 는 왜 필요할까요? 우리는 왜 Entity 자체를 Response 로 사용하지 않을까요?
만약 당장 어떤 이유로 Entity 를 Dto 로 변환한다면 오히려 여기에서는 공개된 생성자와 빌더가 아닌 반드시 entity 를 받아서 생성될 수 있도록 의도하는 정적 팩토리 메소드를 사용하는 것이 더 좋지 않을까요?
|
아참...세미나 리뷰도 요청해주셨는데...제가 시간이 난다면 얼마든 해드리고 싶지만ㅠ 일정상 급하게 6월말까지 끝내야하는 일이 있어서 혹시 리뷰 요청해주신 특별한 이유가 있으실까요? 혹여나 고민점이 있으셨거나 궁금하신 점이 있다면 플그 연락처로 연락 남겨주시면 확인하면 바로 답장해드리겠습니다 |
이 주의 과제
요구사항 분석
질문있어요!
정말 많이 바쁘시겠지만,,,,
시간이 되신다면!! 세미나 과제 피알 리뷰도 부탁드립니다!!
세미나 과제 피알 -> #15
감사합니다🙇