[Spring 장바구니 - 2단계] 디우(김동규) 미션 제출합니다. #132
Conversation
팁리팩토링에 대한 저만의 팁을 말씀해주셨는데요;;; 솔직하게 말씀드리면 없습니다 😭 있으면 알고 싶네요.
저는 기본적으로 2번째로 진행을 하는 것 같아요ㅎㅎ리팩토링의 과정에 운영코드 뿐만이 아니라 테스트에 대한 것도 포함이 되어 있다고 생각하기 때문에 어쩔 수 없는 부분이지 않나 생각합니다! 만약 구조가 현재에 너무 맞지 않다고 한다면 구조를 새로 설계하는 것도 한가지 방법이 될 수도 있겠죠? |
| } | ||
|
|
||
| @GetMapping("/{cartId}") | ||
| public ResponseEntity<CartResponse> getCartItems(@AuthenticationPrincipal String email, |
There was a problem hiding this comment.
controller 테스트가 없는데 acceptance 테스트로 대체하기로 한건지 궁금하네요ㅎㅎ
There was a problem hiding this comment.
맞습니다...음..솔직히 이전까지 별도로 controller에 대한 테스트코드를 작성하지 않았었습니다. 왜냐하면 동일한 테스트를 두 번 작성하는 것과 같다는 생각이 들었기 때문입니다. 서비스 계층에 대한 테스트가 별도로 존재하기 때문에 인수테스트를 하면서 사용자 요청& 응답 처리에 관련된 부분에 대한 테스트를 진행한다고 생각했기 때문입니다.
하지만 해당 질문을 받고 나서 곰곰이 생각해보니, 테스트의 목적에서 차이가 날 수도 있다는 생각이 들었습니다.
구체적인 예를 들면 사용자 시나리오와는 별개로 입력값 validation에 대한 테스트가 있겠습니다. 상품의 가격은 음수일 수 없다.와 같은 테스트를 controller 테스트에서 진행해볼 수 있겠습니다!
그렇지만 이외에는 별도로 Contoller에 대한 테스튼가 존재해야하는 이유를 찾지 못했습니다...
그렇다고 어떻게 보면 내가 사용하는 외부의 기능(javax.validation.constraints패키지에서 제공)에 대한 테스트를 진행하는것이 맞는가? 하는 생각도 들었습니다.
혹시 로운이 생각하기에 acceptance 테스트와는 별개로 controller테스트가 존재해야하는 이유를 공유해주실 수 있을까요...??😅
There was a problem hiding this comment.
시나리오를 작성해서 진행을 하는 것이 atdd 테스트인데요. 개발자는 기능을 테스트 하는 것에 집중을 하지만 atdd는 프로젝트, 기획자분들과 함께 프로그램의 흐름에 집중을 해서 만드는 것이기 때문에 용도가 다르다고 생각해요. 그래서 개발자가 집중하는 기능 테스트는 controller 테스트로 진행하고, atdd는 흐름을 테스트하는 것 같은데요 😄
아마 이 부분은 기획자분들과 함께 일할 때 더 크게 느껴지지 않을까 싶어요.
기획자분들이 기획을 할 때는 사용자의 사용 흐름을 생각하고, 그 흐름에 맞춰 디자인 시안을 만드는 형식으로 진행을 하거든요.
사용자의 흐름에 맞춰 시나리오를 짤 때, 우리 api만 사용하는 것이 아닌 여러 팀의 api를 사용하는 경우에는 어떻게 진행을 해야할까요?
이러한 부분 때문에 atdd로 모든 경우를 테스트할 수 없어, controller 테스트를 진행하는 것이지 않나 생각합니다~
https://tecoble.techcourse.co.kr/post/2021-05-25-unit-test-vs-integration-test-vs-acceptance-test/
| @Configuration | ||
| public class SwaggerConfig { | ||
| @Bean | ||
| public Docket api() { |
There was a problem hiding this comment.
swagger 도입 좋네요ㅎㅎ spring restdocs와 장단을 비교해봐도 좋을것 같아요!
There was a problem hiding this comment.
크루들 사이에서도 Notion과 같이 별도의 툴을 이용해서 API 명세를 관리하는 크루들도 있었고, README, Swagger, Spring REST Docs등 다양하였습니다.
이 둘의 장단을 비교해보면 다음과 같을 것 같습니다.
- Swagger
- 장점
- GET, DELETE 등 HTTP 메소드에 따라 구분(색을통해)해주며 편리하게 사용할 수 있다.
- API 문서화 이외에도 실제 API를 간단하게 테스트하는 기능을 제공한다. (본인은 이를 통해 테스트하기 보다는 postMan을 통해서 하는 것을 선호한다..토큰등을 실어보내는 방법을 몰라서😅)
- 프로덕트 코드에 설정을 통해서 굉장히 간단하게 API 문서를 얻을 수 있다.
- 단점
- 프로덕트 코드에 Swagger 설정과 관련된 코드가 포함되게 된다.
- 프로덕트 코드와 동기화가 안될 수 있다. (본인이 직접 느껴보진 못했지만, 다음의 링크를 참고하였습니다.) 참고 링크
- 장점
- Spring Rest Docs
- 장점
- 프로덕트 코드에 영향이 없다.
- 테스트가 제대로 수행되지 않으면 문서가 작성되지 않는다는 장점이 있다.
- 단점
- 테스트를 작성해야하기 때문에 비용이 든다. (이 부분은 관점에 따라 다르다고 생각합니다. 앞서 참고한 링크를 보면 테스트가 성공해야 문서작성이 된다. 라는 점을 장점으로 꼽은 것으로 보면 꼼꼼한 테스트 작성이 가능해지기 때문에 장점으로 본 것 같습니다. 하지만 테스트 코드 작성도 결국 비용이라는 관점에서는 swagger와 비교했을 때 단점이라고 생각합니다..!)
- 엔트포인트마다 수많은 코드를 추가해줘야한다. (그만큼 여러 메소드를 알아야하기 때문에 제대로 사용하기 위해서는 학습해야 할 양도 많다고 생각합니다.)
- 장점
Spring Rest Docs는 직접 사용해보지는 못했어서..여러 블로그글들을 참고하며 적어보았습니다. 이렇게 정리를 하면서 장단을 비교해보고 나니, 음..🤔 저는 API 명세를 하나만으로 하는 것 보다는 2개정도의 툴을 이용하여 유지보수해나가도 괜찮지 않을까하는 생각이 들었습니다. (이 두 문서의 동기화 부분에서는 사람이 일일이 확인하는 것 이외에 아직 어떻게 하면 좋을지에 대한 생각이 들지는 않지만...)
왜냐하면 각각의 장단이 존재하고 이는 개인의 취향이라는 생각이 듭니다. 내가 속한 팀에서 모든 팀원의 의견을 만족하며 API 명세를 작성해나가기는 힘들다고 생각합니다. 또 예를 들어, Swagger 기반으로 도출된 API에 대한 테스트를 작성하여 Spring REST Docs를 도출한다와 같은 방법도 꼼꼼하게 명세를 도출해내는 방법이 될 수도 있을 것 같다는 생각이 들기 때문입니다!
There was a problem hiding this comment.
맞습니다 가자 장단점이 있고, 팀에서 논의 끝에 정한 것으로 문서를 작성하는데요.
추후에 어떤 것을 사용할지 정해야할 때, 지금 고민한 부분이 도움이 될 것 같구요.
참고로 말씀을 드리면 생각보다 테스트를 유지보수 하는 것이 쉽지 않고, api들이 굉장히 많아서, 2개 다 사용하는 것은 쉽지 않을 것 같습니 다 😅
| new ThumbnailImage(product.getImage())); | ||
| } | ||
|
|
||
| public ProductResponse(Product product) { |
There was a problem hiding this comment.
크게 고민하지 않고 생성자 체이닝 방식을 사용했던 것 같은데요, 로운의 말씀을 듣고 보니 Product 객체로부터 ProductResponse를 만든다는 의미도 "from"이라는 메소드 네임을 통해서 전달받을 수 있어서 좀 더 적절할 것 같다는 생각도 듭니다.
그리고 ProductResponse의 생성자에서 ThumbnailImage를 생성하고 있는데, 단순 "객체 생성"에 대한 책임만을 가지는 생성자에서 ThumbnailImage 객체 생성에 대한 책임도 가지고 있어 적절하지 못했다는 생각이 듭니다. 특히 ProductResponse에서 보다 OrderResponse를 보면 기존 코드가 다음과 같습니다.
public OrderResponse(Orders orders) {
this.id = orders.getId();
this.orderedProducts = orders.getOrderDetails().stream()
.map(orderDetail -> ProductResponse.from(orderDetail.getProduct()))
.collect(toList());
}위 코드를 보면 인자로 받은 Orders 객체에 대해서 ProductResponse 리스트로 변환하는 책임을 생성자에서 함께 가지고 있는데요, 이는 객체 생성과 관련된 최소한의 책임만을 가지는 생성자에 대해서 적절하지 않다는 생각이 들었고, 다음과 같이 정적 팩토리 메소드를 이용하여 확실하게 책임을 분리해줄 수 있다는 생각이 들었습니다.
public static OrderResponse from(Orders orders) {
final List<ProductResponse> orderedProducts = orders.getOrderDetails()
.stream()
.map(orderDetail -> ProductResponse.from(orderDetail.getProduct()))
.collect(toList());
return new OrderResponse(orders.getId(), orderedProducts);
}| @@ -7,7 +7,7 @@ public class Orders { | |||
| private final Long id; | |||
| private final List<OrderDetail> orderDetails; | |||
|
|
|||
| public Orders(final Long id, final List<OrderDetail> orderDetails) { | |||
| public Orders(Long id, List<OrderDetail> orderDetails) { | |||
There was a problem hiding this comment.
하나의 주문에 여러개의 주문상세 내용이 있는객체인데 Orders가 맞을까요?
There was a problem hiding this comment.
지금까지 제가 사용했던 일급컬렉션 클래스들을 보면 모두 XXXs 와 같은 형태로 네이밍을 해주었었습니다.
따라서 만약 제가 이코드를 처음 보았다면 "아, Order에 대한 일급컬렉션이겠구나" 라고 생각할 것 같습니다. 즉 하나의 주문과 관련된 클래스가 아닌 여러 주문들에 관련된 클래스라고 생각할 것 같습니다. (아마 로운도 그러한 생각이 들어서 이와 같은 리뷰를 남겨주신것이라고 생각이듭니다..!!)
기존에 Orders와 같이 해준 이유는 DB Table과 네이밍을 동일하게 가져가려고 했던 것이고 스킴 정의시에 Orders를 사용한 이유는 Order 가 예약어이기 때문입니다..!! 하지만 위와 같이 오해의 소지가 있다는 것을 고려하면 테이블과는 약간의 차이가 존재하게 되지만 Order로 표현하는게 자바 애플리케이션 내에서는 보다 적절하다는 생각이 듭니다! 수정해보도록 하겠습니다!!
There was a problem hiding this comment.
네 맞습니다ㅎㅎ 일급컬렉션은 여러개를 가지고 있는 형태를 묶어 놓은 것인데 의미가 다른 것 같아서요.
db에서 Order가 예약어 이지만 `를 사용하면 Order테이블을 만들어 사용할 수 있습니다ㅎㅎ
이 부분은 다른 방법을 더 고민해봐도 좋을 것 같습니다
| return findOrderResponseDtoByOrderId(orderId); | ||
| private void reduceStockQuantity(List<Cart> carts) { | ||
| for (Cart cart : carts) { | ||
| final Product product = productDao.findById(cart.getProduct().getId()); |
There was a problem hiding this comment.
모든 product를 찾은 후에 업데이트를 하고 있는데요.
cart에 있는 모든 product를 한번에 업데이트할 수도 있지 않을까 하는 생각이 드네요ㅎㅎ
There was a problem hiding this comment.
쿼리문에서 UPDATE문과 서브쿼리(SELECT)를 이용해서 카트의 productId와 같은 product들을 찾아와 product.stock_quantity - cart_item.quantity 로 얻은 값을 product에 update해주는 식으로 쿼리 하나로 한 번에 업데이트 해주도록 수정해주었습니다...!!
그런데 질문이 있습니다..음..현재 저는 Product 와 관련된 사항이 변경되는 것이기 때문에 ProductDao에 updateStockQuantity라는 메소드를 두었습니다.
그런데 어떻게 보면 "주문" 이라는 비즈니스 로직과 관련된 내용이기 때문에 해당 로직이 `OrderDao에 존재해야하나하는 의문이 들었습니다..! 혹시 로운은 이와 관련하여 어떻게 생각하시나요..?? 혹시 이를 나누는 기준같은게 있을까요..??😅
There was a problem hiding this comment.
dao 대신 service의 사용과 좀 겹칠 수 있을 것 같은데요.
service가 비지니스로직을 담당하기 때문에 db와 관련된 부분도 같이 관리를 한다고 생각해요.
실제로 db에 crud 하는 것들도 비지니스 로직에 포함된 부분이니까요.
그렇기 때문에 각 도메인을 담당하는 서비스가 생겼다면 그 도메인과 관련된 부분은 서비스를 통해 메세지를 전달하는 방식으로 해야하지 않을까요?
저는 개인적으로 service도 객체지향적으로 생각해야한다고 생각해요.
우리가 객체의 내부 데이터나 로직을 모른 상태로 메세지만 전달하여 로직을 수행하도록 하는 것처럼 service도 이와 같이 해야한다고 생각합니다!
서비스 혹은 layer도 객체로 본다면 각각의 역할을 무엇으로 생각했는지에 맞춰서 개발해야하지 않나 생각하거든요~
여기서 저는 db와 관련된 부분을 담당하는 service를 작은 단위의 service로 생각을 했던거 같아요.
entity가 필요한 모든 데이터를 가지고 있으면 비지니스 로직을 수행해도 괜찮다고 생각하지만 개인적으로 확실하게 entity와 domain이 일치한다고 생각하지 않아요.
도메인이 여러개의 entity를 필요로 한다면 큰 의미의 도메인이 된다고 생각할 수도 있지 않을까요? entity의 모든 column이 필요한 것이 아닌 A entity에서는 a,b,c column만 필요하고 B entity에서는 d,e만 필요한 도메인이 생길 수도 있지 않을까요? 이때 두 entity로 로직을 수행할지 도메인으로 변환해서 수행할지, entity를 완전한 domain으로 볼지 등등 개발자마다 의견이 다 다르다고 생각해요.
저의 의견으로는 order가 큰 의미의 도메인이고 product가 작은 의미의 도메인이 될텐데요.
해당 값이 product 도메인 내에서 관리를 하기 때문에 product service의 메서드를 호출하도록 했을 것 같습니다~
현재의 구조에서는 디우와 같이 productDao에서 하도록 했을 것 같네요
| @@ -21,48 +24,93 @@ public class CartService { | |||
| private final CustomerDao customerDao; | |||
There was a problem hiding this comment.
customerService가 있는데 customerDao를 사용한 이유가 있을까요?
There was a problem hiding this comment.
이 부분에 대해서도 크루들 사이에서 많은 의견이 오갔었습니다. 따로 정리하지는 않아서..ㅠㅠ 기억이 나는대로 내용을 정리해보면 다음과 같습니다!🙂
우선 Service에서 Service를 의존해도 된다는 입장의 가장 큰 이유는 동일한 비즈니스 로직을 작성해줄 필요가 없다였던 것으로 기억합니다.
예를 들어서 CartService에서 CustomerService에 의존하고 findByEmail를 호출하고 이 때 유저에 대한 검증이 필요하다면, 이메일 검증등과 같은 비즈니스 로직에 대한 처리를 또 다시 CartService에서 해줄 필요가 없어지게 됩니다. 물론 CartService의 책임도 아니라고 생각합니다. 하지만 Service계층 끼리의 순환참조등과 같은 부분에 대해서는 주의가 필요하다고 생각합니다.
반대로 Service에서 Dao에 의존하는 것은 우선 의존 관계의 일관성이 생깁니다. 모두 Service -> Dao로의 의존을 가지게 됩니다. 하지만 앞서 말씀드린 것과 같이 이미 작성되어있는 비즈니스 로직 처리에 대해서 또 다시 처리를 해주어야할 수 있습니다.
제가 DAO들에만 의존하도록 한 이유는 우선 의존 관계의 일관성에 있습니다. 순환 참조등에 대해서 고려할 필요가 없습니다. 또한 현재 미션들을 수행하면서는 하나의 트랜잭션과 관련된 복잡한 비즈니스 로직 처리가 필요한 경우보다는 단순 데이터 조회 혹은 저장과 과련된 부분이 많았다고 생각이 듭니다. 즉, 이번 미션을 포함하여 DAO에대한 의존을 하여 데이터 조회 혹은 저장 등과 같은 기능만을 필요로 하지, Service 계층에 대한 의존이 불필요했다고 생각합니다!
하지만 저도 이 둘에 대해서 정답은 없다고 생각되고, 만약 비즈니스 로직이 더욱 복잡해진다고 하고, 각 서비스 클래스 마다 중복되는 로직이 많아진다라고 하면 Service에 대한 의존도 고려해볼 것 같습니다.😁
| productId1 = 상품_등록되어_있음("치킨", 10_000, "http://example.com/chicken.jpg"); | ||
| productId2 = 상품_등록되어_있음("맥주", 20_000, "http://example.com/beer.jpg"); | ||
| final ProductRequest chickenRequest = | ||
| new ProductRequest("치킨", 10_000, 1_000, CHICKEN_IMAGE); |
There was a problem hiding this comment.
product에 대한 것들도 중복이 많은 것 같은데 이것도 fixture로 활용할 수 있지 않을까요?
|
|
||
| import woowacourse.shoppingcart.domain.Image; | ||
|
|
||
| public class ThumbnailImage { |
There was a problem hiding this comment.
오..! 맞네요...저는 DTO라고 생각을 하고 네이밍을 지었다보니 별다른 생각을 못했었는데, 해당 네이밍을 처음 보는 사람입장에서는 로운처럼 도메인이라고 생각할 것 같습니다...부주의했네요😅
음..우선 네이밍에 Request나 Response를 붙여 요청객체, 응답객체와 같이 DTO임을 나타내주지 않은 이유는 요청과 응답 모두에서 사용되는 DTO라는 생각이 들었기 때문입니다.
음..우선은 DTO라는 것을 강조해주기 위해 ThumbnailImage -> ThumbnailImageDto와 같이 네이밍을 해줄 수 있을 것 같습니다!!
수정해보도록 하겠습니다!!🙂
정적 팩토리 메소드를 사용해서 생성자는 "객체 생성"과 관련된 책임만을 가지도록 수정
| return findOrderResponseDtoByOrderId(orderId); | ||
| private void reduceStockQuantity(List<Cart> carts) { | ||
| for (Cart cart : carts) { | ||
| productDao.updateStockQuantity(cart.getId(), cart.getProduct().getId()); |
There was a problem hiding this comment.
개인적인 의견입니다~
저는 quantity계산은 server에서 했을 것 같아요.
Map<Long, Cart> productCarts;
List<Porduct> products = productDao.findIds();
for (Product product : products) {
product.updateQuantity(quantity);
}
productDao.batchUpdate(products);매우 간단하게 표현해봤는데요. 이런식으로 server에서 하지 않았을까 싶습니다~
안녕하세요 로운!! 디우입니다😁
1단계 제출하고 꽤 시간이 많이 지났네요ㅠ.ㅠ
생각보다 배포 및 프런트와의 회의 과정 등이 시간이 많이 걸려서 이전에 말씀드린 것 중에서 제외된 부분이 많습니다..ㅠ.ㅠ
이번에도 프런트팀과 어떤 내용을 고민하였고, 어떻게 구현하였는지에 대해서 정리하여 조금 더 질 좋은 리뷰가 오갈 수 있도록 정리해보았습니다!!
데모 페이지
프런트에서 데모 페이지를 함께 배포해주었는데, 현재 제가 배포한 서버를 사용중입니다. 혹시나 도메인이 다른 사이트로 연결된다면, 크롬 캐시를 비운 후에 접속 하시면 가능합니다!😃
API 명세
2단계 요구사항에 대해서 front와 논의하여 도출한 API 명세에 대한 정리를 Notion Page에 이전과 같이 정리해두었으며,
API.md파일에도 정리해두었습니다. 또한 Swagger 설정을 추가하여 문서자동화를 해보았습니다.(굉장히 편리하다는 느낌을 받았습니다.🙂)에러 코드
이번 2단계 요구사항을 진행하면서 custom error code 또한 추가하게 되었는데요, 해당 과정에서 많은 부분을 느끼게 되었던 것 같습니다. Customer Error Code 정리 엑셀
가장 먼저
여기서 더 프로젝트의 규모가 커진다면 과연 우리가 해당 문서를 계속해서 유지보수 할 수 있겠는가?라는 질문에 자신있게 "네!" 라고 말하지 못할 것 같다는 생각을 하였습니다. 현업에서는 더더욱 그러지 못할 것 같다는 생각을 하였습니다. 또한 신입사원이 공부해야할 내용도 많아지고 새로운 에러 상황이 이미 등록되어 있는지 없는지 체크하며 추가해나갈 자신이 없었습니다. 또한 이전에 말씀드렸던 것과 같은 장점도 무색해진다는 느낌을 받았습니다.Password Encoder
이전에 자신있게 Spring Security에서 지원하는 비밀번호 암호화 인터페이스인 Password Encoder를 공부하고 적용해본다고 말씀드렸었는데요...배포까지 하려니 일정이 빡빡하고, 테코톡 발표도 하다보니 생각보다 여유가 생기질 않아서...아직 적용해보지 못했습니다..🥲 하지만 레벨3 프로젝트를 진행할 때에는 꼭 알고 있어야 하는 개념이라고 생각되므로 공부를 하고, 해당 branch에 적용하면서 익히도록 하겠습니다...!!
레거시 코드
레거시 코드를 개선해보는 경험을 이번 장바구니 미션에서 처음 해보게 되었는데요..생각보다 하나를 고쳤을 때 테스트 깨지는 부분들이 많아서 어떻게하면 좋을지에 대한 고민이 들었습니다..
예를 들어 Product 도메인에 대해서만 먼저 개선하고 싶은데, DAO, Service, Controller, 또 장바구니 같은 경우 다른
주문쪽 까지 고려하면서 리팩토링 해야 하더라구요...이러 부분에서 어려움을 겪었습니다.그런데 현업에 가면 레거시 코드도 많이 보고 리팩토링하는 경험도 많이 가지게 될 것이고, 지금보다 규모가 더 커질 것으로 생각되는데요...혹시 로운만의 팁이 있을지..공유해주시면 너무 감사할 것 같습니다..!!
application.yml
application.yml 을 분리해주었습니다. 운영환경과 로컬 개발 환경 그리고 테스트 환경을 분리해주었습니다.
테스트환경과 로컬 개발 환경이 동일하기는 하지만, 혹시나 다를 수도 있기 때문에 분리해주었습니다.
또 운영환경에서 사용될 설정에 대해서는 application-prod.yml을 사용하도록 하고 OS의 환경 변수를 이용하여 값을 넣어줄 수 있도록 설정을 구성하였습니다.
1단계 때 생각을 정리할 수 있는 질문을 주셔서 스스로 정리를 해볼 수 있어서 너무 좋은 경험이었습니다..!!
또한 로운의 생각도 공유해주셔서 큰 도움이 되었다고 생각합니다!!😃
이번 2단계 리뷰도 잘 부탁드립니다!🙇♂️