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

[2단계 - 장바구니 기능] 저문(유정훈) 미션 제출합니다. #333

Merged
merged 26 commits into from
May 15, 2023

Conversation

jeomxon
Copy link

@jeomxon jeomxon commented May 5, 2023

안녕하세요 시카 :) 저문입니다.
잘 지내고 계신가요?
이번 2단계는 생각보다 오래 걸렸네요...
많은 시간을 투자했고, 그만큼 학습하고 싶었던 부분도 많았는데 욕심이었던 것 같습니다.
금방 끝낼 수 있을거라고 생각했는데
생각보다 오래걸려서 속상하기도 하고 부족한 부분을 더 채우고 싶은 마음이 커지네요.

Interceptor와 ArgumentResolver를 사용하는 부분은 완벽하게 학습이 되지 않은 상태라
섣불리 적용하는 것보다는 이해를 바탕으로 코드에 녹여내는 것이 좋을 것 같아서
다음 리뷰요청 때 반영하겠습니다!

추가된 컨트롤러에 대한 테스트코드 작성도 인증에 대한 중복 제거를 한 후에 적용하면 좋을 것 같아
다음 리뷰요청 때 함께 반영하겠습니다.

형편없는 코드가 대부분일 것이라 생각이 듭니다.
부족한 부분에 대해서는 가감없이 리뷰 부탁드립니다!!

또한 1단계에 주신 리뷰에 대한 생각과 답변도 달아놓았습니다.

#242

추가적으로 구체적인 설명을 좀 더 듣고싶은 부분도 있어서 남겨두겠습니다!

#242 (comment)

시카의 의도를 파악하기가 어려웠던 부분이었습니다...🥲
제가 아직 이 부분에 대한 생각, 그리고 학습이 부족한 것 같습니다.
질문에 대한 답이 잘 생각나지 않고 왜 이런 질문을 하셨는지를 이해하기 어려웠는데,
이 부분에 대해 학습하면 좋을만한 자료나 키워드가 있을지 궁금합니다.
혹시 가능하시다면 조금 더 구체적으로 설명해주실 수 있을까요?

2단계를 진행하며 궁금한 점이 생겨서 아래에 남겨두었습니다.

더미 데이터로 테스트 코드를 작성하는 것에 대하여

현재 CartDaoTest는 data.sql에 있는 더미 데이터를 이용하여 테스트를 진행하고 있습니다.
이렇게 작성하면서 들었던 의문이 있었습니다.
코드의 표면 상으로 데이터가 드러나지 않는 상태이기에 테스트 코드의 가독성이 떨어진다고 생각했습니다.
어떤 방식으로 이를 개선할 수 있을지 궁금합니다.

2단계 리뷰도 잘 부탁드립니다🙇🏻‍♂️🙇🏻‍♂️🙇🏻‍♂️

Copy link

@dks301 dks301 left a comment

Choose a reason for hiding this comment

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

안녕하세요 저문, 리뷰어 시카입니다. 😄

2단계도 요구사항에 맞춰 잘 구현해주셨네요! 👍
몇 가지 코멘트 남겨드렸으니 확인 부탁드려요!

추가로 궁금한 점이 생기면 언제든지 DM 주세요 ㅎㅎ

src/main/java/cart/controller/CartController.java Outdated Show resolved Hide resolved
src/main/java/cart/controller/CartController.java Outdated Show resolved Hide resolved
public class ProductDto {
public class CartResponse {
Copy link

Choose a reason for hiding this comment

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

  1. ProductDto를 CartResponse로 바꾸신 이유가 있을까요??
  2. ProductResponse와 같은 값을 가지는걸로 보이는데요...! Cart와 Product가 같은 응답을 가지는게 맞는지 고민해보면 어떨까요???

Copy link
Author

Choose a reason for hiding this comment

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

  1. Reqeust와 Response에 대한 DTO의 네이밍을 통일해주기 위해 변경했습니다.
  2. 현재는 같은 응답을 가지지만 추후에는 cart와 product의 각각의 필드가 다르게 변경될 수도 있다는 생각에 두개를 구분해서 작성했습니다. 제 생각이 잘못되었거나 더 좋은 방법이 있으면 다시 고민해보겠습니다!

Copy link

Choose a reason for hiding this comment

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

현재는 같지만 추후 다르게 확장되는 경우 현재로선 같은 값이라도 다른 클래스로 구분하는 부분은 좋은 생각이라고 생각해요 👍👍

다만, https://github.com/woowacourse/jwp-shopping-cart/pull/333/files#r1186689522 를 적용한다면 달라 질 것으로 생각되는데요...! 해당 코멘트를 고민해보고 이 부분도 다시 고민해보면 어떨까요???

Copy link
Author

Choose a reason for hiding this comment

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

public class CartResponse {

    private final List<ProductResponse> productResponses;

    public CartResponse(final List<ProductResponse> productResponses) {
        this.productResponses = productResponses;
    }

    public List<ProductResponse> getProductResponses() {
        return productResponses;
    }
}

이런 방식으로 Cart가 Product 여러 개를 가질 수 있도록 변경한다면,

const cartItems = response.data;

let productElement = '';
cartItems.forEach(cartItem => {
    productElement += `
            <div class="cart-item">
              <div class="cart-item-info">
                <img src="${cartItem.imageUrl}" alt="${cartItem.imageUrl}"/> <!-- 상품 이미지 -->
                <div class="cart-item-name">${cartItem.name}</div> <!-- 상품 이름 -->
                <div class="cart-item-price">${cartItem.price}원</div> <!-- 상품 가격 -->
                <button type="submit" id="delete-btn" class="cart-item-delete" onclick="removeCartItem(${cartItem.id})">Delete</button>
              </div>
            </div>
          `;

위의 js 코드와 일치하지 않아서 문제가 생깁니다.
따라서 getter를 사용하여 body를 넘겨야하는데, 이렇게 Cart가 여러 Product를 가지면서 getter로 넘겨주는 것과
기존에 사용했던 방식인 List로 넘기는 방식의 차이가 있나요?

최대한 js 코드를 바꾸지 않고 적용하려다보니 발생한 문제같기도 합니다...
제가 시도한 방법이 시카의 의도에 부합한지 궁금합니다..!

Copy link

Choose a reason for hiding this comment

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

지금 구현하신 부분대로 js 코드를 바꾸면 cost cartItems = response.data.productResponses가 되겠네요 ㅎㅎㅎ
일단 프론트(js)를 변경하지 않기위해서 CartResponse라는 DTO가 Product DTO와 같은 값을 가지는건 히스토리를 알지 못하는 이상 이해하기 힘들지 않을까요??

개인적으로 js를 변경하지 않고 한다면, CartController에서 가져오지 않고 ProductController에서 List를 body로 갖게 반환할 수 있는지, 그리고 그게 RESTful 한지 고민해볼 것 같아요 ㅎㅎㅎ

Comment on lines 53 to 54
@PostMapping("/cart/products/{productId}")
public ResponseEntity<Void> addProduct(@PathVariable final Long productId, final HttpServletRequest request) {
Copy link

Choose a reason for hiding this comment

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

#242 (comment) 도 RESTful에 대한 이야기인데요 ㅎㅎㅎ

  1. 답변은 조금 길긴 하지만 그런 REST API로 괜찮은가 영상을 참고해 고민해보면 어떨까요??
  2. 답변은 레벨1 미션의 객체지향적으로 고민해볼때와 같이, void addProduct() vs Product addProduct() 와 같은 부분을 고민해보았으면 좋을 것 같았어요😄
  3. delete 의 경우 일반적인 CRU와 다르게 특수성이 있는데요...! 예를들어 create를 하는 경우 정확하게 생성이 되었는지의 여부가 중요하지만, delete의 경우 정확하게 삭제가되었는지가 관심사일 수 있고 or 몇 개가 삭제가 되었는지만 관심사일 수 있고 or 아에 관심이 없을 수도 있는데 그 차이에 대해 고민해보면 좋을 것 같아요!

Copy link
Author

Choose a reason for hiding this comment

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

  1. 영상 추천해주셔서 감사합니다! 학습하는데 훨씬 더 도움이 될 것 같습니다.

  2. 어느정도는 이해가 된 것 같습니다! 풀어서 설명해주셔서 감사합니다.
    제가 return void method를 사용하지 않은 이유는 다음과 같습니다.
    현재의 구조에서는 CartController에서 body를 통해 반환을 하는 메서드가 존재하고 있고,
    @RestController + ResponseEntity의 조합을 가져가고자 선택을 했습니다.
    따라서 통일성을 위해서 위의 조합을 그대로 반영하였습니다.
    또한 ResponseEntity를 사용하면서 추가적으로 전달할 정보가 생긴다면
    반환할 수 있는 정보(헤더, 바디, 상태코드 등)가 훨씬 다양하기에 선택한 것도 있습니다.

  3. 그런 특수성이 있는지 전혀 몰랐습니다..! 고민해보겠습니다.
    다만 현재 상황에서는 삭제에 굳이 다른 정보가 필요할까?라는 생각이 들었습니다.
    현재는 하나의 상품만 삭제가 되는 방식이기도 하고, 정확한 정보를 받는다고 하더라도 이후에 사용할 여지가 없다고 판단했기 때문입니다.
    추가적으로 create에는 delete와 같은 특수성이 왜 존재하지 않는지 궁금합니다.
    create도 마찬가지로 정확하게 생성이 되었는지, 몇 개가 생성되었는지가 관심사가 될 수 있지 않나요?

Copy link

Choose a reason for hiding this comment

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

  1. 그렇다면 영상 내용을 적용해 봤을 때 ResponseEntity vs ResponseEntity 중 어느것을 사용하는 것이 좋다고 생각하시나요??
  2. delete의 특수성은 한 번 쯤 고민만 해보면 좋을 것 같아서 코멘드드렸구요 ㅎㅎㅎ create의 경우는 명확하게 정확하게 어떤 데이터가 생성되었는지(1개 or collection 전체)가 관심사가 아닐까요??
    예를 들어서, 회원 가입을 하는 경우 client는 정확한 값으로 회원 가입이 되었는지까지가 관심사기에 응답으로 생성된 모든 값을 보내줄 수 있는데요...! 반대로 회원 탈퇴의 경우 어떠한 회원이 탈퇴되었는지는 client 입장에서는 관심사가 아니기 때문에 삭제가 정상적으로 되었는지 여부만 관심사일 확률이 높지 않을까요? ㅎㅎㅎ

Copy link
Author

@jeomxon jeomxon May 14, 2023

Choose a reason for hiding this comment

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

  1. @RestController + ResponseEntity의 조합을 사용하는 것이 좋다고 판단됩니다.
    @RestController라는 어노테이션을 통해서 코드 라인에서도 Rest요청임을 명시하는 것이 좋다고 생각했습니다.
    REST를 따르려면 self-descriptive messages, 즉 메시지는 스스로 설명해야한다라는 규칙이 있기에
    ResponseEntity를 통해서 void와는 달리 헤더 등에도 더 많은 내용을 포함할 수 있고,
    이는 메시지를 통해 온전히 모든 내용을 설명할 수 있게 도와줍니다.

혹시 제가 영상을 통해서 의도와 다르게 학습했을 수 있는데,
틀린 내용이 있다면 잡아주시면 감사드리겠습니다!

  1. 이제 이해가 되는 것 같습니다!
    클라이언트의 관심사 기준으로 잘 고민해보면 해답을 얻을 수 있다고 느꼈습니다.
    좋은 답변 감사드립니다🙇🏻‍♂️

Comment on lines +3 to +7
public class Cart {

private final Long id;
private final Long memberId;
private final Long productId;
Copy link

Choose a reason for hiding this comment

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

현재는 도메인이 특별한 비즈니스 로직이 없고 DB에 종속적인 class로 보이는데요...!
DB에 종속되지 않고 객체 지향적인 객체로 우선 정의해보면 어떨까요??

객체 자체로만 고민해보면다면,

  • 한 명의 Member는 하나의 Cart를 가지고
  • 하나의 Cart에는 여러개의 Product를 가질 수 있다.
  • 등등

위와 같은 방식으로 도메인에 비즈니스 로직이 들어갈 수 있지 않을까요???

Copy link
Author

Choose a reason for hiding this comment

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

domain패키지 내부에 있는 클래스들은 DB에 종속적인 클래스로 설계하였습니다.
즉 엔티티의 역할로써 사용한 것인데요.
이런 경우는 패키지를 리네이밍 하는 것이 더 좋은 방법이 될까요?

저는 DB에 종속적인 클래스 그 이상의 역할이 필요한가?라는 질문에 '아니다'라는 결론을 내렸고,
따라서 엔티티처럼 사용했던 것 같습니다.

시카가 말씀해주신 것처럼 객체지향적인 객체로 정의하는 이유는
검증 등의 추가 로직이 도메인이 함께 가질 수 있기 때문인가요?
생각이 잘 떠오르지 않아서 그런데
어떤 이유에서 객체지향적인 객체로 우선 정의하라고 하셨는지 궁금합니다.

Copy link

Choose a reason for hiding this comment

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

이 부분은 정답이 있는 부분은 아닌데요...! 일반적으로 Anemic vs Rich 도메인 모델이라고 볼 수 있습니다.

현재와 같이 가져간다면, 비즈니스 로직이 Service or DAO에서 처리되기 떄문에 캡슐화 원칙을 위배하고, 객체 지향적 장점을 가지고 갈 수 없게 되는데요...!

정답은 없지만, 처음 학습하는 과정에서는 Rich Domain Model로 작업해 도메인 모델을 명시적이고 직관적으로 작성하여 레벨 1에서 학습했던 객체 지향적 장점을 가지고 가면서 스프링 프레임워크를 적용하는 방법으로 진행해보는게 좋을 것 같은데 어떻게 생각하시나요?? 😄

Copy link
Author

Choose a reason for hiding this comment

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

이번에 진행하게 됐던 지하철 미션을 하면서 특히나 느꼈던 것 같습니다.
장바구니 미션에서는 전혀 객체지향에 대해 생각하지 않았는데, 다시 생각해보니 그러면 객체지향언어를 사용할 이유가 전혀 없다는 생각이 들었습니다.
시카가 보내주신 링크를 통해서 Anemic과 Rich Domain Model에 대해 알게 되었고, 지하철 미션에서는 적용하려고 노력해봤습니다.

정답은 없지만, 처음 학습하는 과정에서는 Rich Domain Model로 작업해 도메인 모델을 명시적이고 직관적으로 작성하여 레벨 1에서 학습했던 객체 지향적 장점을 가지고 가면서 스프링 프레임워크를 적용하는 방법으로 진행해보는게 좋을 것 같은데 어떻게 생각하시나요?? 😄

이 말씀의 필요성을 충분히 느끼게 되었고, 적용해야한다고 생각합니다.
하지만 현재 장바구니 기능에서는 도메인 객체가 가져야할 핵심 기능이 크게 존재하지 않다고 판단했습니다.
로직이라고 하면 계층간의 dto를 변환하는 역할 밖에 없다고 생각이 들어서 기존의 방식을 유지하고,
이어지는 미션에서 비즈니스 로직이 생기게 되면 추가하는 것이 좋을 것 같다고 생각합니다!

@jeomxon
Copy link
Author

jeomxon commented May 8, 2023

안녕하세요 시카 :)
말씀드렸던 HandlerInterceptor와 HandlerMethodArgumentResolver를 추가하고,
컨트롤러에 대한 테스트코드를 추가해보았습니다.

제가 현재 HandlerInterceptor와 HandlerMethodArgumentResolver를 올바른 방식으로 사용했는지가 궁금합니다.
학습을 하면서 많은 시간을 투자하고 어려움을 겪었기에 확실하게 배워가고 싶은데,
아직은 코드에 제대로 녹여내지 못한 것 같은 느낌이 듭니다.
수정할 부분이 있으면 가감없이 말씀해주시면 감사드리겠습니다!!

또한 현재의 테스트에서는 대부분이 성공 테스트가 되었는데,
실패 테스트는 어떤 방식으로 작성하는 것이 좋을지 궁금합니다.
특히 컨트롤러 요청에 대한 실패 테스트는 어떤 방식으로 작성할 수 있는지 궁금합니다!

리뷰 잘 부탁드립니다! 감사합니다🙇🏻‍♂️🙇🏻‍♂️

Copy link

@dks301 dks301 left a comment

Choose a reason for hiding this comment

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

안녕하세요 저문, 리뷰어 시카입니다. 😄

코멘트를 잘 학습하고 고민하신 부분이 보이네요! 👍
진행중인 코멘트에 대한 답변을 남겨드렸으니 확인 부탁드려요!

추가로 궁금한 점이 생기면 언제든지 DM 주세요 ㅎㅎ

Comment on lines +3 to +7
public class Cart {

private final Long id;
private final Long memberId;
private final Long productId;
Copy link

Choose a reason for hiding this comment

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

이 부분은 정답이 있는 부분은 아닌데요...! 일반적으로 Anemic vs Rich 도메인 모델이라고 볼 수 있습니다.

현재와 같이 가져간다면, 비즈니스 로직이 Service or DAO에서 처리되기 떄문에 캡슐화 원칙을 위배하고, 객체 지향적 장점을 가지고 갈 수 없게 되는데요...!

정답은 없지만, 처음 학습하는 과정에서는 Rich Domain Model로 작업해 도메인 모델을 명시적이고 직관적으로 작성하여 레벨 1에서 학습했던 객체 지향적 장점을 가지고 가면서 스프링 프레임워크를 적용하는 방법으로 진행해보는게 좋을 것 같은데 어떻게 생각하시나요?? 😄

public class ProductDto {
public class CartResponse {
Copy link

Choose a reason for hiding this comment

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

현재는 같지만 추후 다르게 확장되는 경우 현재로선 같은 값이라도 다른 클래스로 구분하는 부분은 좋은 생각이라고 생각해요 👍👍

다만, https://github.com/woowacourse/jwp-shopping-cart/pull/333/files#r1186689522 를 적용한다면 달라 질 것으로 생각되는데요...! 해당 코멘트를 고민해보고 이 부분도 다시 고민해보면 어떨까요???

Comment on lines 53 to 54
@PostMapping("/cart/products/{productId}")
public ResponseEntity<Void> addProduct(@PathVariable final Long productId, final HttpServletRequest request) {
Copy link

Choose a reason for hiding this comment

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

  1. 그렇다면 영상 내용을 적용해 봤을 때 ResponseEntity vs ResponseEntity 중 어느것을 사용하는 것이 좋다고 생각하시나요??
  2. delete의 특수성은 한 번 쯤 고민만 해보면 좋을 것 같아서 코멘드드렸구요 ㅎㅎㅎ create의 경우는 명확하게 정확하게 어떤 데이터가 생성되었는지(1개 or collection 전체)가 관심사가 아닐까요??
    예를 들어서, 회원 가입을 하는 경우 client는 정확한 값으로 회원 가입이 되었는지까지가 관심사기에 응답으로 생성된 모든 값을 보내줄 수 있는데요...! 반대로 회원 탈퇴의 경우 어떠한 회원이 탈퇴되었는지는 client 입장에서는 관심사가 아니기 때문에 삭제가 정상적으로 되었는지 여부만 관심사일 확률이 높지 않을까요? ㅎㅎㅎ

Copy link

@dks301 dks301 left a comment

Choose a reason for hiding this comment

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

안녕하세요 저문, 리뷰어 시카입니다. 😄

이번 미션에서는 깊이 고민해볼 만한 내용을 중점적으로 코멘트 드렸는데요...!
다음 미션으로 바쁘실텐데 많은 고민을 하면서 마무리까지 잘해주신것 같아요 💯👏
이번 미션은 이만 머지하겠습니다. 수고하셨습니다!

추가로 궁금한 점이 생기면 언제든지 DM 주세요 ㅎㅎ

public class ProductDto {
public class CartResponse {
Copy link

Choose a reason for hiding this comment

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

지금 구현하신 부분대로 js 코드를 바꾸면 cost cartItems = response.data.productResponses가 되겠네요 ㅎㅎㅎ
일단 프론트(js)를 변경하지 않기위해서 CartResponse라는 DTO가 Product DTO와 같은 값을 가지는건 히스토리를 알지 못하는 이상 이해하기 힘들지 않을까요??

개인적으로 js를 변경하지 않고 한다면, CartController에서 가져오지 않고 ProductController에서 List를 body로 갖게 반환할 수 있는지, 그리고 그게 RESTful 한지 고민해볼 것 같아요 ㅎㅎㅎ

@dks301 dks301 merged commit afb72db into woowacourse:jeomxon May 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants