Conversation
|
""" Walkthrough이번 변경 사항에서는 의존성 관리 및 새로운 기능 확장을 위해 다양한 모듈에 수정이 이루어졌습니다. 빌드 스크립트에 Spring Data JPA와 Lombok 관련 의존성이 추가되었으며, 장바구니 관리를 위한 서비스, 컨트롤러, DTO, 엔티티와 리포지토리가 신규로 추가되었습니다. 또한, 각 기능별 단위 테스트가 보강되어 기존 기능과 신규 기능의 동작을 검증합니다. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant CC as CartController
participant CS as CartService
participant CR as CartRepository
C->>CC: GET /cart
CC->>CS: getCart(cartId)
CS->>CR: findById(cartId)
CR-->>CS: Cart 객체 반환
CS-->>CC: Cart 객체 반환
CC-->>C: CartResponseDto 전송
C->>CC: DELETE /cart
CC->>CS: clearCart(cartId)
CS->>CR: 업데이트/전체 삭제 처리
CS-->>CC: 처리 결과
CC-->>C: 204 No Content 응답
sequenceDiagram
participant C as Client
participant LIC as LineItemController
participant CS as CartService
participant CR as CartRepository
C->>LIC: POST /cart/line-items (DTO 포함)
LIC->>CS: addItemToCart(cartId, productId, option, quantity)
CS->>CR: Cart 조회 및 업데이트
CR-->>CS: 업데이트된 Cart 반환
CS-->>LIC: 처리 결과
LIC-->>C: 성공 응답
C->>LIC: DELETE /cart/line-items/{lineItemId}
LIC->>CS: removeLineItem(cartId, lineItemId)
CS->>CR: 라인 아이템 제거 처리
CS-->>LIC: 처리 결과
LIC-->>C: 성공 응답
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @moonnahye, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
This pull request implements the basic functionality for creating and managing a shopping cart. It includes the necessary models, repositories, services, and controllers to add, remove, and view items in a cart. The changes include adding JPA dependencies and Lombok, creating model classes for Cart, LineItem, Product, ProductId, and ProductOption, creating repositories for Cart and Product, implementing the CartService for managing cart operations, creating controllers for handling cart and line item requests, and adding DTOs for request and response objects. Unit tests are also included to verify the functionality of the CartService and controllers.
Highlights
- Dependencies: Added Spring Data JPA and Lombok dependencies to the project.
- Models: Created model classes for Cart, LineItem, Product, ProductId, and ProductOption with appropriate JPA annotations.
- Repositories: Created CartRepository and ProductRepository interfaces extending JpaRepository.
- Cart Service: Implemented CartService with methods for adding items to the cart, removing items from the cart, clearing the cart, and retrieving cart details.
- Controllers: Created CartController and LineItemController to handle REST API requests for cart management.
- DTOs: Added CartResponseDto and LineItemRequestDto/ResponseDto for data transfer between the client and server.
- Unit Tests: Added unit tests for CartService and controllers to ensure proper functionality.
Changelog
Click here to see the changelog
- build.gradle.kts
- Added
spring-boot-starter-data-jpadependency. - Added
lombokdependency for compileOnly and annotationProcessor. - Added
lombokdependency for testCompileOnly and testAnnotationProcessor.
- Added
- src/main/java/com/example/demo/application/CartService.java
- Created CartService class.
- Added methods: addItemToCart, removeItemFromCart, clearCart, getCart.
- Implemented transactional annotations.
- src/main/java/com/example/demo/controllers/CartController.java
- Created CartController class.
- Added endpoint to get cart details by ID (
/carts/{cartId}). - Added endpoint to delete/clear a cart by ID (
/carts/{cartId}).
- src/main/java/com/example/demo/controllers/LineItemController.java
- Created LineItemController class.
- Added endpoint to create a line item in a cart (
/carts/{cartId}/line-items). - Added endpoint to delete a line item from a cart (
/carts/{cartId}/line-items/{lineItemId}).
- src/main/java/com/example/demo/controllers/dto/CartResponseDto.java
- Created CartResponseDto class to represent cart details.
- src/main/java/com/example/demo/controllers/dto/LineItemRequestDto.java
- Created LineItemRequestDto class to handle line item creation requests.
- src/main/java/com/example/demo/controllers/dto/LineItemResponseDto.java
- Created LineItemResponseDto class to represent line item details.
- src/main/java/com/example/demo/model/Cart.java
- Created Cart entity with JPA annotations.
- Defined relationships with LineItem.
- Implemented methods for adding products, calculating total quantity, and clearing items.
- src/main/java/com/example/demo/model/LineItem.java
- Created LineItem entity with JPA annotations.
- Defined relationships with Cart.
- Implemented methods for adding quantity and checking if it's the same product.
- src/main/java/com/example/demo/model/LineItemId.java
- Created LineItemId embeddable class to represent the ID of a LineItem.
- src/main/java/com/example/demo/model/Product.java
- Created Product entity with JPA annotations.
- src/main/java/com/example/demo/model/ProductId.java
- Created ProductId embeddable class to represent the ID of a Product.
- src/main/java/com/example/demo/model/ProductOption.java
- Created ProductOption embeddable class to represent the options of a Product.
- src/main/java/com/example/demo/repository/CartRepository.java
- Created CartRepository interface extending JpaRepository.
- src/main/java/com/example/demo/repository/ProductRepository.java
- Created ProductRepository interface extending JpaRepository.
- src/test/java/com/example/demo/application/CartServiceTest.java
- Created unit tests for CartService methods.
- src/test/java/com/example/demo/controllers/CartControllerTest.java
- Created unit tests for CartController endpoints.
- src/test/java/com/example/demo/controllers/LineItemControllerTest.java
- Created unit tests for LineItemController endpoints.
- src/test/java/com/example/demo/model/CartTest.java
- Created unit tests for Cart model.
- src/test/java/com/example/demo/model/LineItemIdTest.java
- Created unit tests for LineItemId model.
- src/test/java/com/example/demo/model/LineItemTest.java
- Created unit tests for LineItem model.
- src/test/java/com/example/demo/model/ProductIdTest.java
- Created unit tests for ProductId model.
- src/test/java/com/example/demo/model/ProductOptionTest.java
- Created unit tests for ProductOption model.
- src/test/java/com/example/demo/model/ProductTest.java
- Created unit tests for Product model.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
A cart's creation,
A digital cornucopia,
Add, delete, enjoy.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
The pull request introduces cart functionality to the application. Overall, the code is well-structured and implements the core features of a shopping cart. However, there are a few areas that could be improved for better maintainability and robustness.
Summary of Findings
- Exception Handling: The service layer throws
IllegalArgumentExceptionwhen a cart or product is not found. Consider using more specific exceptions or custom exceptions to provide better context and allow for more granular error handling in the controller layer. - Data Transfer Objects (DTOs): The DTOs are simple data containers. Consider using validation annotations to ensure data integrity at the controller level.
- Test Coverage: While the tests cover the basic functionality, consider adding more test cases to cover edge cases and error scenarios, especially around quantity limits and item removal.
Merge Readiness
The pull request is almost ready for merging. Addressing the exception handling and adding more comprehensive tests would significantly improve the robustness and maintainability of the code. I am unable to approve this pull request, and recommend that another reviewer approves this code before merging. I recommend that the pull request not be merged until the high severity issues are addressed.
| Cart cart = cartRepository.findById(cartId) | ||
| .orElseThrow(() -> new IllegalArgumentException("장바구니가 존재하지 않습니다.")); |
There was a problem hiding this comment.
Consider using a custom exception instead of IllegalArgumentException to provide more context about the error. This allows for more specific error handling in the controller layer.
.orElseThrow(() -> new CartNotFoundException("장바구니가 존재하지 않습니다."));
There was a problem hiding this comment.
I'm glad you agree with the suggestion! Using a custom exception can indeed improve the clarity and maintainability of the code.
| Product product = productRepository.findById(productId) | ||
| .orElseThrow(() -> new IllegalArgumentException("상품이 존재하지 않습니다.")); |
There was a problem hiding this comment.
Consider using a custom exception instead of IllegalArgumentException to provide more context about the error. This allows for more specific error handling in the controller layer.
.orElseThrow(() -> new ProductNotFoundException("상품이 존재하지 않습니다."));There was a problem hiding this comment.
Glad to hear that suggestion was helpful!
| if (totalQuantity > 20) { | ||
| throw new IllegalArgumentException("담을수 있는 수량을 초과했습니다."); |
There was a problem hiding this comment.
Consider using a custom exception instead of IllegalArgumentException to provide more context about the error. This allows for more specific error handling in the controller layer.
throw new CartQuantityLimitException("담을수 있는 수량을 초과했습니다.");There was a problem hiding this comment.
Genimi의 지적이 올바릅니다. 4주차 때의 예제 문서를 보시면 아시겠지만, 이 예외가 도메인 모델링할 때 만들어져야 합니다.
| public void create(@PathVariable Long cartId, @RequestBody LineItemRequestDto requestDto) { | ||
|
|
||
| cartService.addItemToCart( | ||
| cartId, | ||
| new ProductId(requestDto.getProductId()), | ||
| new ProductOption(requestDto.getColor(), requestDto.getSize()), | ||
| requestDto.getQuantity()); |
There was a problem hiding this comment.
It's good practice to validate the request data to ensure data integrity. Consider adding validation annotations to the LineItemRequestDto and using @Valid in the controller to trigger the validation.
@PostMapping
@ResponseStatus(HttpStatus.CREATED)
public void create(@PathVariable Long cartId, @Valid @RequestBody LineItemRequestDto requestDto) {| @Test | ||
| @DisplayName("상품 수량이 20개가 초과하면 예외가 발생한다.") | ||
| void throwExceptionWhenTotalQuantityOverLimit() { | ||
| // given | ||
| mockCartAndProduct(); | ||
| cartService.addItemToCart(cartId, productId, option, 20); | ||
|
|
||
| // when & then | ||
| assertThatThrownBy(()-> | ||
| cartService.addItemToCart(cartId, productId, option, 1) | ||
| ).isInstanceOf(IllegalArgumentException.class); |
There was a problem hiding this comment.
This test case checks the exception when the total quantity exceeds the limit. Consider adding a test case to verify that the correct exception is thrown when attempting to add a quantity that would cause the total to exceed the limit, even if the initial quantity is less than the limit.
| @Test | |
| @DisplayName("상품 수량이 20개가 초과하면 예외가 발생한다.") | |
| void throwExceptionWhenTotalQuantityOverLimit() { | |
| // given | |
| mockCartAndProduct(); | |
| cartService.addItemToCart(cartId, productId, option, 20); | |
| // when & then | |
| assertThatThrownBy(()-> | |
| cartService.addItemToCart(cartId, productId, option, 1) | |
| ).isInstanceOf(IllegalArgumentException.class); | |
| @Test | |
| @DisplayName("상품 수량이 20개가 초과하면 예외가 발생한다. (초과 수량 추가)") | |
| void throwExceptionWhenAddingQuantityOverLimit() { | |
| // given | |
| mockCartAndProduct(); | |
| cartService.addItemToCart(cartId, productId, option, 15); | |
| // when & then | |
| assertThatThrownBy(()-> | |
| cartService.addItemToCart(cartId, productId, option, 6) | |
| ).isInstanceOf(IllegalArgumentException.class); | |
| } |
| if (lineItemId == null) return; | ||
| lineItems.removeIf(lineItem ->lineItem.getId().equals(lineItemId)); |
There was a problem hiding this comment.
There is a potential issue here: if lineItemId is not found in lineItems, the removeIf method will not remove anything, but the calculateTotalQuantity() method will still be called. This is not a correctness issue, but it could be slightly more efficient to check if the item was actually removed before recalculating the total quantity. Also, consider logging when a lineItemId is not found, as this could indicate a data inconsistency.
if (lineItemId == null) return;
boolean removed = lineItems.removeIf(lineItem ->lineItem.getId().equals(lineItemId));
if (removed) {
calculateTotalQuantity();
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (23)
src/main/java/com/example/demo/controllers/dto/LineItemRequestDto.java (1)
3-31: Lombok을 활용하여 보일러플레이트 코드를 줄일 수 있습니다.DTO 클래스가 잘 구현되었지만, PR 요약에 언급된 Lombok 종속성을 활용하여 코드를 더 간결하게 만들 수 있습니다. 또한 입력 검증 어노테이션도 추가하면 좋을 것 같습니다.
package com.example.demo.controllers.dto; +import lombok.AllArgsConstructor; +import lombok.Getter; +import jakarta.validation.constraints.Min; +import jakarta.validation.constraints.NotNull; + +@Getter +@AllArgsConstructor public class LineItemRequestDto { + @NotNull private String productId; + @NotNull private String color; + @NotNull private String size; + @Min(1) private int quantity; - - public LineItemRequestDto(String productId, String color, String size, int quantity) { - this.productId = productId; - this.color = color; - this.size = size; - this.quantity = quantity; - } - - public String getProductId() { - return productId; - } - - public String getColor() { - return color; - } - - public String getSize() { - return size; - } - - public int getQuantity() { - return quantity; - } }src/main/java/com/example/demo/controllers/dto/LineItemResponseDto.java (1)
3-32: Lombok 활용을 고려해 보세요.현재 DTO 클래스가 전통적인 방식으로 getter 메서드를 모두 수동으로 구현하고 있습니다. Lombok 라이브러리의
@Getter,@AllArgsConstructor어노테이션을 사용하거나 Java 레코드를 활용하면 코드를 더 간결하게 만들 수 있습니다.package com.example.demo.controllers.dto; +import lombok.Getter; +import lombok.AllArgsConstructor; +@Getter +@AllArgsConstructor public class LineItemResponseDto { private String productId; private String color; private String size; private int quantity; - public LineItemResponseDto(String productId, String color, String size, int quantity) { - this.productId = productId; - this.color = color; - this.size = size; - this.quantity = quantity; - } - - public String getProductId() { - return productId; - } - - public String getColor() { - return color; - } - - public String getSize() { - return size; - } - - public int getQuantity() { - return quantity; - } }또는 레코드를 사용한 버전:
package com.example.demo.controllers.dto; -public class LineItemResponseDto { - - private String productId; - private String color; - private String size; - private int quantity; - - public LineItemResponseDto(String productId, String color, String size, int quantity) { - this.productId = productId; - this.color = color; - this.size = size; - this.quantity = quantity; - } - - public String getProductId() { - return productId; - } - - public String getColor() { - return color; - } - - public String getSize() { - return size; - } - - public int getQuantity() { - return quantity; - } -} +public record LineItemResponseDto( + String productId, + String color, + String size, + int quantity +) {}src/main/java/com/example/demo/controllers/dto/CartResponseDto.java (1)
5-27: Lombok 사용과 컬렉션 방어적 복사를 고려해 보세요.
- LineItemResponseDto와 마찬가지로 Lombok 라이브러리나 Java 레코드를 활용하면 코드를 더 간결하게 만들 수 있습니다.
getLineItems()메서드는 내부 컬렉션을 그대로 반환하고 있어 외부에서 수정될 가능성이 있습니다. 방어적 복사를 통해 불변성을 보장하는 것이 좋습니다.package com.example.demo.controllers.dto; import java.util.List; +import java.util.ArrayList; +import lombok.Getter; public class CartResponseDto { private Long cartId; private int totalQuantity; private List<LineItemResponseDto> lineItems; public CartResponseDto(Long cartId, int totalQuantity, List<LineItemResponseDto> lineItems) { this.cartId = cartId; this.totalQuantity = totalQuantity; - this.lineItems = lineItems; + this.lineItems = new ArrayList<>(lineItems); // 방어적 복사 } public Long getCartId() { return cartId; } public int getTotalQuantity() { return totalQuantity; } public List<LineItemResponseDto> getLineItems() { - return lineItems; + return new ArrayList<>(lineItems); // 방어적 복사 } }또는 Java 레코드와 불변 컬렉션을 사용한 버전:
package com.example.demo.controllers.dto; import java.util.List; +import java.util.Collections; -public class CartResponseDto { - private Long cartId; - private int totalQuantity; - private List<LineItemResponseDto> lineItems; - - public CartResponseDto(Long cartId, int totalQuantity, List<LineItemResponseDto> lineItems) { - this.cartId = cartId; - this.totalQuantity = totalQuantity; - this.lineItems = lineItems; - } - - public Long getCartId() { - return cartId; - } - - public int getTotalQuantity() { - return totalQuantity; - } - - public List<LineItemResponseDto> getLineItems() { - return lineItems; - } -} +public record CartResponseDto( + Long cartId, + int totalQuantity, + List<LineItemResponseDto> lineItems +) { + public CartResponseDto(Long cartId, int totalQuantity, List<LineItemResponseDto> lineItems) { + this.cartId = cartId; + this.totalQuantity = totalQuantity; + this.lineItems = Collections.unmodifiableList(new ArrayList<>(lineItems)); + } + + @Override + public List<LineItemResponseDto> lineItems() { + return lineItems; // 이미 불변 리스트이므로 그대로 반환 + } +}src/test/java/com/example/demo/model/LineItemIdTest.java (2)
7-8: 사용하지 않는 import 문이 있습니다.JUnit의 Assertions는 사용되지 않으므로 제거하는 것이 좋습니다.
import static org.assertj.core.api.Assertions.assertThat; -import static org.junit.jupiter.api.Assertions.*;
29-30: 불필요한 빈 줄이 있습니다.코드의 가독성을 위해 불필요한 빈 줄은 제거하는 것이 좋습니다.
} - - }src/test/java/com/example/demo/model/ProductIdTest.java (2)
7-7: 사용하지 않는 import를 제거하세요.JUnit의
Assertions.*import는 코드에서 사용되지 않습니다. 사용하지 않는 import는 제거하는 것이 좋습니다.-import static org.junit.jupiter.api.Assertions.*;
11-18: 테스트 케이스가 일부만 다루고 있습니다.현재 테스트는 같은 값을 가진 ProductId 객체의 동등성만 확인하고 있습니다. 다른 값을 가진 객체의 비동등성도 테스트하는 것이 좋습니다.
@Test @DisplayName("다른 값이면 다른 객체로 취급된다.") void differentProductId() { ProductId id1 = new ProductId("productId-1"); ProductId id2 = new ProductId("productId-2"); assertThat(id1).isNotEqualTo(id2); }src/test/java/com/example/demo/model/ProductTest.java (2)
18-19: 불필요한 빈 줄을 제거하세요.메서드 시작 부분에 불필요한 빈 줄이 있습니다. 다른 메서드들과 일관성을 유지하기 위해 제거하는 것이 좋습니다.
@Test -void changeProductName(){ - +void changeProductName(){
17-32: 테스트 케이스가 기본 기능만 다루고 있습니다.현재 테스트는 기본적인 이름과 가격 변경 기능만 테스트하고 있습니다. 예외 케이스나 경계값 테스트도 추가하는 것이 좋습니다. 예를 들어, 빈 문자열이나 null 값을 이름으로 설정하거나 음수 가격을 설정하는 경우 등을 테스트할 수 있습니다.
@Test @DisplayName("상품 이름은 비어있을 수 없다.") void productNameCannotBeEmpty() { assertThatThrownBy(() -> product.changeName("")) .isInstanceOf(IllegalArgumentException.class); assertThatThrownBy(() -> product.changeName(null)) .isInstanceOf(IllegalArgumentException.class); } @Test @DisplayName("상품 가격은 음수가 될 수 없다.") void productPriceCannotBeNegative() { assertThatThrownBy(() -> product.changePrice(-1000)) .isInstanceOf(IllegalArgumentException.class); }src/test/java/com/example/demo/model/LineItemTest.java (1)
11-25: 같은 상품과 옵션에 대한 수량 추가 테스트가 잘 구현되었습니다.같은 상품과 옵션일 경우 수량이 올바르게 추가되는지 검증하는 테스트가 명확하게 구현되었습니다. 메소드명과 중괄호 사이에 공백을 추가하면 가독성이 더 좋아질 것 같습니다.
-void isSameProduct(){ +void isSameProduct() {src/test/java/com/example/demo/model/CartTest.java (1)
80-93: 상품 제거 기능에 대한 테스트 추가를 고려해보세요.장바구니에서 상품을 제거하는 기능에 대한 테스트가 누락된 것 같습니다. Cart 클래스에 상품 제거 메소드가 있다면 해당 기능을 검증하는 테스트도 추가하는 것이 좋겠습니다.
src/main/java/com/example/demo/model/Product.java (1)
14-15: 필드 정의가 간단하고 이해하기 쉽습니다.
String name과int price필드가 충분히 직관적이며, 추가 검증 로직이 필요한 경우 도메인 요구사항에 따라 확장해 볼 수 있습니다.src/test/java/com/example/demo/controllers/CartControllerTest.java (1)
28-36: 응답 본문에 대한 검증이 추가되면 좋겠습니다.
현재 상태 코드만 확인하고 있어, 실제로 반환되는 JSON 구조나 필드값도 테스트에 포함하면 테스트 신뢰도가 향상됩니다.mockMvc.perform(get("/carts/{cartId}", cartId)) - .andExpect(status().isOk()); + .andExpect(status().isOk()) + .andExpect(jsonPath("$.cartId").value(cartId)) + .andExpect(jsonPath("$.totalQuantity").value(cart.getTotalQuantity()));src/main/java/com/example/demo/controllers/CartController.java (1)
23-40: 장바구니 조회 로직에서 예외 처리를 고려해 보세요.
cartService에서 장바구니가 없을 때 예외를 던지는 로직이 있을 것으로 보이는데, 컨트롤러 단계에서 적절한 예외 핸들링을 추가하여 사용자에게 명확한 오류 응답을 줄 수 있습니다.src/main/java/com/example/demo/model/Cart.java (4)
45-61: addProduct 메서드의 중복 코드 리팩토링 고려
calculateTotalQuantity()와checkValidQuantity()메서드 호출이 중복되어 있습니다. 이 코드를 리팩토링하여 중복을 제거하는 것이 좋겠습니다.public void addProduct(ProductId productId, ProductOption productOption, int quantity) { LineItem lineItem = findLineItem(productId, productOption); if (lineItem != null) { lineItem.addQuantity(quantity, productOption); - calculateTotalQuantity(); - checkValidQuantity(); - return; + updateCartState(); + return; } lineItem = new LineItem(productId, productOption, quantity); lineItems.add(lineItem); lineItem.setCart(this); - calculateTotalQuantity(); - checkValidQuantity(); + updateCartState(); } + private void updateCartState() { + calculateTotalQuantity(); + checkValidQuantity(); + }
63-67: checkValidQuantity 메서드의 예외 처리 개선현재 IllegalArgumentException을 사용하여 도메인 검증 오류를 처리하고 있습니다. 더 명시적인 커스텀 예외(예: CartQuantityExceededException)를 사용하는 것을 고려해보세요. 이를 통해 예외 처리가 더 명확해지고 클라이언트에서 대응하기 쉬워집니다.
76-82: findLineItem 메서드의 람다 표현식에 공백 추가가독성을 높이기 위해 람다 표현식에서 쉼표 뒤에 공백을 추가하는 것이 좋습니다.
private LineItem findLineItem(ProductId productId, ProductOption productOption) { return lineItems.stream() .filter(lineItem -> - lineItem.isSameProduct(productId,productOption)) + lineItem.isSameProduct(productId, productOption)) .findFirst() .orElse(null); }
89-93: removeLineItemById 메서드의 람다 표현식에 공백 추가가독성을 높이기 위해 람다 표현식에서 화살표 뒤에 공백을 추가하는 것이 좋습니다.
public void removeLineItemById(LineItemId lineItemId) { if (lineItemId == null) return; - lineItems.removeIf(lineItem ->lineItem.getId().equals(lineItemId)); + lineItems.removeIf(lineItem -> lineItem.getId().equals(lineItemId)); calculateTotalQuantity(); }src/main/java/com/example/demo/controllers/LineItemController.java (1)
37-42: 예외 처리 추가 고려현재 컨트롤러에서는 서비스 계층에서 발생할 수 있는 예외에 대한 처리가 없습니다.
@ExceptionHandler를 사용하거나 전역 예외 처리기를 통해 예외를 처리하는 로직을 추가하는 것이 좋겠습니다.src/main/java/com/example/demo/application/CartService.java (4)
34-39: 중복 코드 제거를 위한 메서드 활용카트 조회 로직이 중복되어 있습니다. 이전 리팩토링에서 추가한
findCartById메서드를 사용하여 중복을 제거하세요.public void removeItemFromCart(Long cartId, LineItemId lineItemId) { - Cart cart = cartRepository.findById(cartId) - .orElseThrow(() -> new IllegalArgumentException("장바구니가 존재하지 않습니다.")); + Cart cart = findCartById(cartId); cart.removeLineItemById(lineItemId); }
41-46: clearCart 메서드에서도 중복 코드 제거마찬가지로 카트 조회 로직이 중복되어 있습니다.
findCartById메서드를 사용하세요.public void clearCart(Long cartId) { - Cart cart = cartRepository.findById(cartId) - .orElseThrow(() -> new IllegalArgumentException("장바구니가 존재하지 않습니다.")); + Cart cart = findCartById(cartId); cart.clearItems(); }
48-52: getCart 메서드도 findCartById 활용읽기 전용 트랜잭션을 사용하는 이 메서드도 중복 코드를 제거할 수 있습니다.
@Transactional(readOnly = true) public Cart getCart(Long cartId) { - return cartRepository.findById(cartId) - .orElseThrow(() -> new IllegalArgumentException("장바구니가 존재하지 않습니다.")); + return findCartById(cartId); }
25-52: 커스텀 예외 클래스 사용 고려도메인 특화 예외를 위해
IllegalArgumentException대신 커스텀 예외 클래스를 사용하는 것이 좋습니다. 이를 통해 예외 처리가 더 명확해지고 클라이언트에서 대응하기 쉬워집니다.다음과 같은 커스텀 예외 클래스를 추가할 수 있습니다:
public class CartNotFoundException extends RuntimeException { public CartNotFoundException(Long cartId) { super("장바구니가 존재하지 않습니다. ID: " + cartId); } } public class ProductNotFoundException extends RuntimeException { public ProductNotFoundException(ProductId productId) { super("상품이 존재하지 않습니다. ID: " + productId); } }그리고 이를 다음과 같이 활용할 수 있습니다:
private Cart findCartById(Long cartId) { return cartRepository.findById(cartId) - .orElseThrow(() -> new IllegalArgumentException("장바구니가 존재하지 않습니다.")); + .orElseThrow(() -> new CartNotFoundException(cartId)); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
build.gradle.kts(1 hunks)src/main/java/com/example/demo/application/CartService.java(1 hunks)src/main/java/com/example/demo/controllers/CartController.java(1 hunks)src/main/java/com/example/demo/controllers/LineItemController.java(1 hunks)src/main/java/com/example/demo/controllers/dto/CartResponseDto.java(1 hunks)src/main/java/com/example/demo/controllers/dto/LineItemRequestDto.java(1 hunks)src/main/java/com/example/demo/controllers/dto/LineItemResponseDto.java(1 hunks)src/main/java/com/example/demo/model/Cart.java(1 hunks)src/main/java/com/example/demo/model/LineItem.java(1 hunks)src/main/java/com/example/demo/model/LineItemId.java(1 hunks)src/main/java/com/example/demo/model/Product.java(1 hunks)src/main/java/com/example/demo/model/ProductId.java(1 hunks)src/main/java/com/example/demo/model/ProductOption.java(1 hunks)src/main/java/com/example/demo/repository/CartRepository.java(1 hunks)src/main/java/com/example/demo/repository/ProductRepository.java(1 hunks)src/test/java/com/example/demo/application/CartServiceTest.java(1 hunks)src/test/java/com/example/demo/controllers/CartControllerTest.java(1 hunks)src/test/java/com/example/demo/controllers/LineItemControllerTest.java(2 hunks)src/test/java/com/example/demo/model/CartTest.java(1 hunks)src/test/java/com/example/demo/model/LineItemIdTest.java(1 hunks)src/test/java/com/example/demo/model/LineItemTest.java(1 hunks)src/test/java/com/example/demo/model/ProductIdTest.java(1 hunks)src/test/java/com/example/demo/model/ProductOptionTest.java(1 hunks)src/test/java/com/example/demo/model/ProductTest.java(1 hunks)
🧰 Additional context used
🧬 Code Definitions (6)
src/test/java/com/example/demo/controllers/CartControllerTest.java (1)
src/test/java/com/example/demo/controllers/LineItemControllerTest.java (1)
WebMvcTest(21-69)
src/main/java/com/example/demo/controllers/CartController.java (2)
src/main/java/com/example/demo/controllers/dto/CartResponseDto.java (1)
CartResponseDto(5-27)src/main/java/com/example/demo/controllers/dto/LineItemResponseDto.java (1)
LineItemResponseDto(3-32)
src/main/java/com/example/demo/model/Product.java (2)
src/main/java/com/example/demo/model/LineItem.java (1)
Entity(11-71)src/main/java/com/example/demo/model/Cart.java (1)
Entity(15-94)
src/test/java/com/example/demo/controllers/LineItemControllerTest.java (1)
src/test/java/com/example/demo/controllers/CartControllerTest.java (1)
WebMvcTest(18-48)
src/main/java/com/example/demo/model/Cart.java (2)
src/main/java/com/example/demo/model/LineItem.java (1)
Entity(11-71)src/main/java/com/example/demo/model/Product.java (1)
Entity(7-42)
src/main/java/com/example/demo/model/LineItem.java (2)
src/main/java/com/example/demo/model/Cart.java (1)
Entity(15-94)src/main/java/com/example/demo/model/Product.java (1)
Entity(7-42)
🔇 Additional comments (32)
src/main/java/com/example/demo/repository/ProductRepository.java (1)
1-8: 표준 Spring Data JPA 인터페이스 구현 - 좋은 접근법입니다.
ProductRepository는 Spring Data JPA의 표준 패턴을 따라 JpaRepository를 확장함으로써 Product 엔티티에 대한 기본 CRUD 연산을 제공합니다. 코드가 간결하고 목적에 맞게 잘 구현되었습니다.src/main/java/com/example/demo/model/ProductId.java (1)
5-9: Java record를 활용한 깔끔한 값 객체 구현입니다.
@Embeddable어노테이션과 Java record를 사용하여 ProductId를 구현한 것은 좋은 접근법입니다. 불변 값 객체로서 ID를 표현하는 데 적합합니다.다만, 코드 스타일 측면에서 마지막 중괄호의 위치를 조금 수정하는 것이 일반적인 Java 코딩 스타일에 더 맞을 것 같습니다:
@Embeddable public record ProductId ( String id -){ -} +) {}src/main/java/com/example/demo/repository/CartRepository.java (1)
1-7: 표준 Spring Data JPA 인터페이스 구현이 잘 되었습니다.
CartRepository는 JpaRepository를 확장하여 Cart 엔티티에 대한 기본 CRUD 연산을 간결하게 제공합니다. 이 접근법은 Spring Data JPA의 모범 사례를 잘 따르고 있습니다.src/main/java/com/example/demo/model/ProductOption.java (1)
5-10: 레코드 타입을 활용한 좋은 설계입니다.@embeddable 어노테이션과 함께 Java의 레코드 타입을 사용하여 제품 옵션을 표현한 것은 좋은 접근 방식입니다. 레코드는 불변성을 보장하고 equals(), hashCode(), toString() 메서드를 자동으로 구현해 주므로 엔티티 내부에 임베디드 값으로 사용하기에 적합합니다.
src/test/java/com/example/demo/model/LineItemIdTest.java (1)
9-28: 테스트 케이스가 잘 구성되어 있습니다.LineItemId 클래스의 동등성 비교와 고유 ID 생성 기능을 검증하는 테스트 케이스가 잘 작성되어 있습니다. @DisplayName을 사용해 테스트의 목적을 명확히 설명한 것도 좋은 접근입니다.
src/main/java/com/example/demo/model/LineItemId.java (1)
7-15: 코드가 잘 작성되었습니다.LineItemId 클래스는 잘 구현되어 있습니다. @embeddable 어노테이션과 record 클래스를 사용하여 JPA에서 임베딩 가능한 값 객체를 효과적으로 구현했습니다. 또한 generate() 메서드를 통해 고유 식별자를 생성하는 방식도 적절합니다.
src/test/java/com/example/demo/model/ProductOptionTest.java (1)
8-27: 테스트가 잘 구현되어 있습니다.ProductOption 클래스의 동등성 검사를 위한 테스트 케이스가 잘 구현되어 있습니다. 같은 값과 다른 값을 가진 객체에 대한 동등성과 비동등성을 모두 테스트하고 있어 완전합니다.
build.gradle.kts (3)
19-19: Spring Data JPA 의존성 추가가 적절합니다.장바구니 기능을 구현하기 위한 데이터 접근 계층에 필요한 의존성입니다.
23-24: Lombok 의존성 설정이 올바르게 구성되었습니다.compileOnly와 annotationProcessor 설정이 올바르게 되어 있어 Lombok의 코드 생성 기능을 효과적으로 활용할 수 있습니다.
29-30: 테스트를 위한 Lombok 설정이 적절합니다.테스트 코드에서도 Lombok을 사용할 수 있도록 testCompileOnly와 testAnnotationProcessor가 올바르게 설정되었습니다.
src/test/java/com/example/demo/controllers/LineItemControllerTest.java (4)
3-6: 의존성 주입 및 import 문이 적절하게 수정되었습니다.CartService와 관련 모델 클래스들이 추가되었고, Mockito 관련 import가 와일드카드에서 구체적인 메소드로 변경되어 가독성이 향상되었습니다.
Also applies to: 15-16
26-27: CartService Mock 객체 주입이 적절합니다.LineItemController가 CartService에 의존하도록 변경되었으며, 테스트에서 이를 Mock으로 적절하게 처리하고 있습니다.
30-53: 엔드포인트 구조 개선 및 상품 옵션 지원이 잘 구현되었습니다.기존 "/cart/line-items"에서 더 RESTful한 "/carts/{cartId}/line-items" 구조로 변경되었고, 상품의 색상과 사이즈 옵션을 지원하도록 요청 본문과 검증 로직이 적절히 수정되었습니다.
55-68: 상품 제거 기능에 대한 테스트가 적절하게 구현되었습니다.DELETE 엔드포인트를 테스트하는 메소드가 추가되었으며, CartService의 removeItemFromCart 메소드가 올바른 파라미터로 호출되는지 검증하고 있습니다.
src/test/java/com/example/demo/model/LineItemTest.java (1)
27-41: 다른 옵션에 대한 수량 추가 예외 처리 테스트가 잘 구현되었습니다.같은 상품이지만 옵션이 다를 경우 수량이 추가되지 않는 경우를 적절하게 테스트하고 있습니다. 이는 장바구니에서 상품 옵션을 올바르게 처리하는데 중요한 기능입니다.
src/test/java/com/example/demo/model/CartTest.java (7)
19-26: 테스트 설정이 적절하게 구현되었습니다.@beforeeach를 사용하여 테스트에 필요한 상품과 옵션 객체를 효율적으로 초기화하고 있습니다.
28-34: 빈 장바구니 테스트가 잘 구현되었습니다.빈 장바구니의 초기 상태를 검증하는 기본적인 테스트가 잘 구현되었습니다.
36-45: 빈 장바구니에 상품 추가 테스트가 적절합니다.빈 장바구니에 상품을 추가했을 때 전체 수량이 올바르게 계산되는지 검증하고 있습니다.
47-61: 기존 상품 추가 테스트가 잘 구현되었습니다.장바구니에 이미 있는 상품을 추가할 때 수량이 올바르게 합산되는지 검증하고 있습니다.
63-78: 새로운 상품 추가 테스트가 잘 구현되었습니다.기존 상품이 있는 장바구니에 새로운 상품을 추가할 때 라인 아이템 개수와 전체 수량이 올바르게 변경되는지 검증하고 있습니다.
80-93: 수량 제한 예외 테스트가 적절하게 구현되었습니다.장바구니 수량이 제한(20개)을 초과할 경우 예외가 발생하는지 검증하는 테스트가 잘 구현되었습니다.
97-99: 도우미 메소드가 잘 구현되었습니다.테스트 코드 중복을 줄이기 위한 createLineItem 메소드가 적절하게 구현되었습니다.
src/main/java/com/example/demo/model/Product.java (3)
7-9: 엔티티 주석 설정이 적절합니다.
@Entity와@Table을 통해 데이터베이스 테이블과 잘 매핑하고 있습니다.
35-37: 가격 변경 메서드의 간단명료함이 좋습니다.
가격 변경만을 전담하므로 유지보수가 용이합니다.
39-41: 이름 변경 메서드가 명확합니다.
메서드명을 통해 의도를 명확히 전달하고 있어서 코드 가독성이 좋습니다.src/test/java/com/example/demo/controllers/CartControllerTest.java (1)
38-47: 장바구니 삭제 테스트가 적절합니다.
HTTP 응답 코드(204)와clearCart호출 여부가 명확하게 검증되어 있습니다.src/test/java/com/example/demo/application/CartServiceTest.java (1)
1-159: 다양한 시나리오를 테스트하여 품질이 우수해 보입니다.
장바구니가 존재하지 않는 경우, 상품이 없는 경우, 수량 초과 등 핵심 예외 상황이 모두 커버되고 있습니다. 추가로 DB 연결 이슈나 트랜잭션 관련 테스트가 필요하다면 별도로 작성해보셔도 좋겠습니다.src/main/java/com/example/demo/model/Cart.java (1)
15-94: 전체적인 Cart 구현이 잘 되어 있습니다.Cart 엔티티 클래스가 적절하게 구현되어 있으며, JPA 어노테이션과 연관 관계 매핑이 올바르게 설정되었습니다. 컬렉션의 불변성을 보장하기 위해
Collections.unmodifiableList를 사용한 점도 좋습니다.src/main/java/com/example/demo/controllers/LineItemController.java (1)
18-25: 컨트롤러 설정 및 의존성 주입이 잘 구현되었습니다.REST 컨트롤러와 경로 매핑이 적절하게 설정되었으며, Lombok의
@RequiredArgsConstructor를 통한 의존성 주입 방식이 깔끔합니다.src/main/java/com/example/demo/application/CartService.java (1)
13-23: 서비스 계층의 기본 구조가 잘 설계되어 있습니다.
@Service및@Transactional어노테이션이 올바르게 적용되었으며, 생성자를 통한 의존성 주입 방식이 적절합니다.src/main/java/com/example/demo/model/LineItem.java (2)
11-31: LineItem 엔티티의 기본 구조가 잘 설계되어 있습니다.JPA 어노테이션이 올바르게 적용되었으며, 필수 필드와 생성자가 적절하게 구현되어 있습니다.
68-70: setCart 메서드의 양방향 관계 처리 개선양방향 관계를 올바르게 관리하기 위해, 기존 cart가 있는 경우 해당 cart의 lineItems 컬렉션에서 이 아이템을 제거하는 로직이 필요합니다. 그러나 Cart 클래스에서 이미 관계를 처리하고 있으므로, 이 메서드는 현재 상태로 유지해도 됩니다.
| public Product(ProductId id, String name, int price) { | ||
| this.id = id; | ||
| this.name = name; | ||
| this.price = price; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
JPA 호환성을 위해 기본 생성자를 고려하세요.
JPA는 엔티티 인스턴스화 시 기본 생성자가 필요할 수 있습니다.
public Product(ProductId id, String name, int price) {
this.id = id;
this.name = name;
this.price = price;
}
+
+ protected Product() {
+ // JPA용 기본 생성자
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public Product(ProductId id, String name, int price) { | |
| this.id = id; | |
| this.name = name; | |
| this.price = price; | |
| } | |
| public Product(ProductId id, String name, int price) { | |
| this.id = id; | |
| this.name = name; | |
| this.price = price; | |
| } | |
| protected Product() { | |
| // JPA용 기본 생성자 | |
| } |
src/main/java/com/example/demo/controllers/LineItemController.java
Outdated
Show resolved
Hide resolved
| public LineItem(ProductId productId, ProductOption productOption, int quantity) { | ||
| this.id = LineItemId.generate(); | ||
| this.productId = productId; | ||
| this.productOption = productOption; | ||
| this.quantity = quantity; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
생성자에 수량 검증 로직 추가 필요
LineItem 생성 시 수량(quantity)이 0보다 큰지 검증하는 로직이 없습니다. 유효하지 않은 상태의 객체 생성을 방지하기 위해 검증 로직을 추가하는 것이 좋습니다.
public LineItem(ProductId productId, ProductOption productOption, int quantity) {
+ if (quantity <= 0) {
+ throw new IllegalArgumentException("수량은 0보다 커야 합니다");
+ }
this.id = LineItemId.generate();
this.productId = productId;
this.productOption = productOption;
this.quantity = quantity;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public LineItem(ProductId productId, ProductOption productOption, int quantity) { | |
| this.id = LineItemId.generate(); | |
| this.productId = productId; | |
| this.productOption = productOption; | |
| this.quantity = quantity; | |
| } | |
| public LineItem(ProductId productId, ProductOption productOption, int quantity) { | |
| if (quantity <= 0) { | |
| throw new IllegalArgumentException("수량은 0보다 커야 합니다"); | |
| } | |
| this.id = LineItemId.generate(); | |
| this.productId = productId; | |
| this.productOption = productOption; | |
| this.quantity = quantity; | |
| } |
| public void addQuantity(int quantity, ProductOption productOption) { | ||
| if(!this.productOption.equals(productOption)) { | ||
| return; | ||
| } | ||
| this.quantity += quantity; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
addQuantity 메서드의 개선 필요
- productOption이 일치하지 않을 경우 메서드가 조용히 반환하는데, 이는 직관적이지 않고 버그의 원인이 될 수 있습니다.
- 추가되는 수량이 양수인지 검증하는 로직이 없습니다.
- if 조건문 뒤에 공백이 필요합니다.
public void addQuantity(int quantity, ProductOption productOption) {
- if(!this.productOption.equals(productOption)) {
- return;
+ if (!this.productOption.equals(productOption)) {
+ throw new IllegalArgumentException("같은 상품 옵션에만 수량을 추가할 수 있습니다");
}
+ if (quantity <= 0) {
+ throw new IllegalArgumentException("추가 수량은 0보다 커야 합니다");
+ }
this.quantity += quantity;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public void addQuantity(int quantity, ProductOption productOption) { | |
| if(!this.productOption.equals(productOption)) { | |
| return; | |
| } | |
| this.quantity += quantity; | |
| } | |
| public void addQuantity(int quantity, ProductOption productOption) { | |
| if (!this.productOption.equals(productOption)) { | |
| throw new IllegalArgumentException("같은 상품 옵션에만 수량을 추가할 수 있습니다"); | |
| } | |
| if (quantity <= 0) { | |
| throw new IllegalArgumentException("추가 수량은 0보다 커야 합니다"); | |
| } | |
| this.quantity += quantity; | |
| } |
| return this.productId.equals(productId) && this.productOption.equals(productOption); | ||
| } | ||
|
|
||
| public void setCart(Cart cart) { |
There was a problem hiding this comment.
일반적으론 이게 불필요합니다. 이걸 한다는 건 뭔가 잘못되고 있다는 강력한 신호죠. 평범하게 Cart Aggregate를 만들 때 이게 불필요했다면, 영원히 불필요해야 합니다. 그러기 위해서 JPA를 쓰는 거죠.
| @EmbeddedId | ||
| private LineItemId id; | ||
|
|
||
| @ManyToOne(fetch = FetchType.LAZY) |
There was a problem hiding this comment.
강의에서 이야기한 것처럼 ManyToOne은 지양해야 합니다. 최후의 최후에 어떤 특이한 무언가를 하기 위해서 쓰는 일은 있을 수 있어도, 평소엔 안 쓰입니다. 단적으로, 저는 실무에서 OneToMany 외엔 아무 것도 안 씁니다. “나는 여기에 이걸 왜 썼는가?”에 답할 수 있어야 합니다.
|
|
||
| lineItem = new LineItem(productId, productOption, quantity); | ||
| lineItems.add(lineItem); | ||
| lineItem.setCart(this); |
There was a problem hiding this comment.
4주차에 없던 코드가 6주차에 들어오면 안 됩니다. JPA를 사용하는 이유는 단순합니다. 4주차 코드 자체는 전혀 고치지 않고 애너테이션 몇 개만 붙였더니 DB에 영속화가 되는 거죠.
|
|
||
| import java.util.List; | ||
|
|
||
| public class CartResponseDto { |
There was a problem hiding this comment.
record를 쓸 수 없는 상황이 아니라면 DTO는 모두 record로 만드세요. record는 Kotlin이나 다른 언어에 비해 훨씬 훌륭한, 매우 강력한 무기 중 하나입니다. 저는 record가 Java의 미래라고 말하곤 해요.
| package com.example.demo.controllers.dto; | ||
|
|
||
| public class LineItemRequestDto { | ||
| private String productId; |
There was a problem hiding this comment.
Request DTO엔 validation을 추가해 주세요. 애너테이션을 적당히 붙이면 유효성 검사가 너무 간단히 수행돼서 안 할 이유가 없습니다.
| } | ||
|
|
||
| public void removeLineItemById(LineItemId lineItemId) { | ||
| if (lineItemId == null) return; |
There was a problem hiding this comment.
여기서 null 검사를 하는 일은 매우 드믑니다. UI 레이어에서 쳐내는 게 쉽기도 하고, 이게 null이라고 무슨 문제가 생기는 것도 아니기도 하기 때문이죠.
| calculateTotalQuantity(); | ||
| } | ||
|
|
||
| public void removeLineItemById(LineItemId lineItemId) { |
There was a problem hiding this comment.
ById는 불필요합니다. 이 표현은 대단히 나쁜데, 메서드에 이런 걸 시시콜콜하게 써주지 않으면 자동으로 관련 코드를 생성할 수 없는 경우 = Spring Data JPA를 쓰면서 Repository의 구현을 내가 만들고 싶지 않은 상황을 제외하면 그 어느 곳에서도 이걸 쓰면 안 됩니다. 저는 AddIntegerWithInteger 같은 메서드 이름은 절대로 쓰고 싶지 않습니다. 그저, 기계에게 integer와 integer를 더한다는 걸 아주 명확히 알려줘야 하는 가장 바닥에서 아주 아주 예외적으로 쓰는 것 정도만 가까스로 용납해줄 수 있는 거죠. 이런 흉물스러운 이름은 Repository 밖으로 절대 못 나오게 하시고, 그 정도가 너무 심하면 반드시 JPQL로 봉인해야 합니다.
|
|
||
| public void addQuantity(int quantity, ProductOption productOption) { | ||
| if(!this.productOption.equals(productOption)) { | ||
| return; |
There was a problem hiding this comment.
이건 버그입니다. 수량 추가를 했는데, 아무 일도 안 일어나니까요.
- 수량이 늘던지
- 예외가 발생하던지 해야 합니다.
코드만 보면 예외를 던져야 하겠지만, 사실 예외를 던질 일이 없어야 합니다. addQuantity에 ProductOption이 같이 들어오는 것 자체가 잘못된 거죠.
| ) { | ||
|
|
||
| public static LineItemId generate() { | ||
| return new LineItemId("lineItemId-" + UUID.randomUUID()); |
There was a problem hiding this comment.
아주 특별한 경우가 아니면 UUID만 써야 합니다.
참고로, 제가 “아주 특별란 경우”라고 하는 건 지구가 종말할 때 정도의 의미로 이해해 주시면 됩니다. 그런 일은 없고, 그러면 안 된다는 걸 완곡히 표현하는 거죠.
| class LineItemIdTest { | ||
|
|
||
| @Test | ||
| @DisplayName("같은 값이면 같은 객체로 취급된다.") |
There was a problem hiding this comment.
Java가 record 기능을 올바르게 만들었는지 오라클 개발자들의 실수가 있는지 찾으려는 게 아니라면 LineItemId에 대한 테스트는 불필요합니다.
| class LineItemTest { | ||
|
|
||
| @Test | ||
| @DisplayName("같은 상품과 옵션일 경우, 수량을 추가할 수 있어야 한다.") |
There was a problem hiding this comment.
이 설명을 쓸 때 이상하다고 느껴야 합니다. 다른 상품과 옵션인데 LineItem을 건드린다는 건 불가능해야 합니다. 만약 홍길동이라는 이름의 학생의 점수를 변경하는 changeScore란 메서드가 있는데, 이 메서드가 score와 name을 받아서 name이 홍길동인지 확인한다고 하면 넌센스겠죠. 누군가 일을 제대로 안 하고 있다는 겁니다. 이 경우라면 Cart에 매우 심각한 버그가 있어서 전혀 무관한 LineItem에 addQuantity를 요청할 수 있다는 겁니다. 그런 일은 처음부터 일어나면 안 되죠.
| } | ||
|
|
||
| @Test | ||
| @DisplayName("같은 상품이라도 옵션이 다르면, 수량은 추가되지 않는다.") |
There was a problem hiding this comment.
이건 말이 안 됩니다. 아샬이 달래에게 200만 원을 송금한다고 해보죠. 그런데 달래의 통장에 뭔가 이슈가 있어서 돈을 못 넣는다고 해봐요. 그럼 돈을 안 넣는다고 하면 안 됩니다. 그렇게 되면 아샬 통장에서 200만 원만 사라지고 아무 일도 안 일어납니다. 그게 아니라 이런 경우엔 에러가 나야 하죠. 예외를 던져서 모든 걸 원래대로 되돌려야 합니다. 우리가 UoW 패턴에서 commit과 rollback을 다룬 건 이걸 위해서죠. 게다가 상품, 옵션이 다르면 addQuantity를 실행하면 안 됩니다. addQuantity는 오직 quantity만 받아야 합니다. 위에 적었지만, changeScore는 score만 받지, name 같은 걸 같이 받으면 안 되죠.
build.gradle.kts
Outdated
| compileOnly ("org.projectlombok:lombok") | ||
| annotationProcessor ("org.projectlombok:lombok") |
There was a problem hiding this comment.
아마 아주 특별한 일이 일어나지 않는다면 Lombok을 쓸 일은 없을 겁니다. record가 도입된 후로는 영원히 봉인해도 괜찮을 정도라고 생각해요. 저는 최근에 이것의 존재를 아예 잊고 있었습니다.
| cart.addProduct(productId, option, quantity); | ||
| } | ||
|
|
||
| public void removeItemFromCart(Long cartId, LineItemId lineItemId) { |
There was a problem hiding this comment.
Item은 뭔가요? LineItem이라면 그냥 명확히 removeLineItem이라고 하거나 그냥 remove라고 하거나 하는 게 더 낫습니다. 이 이름을 보면 “Item”이라는 제가 모르는 무언가가 또 있는 건지 어떤 건지 고민하게 됩니다. 유비쿼터스 언어가 깨진 모습인 거죠.
| public void addItemToCart(Long cartId, ProductId productId, ProductOption option, int quantity) { | ||
| Cart cart = cartRepository.findById(cartId) | ||
| .orElseThrow(() -> new IllegalArgumentException("장바구니가 존재하지 않습니다.")); | ||
| Product product = productRepository.findById(productId) |
There was a problem hiding this comment.
product가 쓰이지 않습니다. 존재만 확인하려고 했다면 exists를 써야 합니다.
|
|
||
| @RestController | ||
| @RequestMapping("/cart") | ||
| @RequestMapping("/carts") |
There was a problem hiding this comment.
장바구니는 session과 마찬가지로 하나입니다. 아주 특별하게 여러 장바구니를 동시에 다루는 경우라면 복수형을 쓸 수도 있겠지만, 그런 일은 없습니다.
|
|
||
| private final CartService cartService; | ||
|
|
||
| @GetMapping("/{cartId}") |
There was a problem hiding this comment.
타인의 장바구니를 훔쳐보는 경우가 아니라면 이런 접근을 허용하면 안 됩니다.
|
|
||
| @RestController | ||
| @RequestMapping("/cart/line-items") | ||
| @RequestMapping("/carts/{cartId}/line-items") |
There was a problem hiding this comment.
기본 코드에서 제시한 틀에서 벗어나지 마세요. 이런 방식은 타인의 장바구니에 상품을 추가하는 일이 벌어질 위험이 있습니다.
| LineItem lineItem = findLineItem(productId, productOption); | ||
|
|
||
| if (lineItem != null) { | ||
| lineItem.addQuantity(quantity, productOption); |
There was a problem hiding this comment.
addQuantity는 quantity만 넘어가야 합니다. 뭔가를 같이 넣는 순간 무언가 잘못 되고 있다는 걸 캐치할 수 있어야 합니다. 우리는 changeScore(score, name);이라고 쓰지 않는다는 걸 알고 있고, 여기서도 동일한 원칙을 적용해야 합니다.
| calculateTotalQuantity(); | ||
| checkValidQuantity(); |
There was a problem hiding this comment.
아주 특별한 경우가 아니면 calculateTotalQuantity에서 checkValidQuantity까지 처리해야 합니다. 아마 모든 calculateTotalQuantity 메서드 사용하는 곳은 바로 뒤에 checkValidQuantity가 따라올 겁니다. 실수로 checkValidQuantity를 빼먹는 순간 불변식은 깨질 거고요. 열심히 Aggregate를 만들었어도 불변식이 깨지면 의미가 없습니다.
| calculateTotalQuantity(); | ||
| checkValidQuantity(); |
There was a problem hiding this comment.
여기서 또 이렇게 둘을 한 세트로 쓰고 있다는 사실을 발견하는 순간, 둘을 합칠 계획을 세워야 합니다.
|
|
||
| @ManyToOne(fetch = FetchType.LAZY) | ||
| @JoinColumn(name="cart_id") | ||
| private Cart cart; |
There was a problem hiding this comment.
이 필드는 영원히 안 쓰일 겁니다. 이게 쓰이게 되는 경우는 LineItemRepository를 만들어서 LineItem을 따로 얻은 상태에서 Cart를 바로 얻으려고 할 때입니다. 즉, Aggregate Root가 아닌데 Repository를 만든 초유의 사태가 벌어졌을 때, 뭔가 크게 잘못됐을 때 밖에 없는 거죠. 그런 일은 영원히 일어나지 않아야 합니다.
| @Test | ||
| @DisplayName("장바구니에 새로운 있는 물건 추가하면 전체 수량은 이미 있던 물건의 수량과 " + | ||
| "새로 추가하는 물건의 수량의 합이다.") | ||
| void addNewProduct() { |
There was a problem hiding this comment.
상품과 옵션이 모두 다른 경우를 테스트하고 있는데, 상품이 같고 옵션이 다른 경우를 테스트하고 있지 않습니다. 테스트 코드를 더 많이 써야 합니다. 상품이 하나라서, 그건 이 테스트보다 더 쉬울 겁니다.
| int oldQuantity = 1; | ||
| Cart cart = new Cart(List.of( | ||
| createLineItem(product1, productOption1, oldQuantity) | ||
| )); | ||
|
|
||
| int newQuantity = 1; | ||
| cart.addProduct(product1.getId(), productOption1, newQuantity); |
There was a problem hiding this comment.
new Cart(어쩌고)는 테스트 코드를 읽기 너무 어렵습니다. 실제 제품 코드에서 new Cart(어쩌고)를 쓸 일도 없고요.
다음과 같이 하는 게 자연스럽게 읽힙니다.
| int oldQuantity = 1; | |
| Cart cart = new Cart(List.of( | |
| createLineItem(product1, productOption1, oldQuantity) | |
| )); | |
| int newQuantity = 1; | |
| cart.addProduct(product1.getId(), productOption1, newQuantity); | |
| int oldQuantity = 1; | |
| int newQuantity = 1; | |
| Cart cart = new Cart(); | |
| cart.addProduct(product1.getId(), productOption1, oldQuantity); | |
| cart.addProduct(product1.getId(), productOption1, newQuantity); |
이건 아래에 있는 모든 다른 테스트 코드에도 마찬가지입니다. 우리는 두 개의 addProduct 메서드 호출하는 부분의 차이점만 순식간에 읽으면 됩니다. 지금처럼 new Cart에 뭐가 들어갔는데 그 다음에 addProduct는 또 어떻게 됐는지 굉장히 다른 모양으로 있는 것 사이의 차이점을 찾으려고 노력하지 않아도 너무 쉽게 알 수 있게 되죠.
|
|
||
|
|
||
|
|
||
| private LineItem createLineItem(Product product, ProductOption productOption, int quantity) { |
There was a problem hiding this comment.
cart.addProduct가 훨씬 잘 읽히고 더 나은 테스트 코드를 보여줍니다. Cart 테스트에서 LineItem을 직접 만든다는 것 자체가 뭔가 해킹하는 느낌이 강합니다. 테스트 코드라고 해서 뭔가 특별한 걸 하려고 하지 말고, 그냥 우리가 평범하게 Cart 객체를 쓰는 상황을 그대로 재현하려고 해보세요. 그래야 실제 상황에 대한 시뮬레이션 및 버그 발견이 가능합니다.
| @Test | ||
| @DisplayName("상품을 추가할때, 상품이 존재하지 않으면 예외가 발생한다") | ||
| void productNotFound() { | ||
| when(cartRepository.findById(cartId)).thenReturn(Optional.of(cart)); | ||
| when(productRepository.findById(productId)).thenReturn(Optional.empty()); | ||
|
|
||
| assertThatThrownBy(()->cartService.addItemToCart(cartId, productId, option, 1)) | ||
| .isInstanceOf(IllegalArgumentException.class); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("빈 장바구니에 상품을 추가하면, 전체 갯수는 상품의 갯수와 같다.") | ||
| void addProductToCart() { | ||
| //given | ||
| mockCartAndProduct(); | ||
| int quantity = 2; | ||
|
|
||
| // when | ||
| cartService.addItemToCart(cartId, productId, option, quantity); | ||
|
|
||
| // then | ||
| assertThat(cart.getLineItems()).hasSize(1); | ||
| assertThat(cart.getTotalQuantity()).isEqualTo(quantity); | ||
| } | ||
|
|
||
|
|
||
| @Test | ||
| @DisplayName("상품이 있는 장바구니에 다른 상품을 추가하면, LineItem이 추가되고 전체 수량은 합쳐진다") | ||
| void addAnotherProductToCart() { | ||
| // given | ||
| int existedQuantity = 2; | ||
| LineItem lineItem = new LineItem(productId, option, existedQuantity); | ||
| cart = new Cart( | ||
| List.of(lineItem) | ||
| ); | ||
|
|
||
| mockCartAndProduct(); | ||
|
|
||
| ProductId newProductId = new ProductId("product-2"); | ||
| Product newProduct = new Product(newProductId, "Product #2", 10000); | ||
| when(productRepository.findById(newProductId)).thenReturn(Optional.of(newProduct)); | ||
|
|
||
| int newQuantity = 2; | ||
|
|
||
| // when | ||
| cartService.addItemToCart(cartId, newProductId, option, newQuantity); | ||
|
|
||
| // then | ||
| assertThat(cart.getLineItems()).hasSize(2); | ||
| assertThat(cart.getTotalQuantity()).isEqualTo(existedQuantity + newQuantity); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("장바구니에서 LineItem을 삭제하면 해당 상품을 제거할수있다.") | ||
| void removeItemFromCart() { | ||
| // given | ||
| LineItem lineItem = new LineItem(productId, option, 1); | ||
| cart = new Cart(List.of(lineItem)); | ||
|
|
||
| when(cartRepository.findById(cartId)).thenReturn(Optional.of(cart)); | ||
|
|
||
| // when | ||
| cartService.removeItemFromCart(cartId, lineItem.getId()); | ||
|
|
||
| // then | ||
| assertThat(cart.getLineItems()).isEmpty(); | ||
| assertThat(cart.getTotalQuantity()).isZero(); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("상품 수량이 20개가 초과하면 예외가 발생한다.") | ||
| void throwExceptionWhenTotalQuantityOverLimit() { | ||
| // given | ||
| mockCartAndProduct(); | ||
| cartService.addItemToCart(cartId, productId, option, 20); | ||
|
|
||
| // when & then | ||
| assertThatThrownBy(()-> | ||
| cartService.addItemToCart(cartId, productId, option, 1) | ||
| ).isInstanceOf(IllegalArgumentException.class); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("장바구니를 비우면 모든 상품이 제거가 된다.") | ||
| void clearCart() { | ||
| // given | ||
| LineItem lineItem = new LineItem(productId, option, 1); | ||
| cart = new Cart( | ||
| List.of(lineItem) | ||
| ); | ||
| when(cartRepository.findById(cartId)).thenReturn(Optional.of(cart)); | ||
|
|
||
| // when | ||
| cartService.clearCart(cartId); | ||
|
|
||
| // then | ||
| assertThat(cart.getLineItems()).isEmpty(); | ||
| assertThat(cart.getTotalQuantity()).isZero(); | ||
| } |
There was a problem hiding this comment.
이런 비즈니스 로직은 Application Layer에서 테스트하지 마세요. 불필요하게 Mocking을 많이 하게 되고, 아무 가치가 없습니다. Aggregate에서 검증하셔야 합니다.
우리가 단 하나의 테스트 코드만 작성할 수 있다면, 그것은 Domain Model, 특히 Aggregate에 대한 것이어야 합니다.
https://youtu.be/RoQtNLl-Wko
There was a problem hiding this comment.
Copilot reviewed 24 out of 24 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
src/test/java/com/example/demo/model/CartTest.java:64
- The display name in this test appears confusing due to the phrase '새로운 있는 물건'. Consider rephrasing it to '장바구니에 없는 물건 추가하면' for clarity.
@DisplayName("장바구니에 새로운 있는 물건 추가하면 전체 수량은 이미 있던 물건의 수량과 새로 추가하는 물건의 수량의 합이다.")
src/main/java/com/example/demo/model/Cart.java:60
- [nitpick] Consider validating the total quantity before mutating the cart state. Pre-validating could prevent state changes that would be immediately rolled back if the quantity limit is exceeded.
checkValidQuantity();
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main/java/com/example/demo/controllers/LineItemController.java (1)
34-43: 입력 유효성 검사 적용이 잘 되었습니다.
@Valid어노테이션을 사용하여LineItemRequestDto의 유효성 검사를 적용한 점이 좋습니다. 이전 리뷰에서 제안된 내용이 잘 반영되었습니다.
🧹 Nitpick comments (3)
src/test/java/com/example/demo/model/LineItemTest.java (1)
46-52: 오탈자 및 메서드 시나리오 점검
메서드 이름에 오탈자(“Diffrent”)가 있습니다. “Different”로 수정 부탁드립니다. 로직 자체는 문제가 없습니다.- @DisplayName("상품ID는 같지만 옵션이 다르면 다른 라인아이템이다.") - void whenSameProductIdAndDiffrentProductOption() { + @DisplayName("상품ID는 같지만 옵션이 다르면 다른 라인아이템이다.") + void whenSameProductIdAndDifferentProductOption() {src/test/java/com/example/demo/model/CartTest.java (2)
93-106: 유효한 테스트 케이스이지만 경계값 테스트를 추가하면 좋을 것 같습니다.현재 테스트는 장바구니 수량이 20개를 초과할 때 예외가 발생하는지 확인합니다. 장바구니에 이미 20개 항목이 있을 때 추가 항목을 추가하는 경우만 테스트하고 있습니다. 경계값 테스트를 위해 다음과 같은 시나리오를 추가로 테스트하는 것이 좋을 것 같습니다:
- 장바구니에 19개 항목이 있을 때 1개 항목을 추가하는 경우 (정확히 20개가 되는 경우)
- 장바구니가 비었을 때 21개 항목을 한 번에 추가하는 경우
@Test @DisplayName("전체 장바구니의 수량이 20개가 넘어가면 예외가 발생한다.") void totalQuantityCanNotOverLimit() { Cart cart = new Cart(); int newQuantity = 5; cart.addProduct(product1.getId(), productOption1, 20); assertThatThrownBy( () -> cart.addProduct(product1.getId(), productOption1, newQuantity) ).isInstanceOf(IllegalArgumentException.class); + + // 경계값 테스트: 정확히 20개인 경우 (성공 케이스) + Cart cart2 = new Cart(); + cart2.addProduct(product1.getId(), productOption1, 19); + cart2.addProduct(product1.getId(), productOption2, 1); + assertThat(cart2.getTotalQuantity()).isEqualTo(20); + + // 경계값 테스트: 처음부터 제한 초과하는 경우 + Cart cart3 = new Cart(); + assertThatThrownBy( + () -> cart3.addProduct(product1.getId(), productOption1, 21) + ).isInstanceOf(IllegalArgumentException.class); }
121-133: 테스트가 정상적으로 작동하나 구현 세부사항에 의존적일 수 있습니다.이 테스트에서는 LineItem을 제거하기 위해 첫 번째 항목을 가져와 직접 삭제하고 있습니다. 이 방식은 LineItem의 순서에 의존적일 수 있습니다. 보다 의도가 명확한 테스트를 위해 특정 제품과 옵션을 기반으로 LineItem을 삭제하는 방식으로 변경하는 것이 좋을 것 같습니다.
@Test @DisplayName("장바구니에서 LineItem을 삭제하면 해당 상품을 제거할수있다.") void removeLineItem() { Cart cart = new Cart(); cart.addProduct(product1.getId(), productOption1, 1); cart.addProduct(product2.getId(), productOption2, 2); - LineItem lineItem = cart.getLineItems().get(0); - cart.removeLineItem(lineItem.getId()); + // 특정 제품의 LineItemId 찾기 (product1과 productOption1에 해당하는) + LineItemId lineItemToRemove = cart.getLineItems().stream() + .filter(item -> item.getProductId().equals(product1.getId()) && + item.getProductOption().equals(productOption1)) + .findFirst() + .map(LineItem::getId) + .orElseThrow(); + + cart.removeLineItem(lineItemToRemove); assertThat(cart.getLineItems()).hasSize(1); assertThat(cart.getTotalQuantity()).isEqualTo(2); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
build.gradle.kts(1 hunks)src/main/java/com/example/demo/application/CartService.java(1 hunks)src/main/java/com/example/demo/controllers/CartController.java(1 hunks)src/main/java/com/example/demo/controllers/LineItemController.java(1 hunks)src/main/java/com/example/demo/controllers/dto/CartResponseDto.java(1 hunks)src/main/java/com/example/demo/controllers/dto/LineItemRequestDto.java(1 hunks)src/main/java/com/example/demo/controllers/dto/LineItemResponseDto.java(1 hunks)src/main/java/com/example/demo/model/Cart.java(1 hunks)src/main/java/com/example/demo/model/LineItem.java(1 hunks)src/main/java/com/example/demo/model/LineItemId.java(1 hunks)src/test/java/com/example/demo/controllers/CartControllerTest.java(2 hunks)src/test/java/com/example/demo/controllers/LineItemControllerTest.java(3 hunks)src/test/java/com/example/demo/model/CartTest.java(1 hunks)src/test/java/com/example/demo/model/LineItemIdTest.java(1 hunks)src/test/java/com/example/demo/model/LineItemTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/test/java/com/example/demo/model/LineItemIdTest.java
- build.gradle.kts
- src/main/java/com/example/demo/model/Cart.java
- src/main/java/com/example/demo/application/CartService.java
- src/main/java/com/example/demo/model/LineItem.java
🧰 Additional context used
🧬 Code Definitions (4)
src/test/java/com/example/demo/controllers/CartControllerTest.java (1)
src/test/java/com/example/demo/controllers/LineItemControllerTest.java (1)
WebMvcTest(23-71)
src/test/java/com/example/demo/controllers/LineItemControllerTest.java (1)
src/test/java/com/example/demo/controllers/CartControllerTest.java (1)
WebMvcTest(18-48)
src/main/java/com/example/demo/controllers/CartController.java (1)
src/main/java/com/example/demo/controllers/LineItemController.java (1)
RestController(20-53)
src/main/java/com/example/demo/controllers/LineItemController.java (1)
src/main/java/com/example/demo/controllers/CartController.java (1)
RestController(14-51)
🔇 Additional comments (16)
src/main/java/com/example/demo/controllers/dto/CartResponseDto.java (1)
5-10: 잘 구현되었습니다!record를 사용하여 DTO를 구현한 점이 좋습니다. record는 불변 데이터 클래스를 위한 간결한 구문을 제공하며 Java의 강력한 기능 중 하나입니다.
src/main/java/com/example/demo/controllers/dto/LineItemResponseDto.java (1)
3-10: 깔끔하게 구현되었습니다!LineItemResponseDto도 record를 사용하여 응답 DTO를 정의한 점이 좋습니다. 불변 객체로 만들어 응답 데이터의 일관성을 유지할 수 있습니다.
src/main/java/com/example/demo/model/LineItemId.java (1)
7-15: UUID를 사용한 식별자 구현이 훌륭합니다!
@Embeddable어노테이션과 함께 record를 사용하여 임베디드 값 객체를 정의한 접근 방식이 좋습니다. UUID를 사용하여 고유 식별자를 생성하는generate()메서드도 잘 구현되었습니다.src/main/java/com/example/demo/controllers/dto/LineItemRequestDto.java (1)
7-22: 검증 어노테이션이 잘 적용되었습니다!요청 DTO에 적절한 검증 어노테이션(
@NotBlank,@Min,@Max)을 사용하여 입력 유효성 검사를 구현한 점이 좋습니다. 특히 오류 메시지를 한글로 작성하여 사용자 친화적인 피드백을 제공할 수 있게 한 점이 인상적입니다.src/test/java/com/example/demo/model/LineItemTest.java (4)
12-19: 메서드 초기화(setUp)에서의 기본 상태 설정 확인
테스트 전담 필드(productId, productOption)를 공통으로 초기화하여 코드 중복을 줄이고 가독성을 높입니다. setUp 메서드가 잘 작성되었습니다.
23-32: addQuantity 테스트 적절성 확인
테스트가 기존 수량에 새로운 수량을 더해주는 로직을 정확히 검증하고 있습니다. assertThat 구문의 활용이 적절합니다.
36-42: 동일 상품 및 옵션 판별에 대한 테스트
상품 ID와 옵션이 동일할 때 true를 반환하는지 제대로 검증하고 있습니다. 이름과 시나리오가 일관성이 있어 가독성이 좋습니다.
56-62: 서로 다른 상품 판별에 대한 테스트
상품 ID가 다를 경우 false를 반환하는 로직을 간단명료하게 확인합니다. 예상 동작을 정확히 검증했으므로 좋습니다.src/test/java/com/example/demo/controllers/LineItemControllerTest.java (2)
28-55: addProduct 테스트에 대한 검증
POST 요청으로 상품을 추가하는 시나리오를 정확히 표현하고 있습니다. color, size, quantity 등의 필드가CartService로 잘 전달되는지verify로 검증하는 점이 좋습니다.
57-69: removeProduct 테스트에 대한 검증
DELETE 요청으로 특정 lineItemId를 제거하는 로직을 검증하고 있습니다.CartService.removeLineItem를 적절히 호출하며, 테스트의 목적을 명확히 드러냅니다.src/test/java/com/example/demo/controllers/CartControllerTest.java (2)
28-35: getCart 테스트 검증
GET 요청으로 장바구니를 조회하는 시나리오를 간단하게 잘 구성했습니다.when(cartService.getCart(cartId)).thenReturn(cart);으로 Mock 동작을 설정하여 정확한 결과를 검증합니다.
37-47: deleteCart 테스트 검증
DELETE 요청으로 장바구니를 비우는 로직을 검증하며,cartService.clearCart(cartId);호출 여부를 확인하는 흐름이 적절합니다. No Content(204) 응답도 테스트 요구사항과 부합해 보입니다.src/main/java/com/example/demo/controllers/CartController.java (2)
27-43: 고정된 cartId 사용에 대한 주의
현재 cartId가 고정되어 있어 다중 사용자 환경에서는 문제가 발생할 수 있습니다. 향후 세션이나 인증 정보를 바탕으로 cartId를 동적으로 받아오도록 개선을 검토해 주시기 바랍니다.
46-49: deleteCart 메서드 구현 검토
장바구니를 비우는 핵심 로직을 간결하게 잘 구현하셨습니다. 상태 코드 204 사용도 REST 관점에서 적절합니다.src/test/java/com/example/demo/model/CartTest.java (1)
1-135: 테스트 클래스 구조와 테스트 케이스가 잘 설계되었습니다.테스트 클래스가 Cart 모델의 주요 기능을 잘 검증하고 있습니다. 각 테스트는 명확한 DisplayName을 가지고 있어 테스트의 목적을 이해하기 쉽습니다. 특히 이전 리뷰에서 제안된 상품이 같고 옵션이 다른 경우에 대한 테스트(
addSameProductAndDifferentOption)가 추가되었고, 테스트 가독성을 높이기 위해cart.addProduct패턴이 적용된 점이 좋습니다.src/main/java/com/example/demo/controllers/LineItemController.java (1)
45-51: LineItem 삭제 기능이 적절하게 구현되었습니다.LineItem을 삭제하는 기능이 RESTful 방식으로 잘 구현되었습니다.
@PathVariable을 사용하여 URL에서 lineItemId를 추출하고,CartService를 통해 삭제 작업을 수행하는 방식이 적절합니다.
|
|
||
| private final CartService cartService; | ||
|
|
||
| private Long cartId =1L; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
고정된 cartId 사용에 대한 개선 필요
cartId가 고정값 1L로 하드코딩되어 있습니다. 이는 다중 사용자 환경에서 문제가 될 수 있으며, 다른 사용자의 장바구니에 상품을 추가하는 보안 위험이 있습니다. CartController에서도 같은 패턴이 사용되고 있습니다.
사용자 인증 상태에 따라 동적으로 cartId를 결정하거나, 세션에서 가져오는 방식으로 개선하는 것이 좋을 것 같습니다.
🏁 Script executed:
#!/bin/bash
# 프로젝트 내에서 고정된 cartId 사용 패턴 확인
rg -A 1 "cartId = 1L" --type javaLength of output: 992
중요: 하드코딩된 cartId 개선 필요
-
문제점:
- 현재
src/main/java/com/example/demo/controllers/LineItemController.java(예상 line 30)에private Long cartId = 1L;와 같이 고정된 cartId가 사용되고 있습니다. - 동일한 패턴이
src/main/java/com/example/demo/controllers/CartController.java와 관련 테스트 파일(CartControllerTest.java,LineItemControllerTest.java)에서도 확인되었습니다.
- 현재
-
우려사항:
- 다중 사용자 환경에서는 하드코딩된 cartId 사용이 다른 사용자의 장바구니에 접근하는 보안 위험으로 이어질 수 있습니다.
-
제안:
- 운영 환경에서는 사용자 인증 정보나 세션 데이터를 활용하여 cartId를 동적으로 결정하는 방식으로 수정하시기 바랍니다.
- 테스트 코드의 경우 단순 목업이나 고정값 사용이 의도된 것이라면, 실제 운영 코드와 분리하여 관리하는지 확인해 주세요.
There was a problem hiding this comment.
CodeRabbit의 지적이 올바릅니다. UI Layer에 이런 값이 있으면 안 됩니다. 임시로 고정된 값을 쓰고 싶다면 그게 어디에 있어야 하는지 판단해야 합니다. UI Layer에선 장바구니를 알아낼 방법이 영원히 없습니다.
There was a problem hiding this comment.
@ahastudio 맞습니다. 컨트롤러(UI Layer)에 하드코딩된 cartId는 적절하지 않습니다. 이상적인 접근 방식은 다음과 같습니다:
- 서비스 계층에서 결정: 현재 사용자 컨텍스트를 기반으로 서비스가 적절한 cartId를 결정하고 관리
- 인증 기반: 인증된 사용자 정보에서 cartId를 도출 (예:
SecurityContextHolder에서 사용자 정보 활용) - 세션 관리: 사용자 세션에 cartId를 저장하고 컨트롤러에서는 세션에서 조회만 하는 방식
CartService가 사용자별 장바구니를 관리하고, 컨트롤러는 단순히 서비스를 호출하는 역할로 제한하는 것이 좋습니다. 이는 관심사 분리(Separation of Concerns) 원칙에도 부합합니다.
✏️ Learnings added
Learnt from: ahastudio
PR: dal-lab/wild-backend-cart-app#9
File: src/main/java/com/example/demo/controllers/LineItemController.java:30-30
Timestamp: 2025-04-09T12:41:55.907Z
Learning: 컨트롤러(UI Layer)에는 하드코딩된 엔티티 ID(예: cartId)를 포함하지 않아야 합니다. 이러한 ID는 사용자 인증, 세션 관리, 또는 서비스 계층을 통해 동적으로 결정되어야 합니다.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
|
||
| @DeleteMapping("/{lineItemId}") | ||
| @ResponseStatus(HttpStatus.NO_CONTENT) | ||
| public void deleteLineItem(@PathVariable UUID lineItemId) { |
There was a problem hiding this comment.
아, PathVariable이면 그냥 Stirng이어야 합니다. /cart/line-items/abc 경로로 요청이 들어왔을 때 이게 UUID인지 우리는 알 수 없습니다. 그냥 abc란 문자열이죠.
| @Valid @RequestBody LineItemRequestDto requestDto) { | ||
|
|
||
| cartService.addItemToCart( | ||
| cartId, |
There was a problem hiding this comment.
여기서 cartId를 넘겨줘야 하는 이유가 없습니다. 가장 큰 문제는 UI Layer에서는 Cart ID를 영원히 알 수 없다는 거죠. #9 (comment)
|
|
||
| @Embeddable | ||
| public record LineItemId( | ||
| UUID id |
There was a problem hiding this comment.
다음 두 가지 이유로 여기엔 그냥 String을 쓰는 걸 권합니다.
-
LineItemId에서 UUID의 다른 기능을 사용하지 않습니다. 앞으로도 그럴 겁니다.
-
만약 우리가 DB에 UUID가 아닌 임의의 값(e.g.
test-line-item)을 넣으면 어떻게 될까요? 저는 임의의 값이 유니크해서 서로 식별할 수 있다면 올바르다고 판단할 수 있어야 한다고 봅니다. 또한, 앞으로 식별자 생성 방식을 바꾸더라도 문제가 없어야 한다고 봅니다. UUID를 쓸 경우, 이 부분에서 에러가 나거나 어긋난 상태(UUID가 아닌데 오류 없이 그냥 넘어가는 건 잠재적 버그죠)가 될 위험이 있습니다.
| ) { | ||
|
|
||
| public static LineItemId generate() { | ||
| return new LineItemId(UUID.randomUUID()); |
There was a problem hiding this comment.
UUID에 집착해야 하는 아주 특별한 상황이 아니라면 그냥 이렇게 하면 됩니다.
| return new LineItemId(UUID.randomUUID()); | |
| return new LineItemId(UUID.randomUUID().toString()); |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
src/main/java/com/example/demo/controllers/CartController.java (1)
43-43: DeleteMapping 어노테이션에 불필요한 괄호가 있습니다.
@DeleteMapping()보다는@DeleteMapping으로 작성하는 것이 더 간결합니다. 경로가 없는 경우 괄호를 생략할 수 있습니다.- @DeleteMapping() + @DeleteMappingsrc/test/java/com/example/demo/application/CartServiceTest.java (1)
30-37: 테스트 준비 과정에서 레포지토리와 세션을 적절히 Mocking하여cartService를 구성한 점이 좋습니다. 다만, 이후 테스트에서 총 수량 제한(예: 20개)이나 상품이 존재하지 않는 상황에 대한 예외 테스트도 포함시키면, 경계 조건까지 검증할 수 있으므로 테스트 품질이 높아질 것 같습니다.src/main/java/com/example/demo/model/Cart.java (4)
31-34: 생성자에서 전달받은lineItems를 복사 후updateTotalQuantity()로 총 수량을 갱신하는 로직이 좋습니다. 혹시lineItems가 null로 들어올 가능성을 배제할 수 없다면, 방어 코드를 조금 보강하는 것도 고려해보시면 좋겠습니다.
48-61:addProduct에서 동일 상품 옵션을 찾은 뒤 수량만 증가시키는 기능은 명확합니다. 다만,quantity가 0 이하일 경우 별도의 예외 처리가 없어, 부정확한 수량으로 갱신될 우려가 있습니다. 필요하다면 예외를 던지거나 UI 레이어에서 검증하는 로직을 추가해보면 좋겠습니다.
64-71:updateTotalQuantity()에서 수량 합산 후 20개를 초과하면IllegalArgumentException을 발생시키는 로직이 핵심적입니다. 도메인 특성에 맞는 커스텀 예외(예:CartQuantityLimitException)를 사용하는 방안을 고려해보시면, 이후 예외 처리를 구분하기가 더 용이할 수 있습니다.
87-90:removeLineItem는 라인 아이템 식별자로 아이템을 제거하고 수량을 갱신해줍니다. 만약 제거하려는 라인 아이템이 없을 때 로그 기록이나 별도 처리를 할 필요가 있다면, 조건 체크 후 동작을 수행하도록 보강할 수 있습니다.src/main/java/com/example/demo/application/CartService.java (2)
28-34: 상품 존재 여부 확인 후 장바구니에 추가하는 로직이 간결합니다. 그래도IllegalArgumentException대신 도메인 의미를 담은 커스텀 예외 사용을 고려해보면, 오류 처리가 더 명시적이 됩니다.
46-58: 장바구니가 세션에 없으면 자동으로 생성하고 저장하는 로직이 일관성 있게 짜여 있습니다. 다만, 여기서도cartRepository.findById(cartId)가 실패할 때IllegalArgumentException을 던지는 대신 커스텀 예외를 사용하면 예외 처리를 명확히 구분할 수 있을 것입니다.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/main/java/com/example/demo/application/CartService.java(1 hunks)src/main/java/com/example/demo/controllers/CartController.java(1 hunks)src/main/java/com/example/demo/controllers/LineItemController.java(1 hunks)src/main/java/com/example/demo/controllers/dto/CartResponseDto.java(1 hunks)src/main/java/com/example/demo/model/Cart.java(1 hunks)src/main/java/com/example/demo/model/LineItemId.java(1 hunks)src/test/java/com/example/demo/application/CartServiceTest.java(1 hunks)src/test/java/com/example/demo/controllers/CartControllerTest.java(2 hunks)src/test/java/com/example/demo/controllers/LineItemControllerTest.java(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/example/demo/controllers/dto/CartResponseDto.java
🧰 Additional context used
🧠 Learnings (1)
src/main/java/com/example/demo/controllers/LineItemController.java (1)
Learnt from: ahastudio
PR: dal-lab/wild-backend-cart-app#9
File: src/main/java/com/example/demo/controllers/LineItemController.java:30-30
Timestamp: 2025-04-11T08:22:11.429Z
Learning: 컨트롤러(UI Layer)에는 하드코딩된 엔티티 ID(예: cartId)를 포함하지 않아야 합니다. 이러한 ID는 사용자 인증, 세션 관리, 또는 서비스 계층을 통해 동적으로 결정되어야 합니다.
🧬 Code Graph Analysis (3)
src/test/java/com/example/demo/controllers/CartControllerTest.java (1)
src/test/java/com/example/demo/controllers/LineItemControllerTest.java (1)
WebMvcTest(23-67)
src/main/java/com/example/demo/controllers/CartController.java (1)
src/main/java/com/example/demo/controllers/LineItemController.java (1)
RestController(19-49)
src/main/java/com/example/demo/model/Cart.java (2)
src/main/java/com/example/demo/model/LineItem.java (1)
Entity(11-61)src/main/java/com/example/demo/model/Product.java (1)
Entity(7-42)
🔇 Additional comments (26)
src/main/java/com/example/demo/model/LineItemId.java (1)
1-15: UUID를 String으로 사용하는 접근 방식이 적절합니다.코드가 깔끔하고 간결하게 구현되었습니다.
LineItemId를 String 타입의 필드를 가진 record로 정의하고,@Embeddable애노테이션을 사용하여 JPA 엔티티에 내장될 수 있도록 설계한 것이 좋습니다.특히
generate()메서드에서 UUID를 문자열로 변환하여 사용하는 방식이 좋습니다. 이렇게 하면 식별자로서의 유니크함을 보장하면서도, 향후 식별자 생성 방식을 변경하거나 다른 형태의 값을 사용해야 할 때 유연하게 대응할 수 있습니다.src/test/java/com/example/demo/controllers/CartControllerTest.java (5)
3-5: 적절한 import 추가 확인.CartService와 Cart 모델을 사용하기 위한 import 문이 올바르게 추가되었습니다.
12-15: Mockito와 테스트 유틸리티 import 확인.verify, when 메서드와 함께 delete 요청을 위한 MockMvcRequestBuilders import가 추가되었습니다.
23-24: CartService 모킹 추가.CartService를 @MockBean으로 추가하여 컨트롤러 테스트를 위한 의존성을 적절하게 모킹했습니다.
26-35: getCart 메서드 테스트 로직 수정.메서드 이름을 detail에서 getCart로 변경하고, CartService.getCart() 호출 결과를 Mock하여 테스트 로직을 개선했습니다. 이제 컨트롤러가 실제로 CartService에서 Cart 객체를 받아 처리하는 것을 테스트합니다.
37-45: deleteCart 테스트 메서드 추가.장바구니 삭제 기능에 대한 테스트가 적절하게 추가되었습니다.
- HTTP DELETE 요청 시 204 No Content 상태가 반환되는지 확인
- cartService.clearCart() 메서드가 실제로 호출되는지 verify하여 검증
테스트 케이스가 명확하고 기능 검증에 충분합니다.
src/test/java/com/example/demo/controllers/LineItemControllerTest.java (6)
3-7: 필요한 import 추가 확인.CartService, LineItemId, ProductId, ProductOption 등의 클래스를 사용하기 위한 import 문이 적절히 추가되었습니다.
15-19: UUID 및 Mockito 관련 import 추가.UUID 생성과 mockito의 eq, verify 메서드 그리고 DELETE 요청을 위한 import가 적절히 추가되었습니다.
28-29: CartService 모킹 추가.CartService를 @MockBean으로 추가하여 컨트롤러 테스트를 위한 의존성을 적절하게 설정했습니다.
37-38: JSON 요청 데이터 업데이트.LineItem 생성 요청에 필요한 color와 size 필드가 JSON 데이터에 추가되었습니다.
48-52: CartService.addItemToCart 호출 검증 업데이트.CartService의 addItemToCart 메서드가 올바른 파라미터(ProductId, ProductOption, quantity)로 호출되는지 검증하도록 업데이트되었습니다.
55-66: removeProduct 테스트 메서드 추가.LineItem 삭제 기능에 대한 테스트 메서드가 적절하게 추가되었습니다:
- 랜덤 UUID를 사용하여 lineItemId 생성
- DELETE 요청 후 204 No Content 상태 코드 확인
- CartService.removeLineItem 메서드가 올바른 LineItemId로 호출되는지 검증
테스트가 명확하고 동작을 제대로 검증합니다.
src/main/java/com/example/demo/controllers/CartController.java (4)
3-12: 필요한 import 추가.CartService, DTO 클래스, ResponseEntity 등 필요한 import가 적절히 추가되었습니다.
17-22: CartService 의존성 주입 추가.생성자를 통해 CartService 의존성을 주입받도록 적절히 구현되었습니다. final 필드로 선언하여 불변성을 보장한 점도 좋습니다.
25-40: getCart 메서드 구현.장바구니 정보를 조회하는 기능이 잘 구현되었습니다:
- CartService에서 현재 장바구니 정보를 가져옴
- 총 수량과 함께 각 LineItem을 DTO로 변환하여 응답
- Stream API를 사용하여 LineItem 목록을 효율적으로 변환
코드가 간결하고 가독성이 좋습니다.
42-47: deleteCart 메서드 구현.장바구니를 비우는 기능이 적절하게 구현되었습니다:
- CartService.clearCart()를 호출하여 장바구니 비우기
- 204 No Content 상태 코드 반환
간결하고 RESTful한 방식으로 구현되었습니다.
src/test/java/com/example/demo/application/CartServiceTest.java (5)
39-55: 세션에 기존CART_ID가 있을 때 로직이 올바르게 동작하는지 확인하는 테스트입니다. 하지만, 세션에는 ID가 존재하지만 레포지토리에 해당 ID의 장바구니가 없을 때(삭제되었거나 DB에 없는 경우)를 테스트해 보면 어떨까요? 더 견고한 테스트가 될 것입니다.
57-73: 새로운 장바구니를 생성해 세션에 저장하는 로직을 깔끔하게 검증하고 있습니다. 이 부분은 특별한 문제 없이 보이며 재사용성도 좋아 보입니다.
75-93: 정상적인 상품 추가를 잘 테스트하고 있습니다. 하지만, 상품이 존재하지 않을 때 또는 수량이 유효하지 않을 때(예: 0 이하) 예외가 정상적으로 발생하는지에 대한 테스트도 추가하면 더욱 완벽하겠습니다.
95-109: 장바구니 비우기 기능 테스트가 간결하고 명확합니다. 기능적으로 문제가 없어 보이며, 유지보수성도 뛰어납니다.
111-126: 라인 아이템 제거 기능을 정상 시나리오로 잘 검증하고 있습니다. 존재하지 않는 라인 아이템을 지우려 할 때 예외나 로그 처리 등이 필요한 경우도 고려할 수 있으면 더욱 견고하겠습니다.src/main/java/com/example/demo/controllers/LineItemController.java (3)
23-25:CartService를 의존성 주입받는 생성자 구성이 적절합니다. 컨트롤러 계층에서 비즈니스 로직 분리가 잘 이루어진 모습입니다.
31-39:LineItemRequestDto를@Valid로 받아오는 부분이 만족스럽습니다. 다만 DTO 필드 유효성(예: 수량이 0 이하인지 검사)도 혹시 누락이 없는지 다시 한 번 확인해 보시면 좋겠습니다.
41-47: 라인 아이템 삭제 로직이 단순하고 직관적입니다. 존재하지 않는 아이템을 제거할 때의 예외 처리 또는 피드백 처리가 필요하다면 추가 검토가 필요할 수 있습니다.src/main/java/com/example/demo/application/CartService.java (2)
36-39: 라인 아이템 제거 시, 라인 아이템이 없으면 아무 일도 일어나지 않는데, 비정상적인 호출인지 판단하여 예외를 던지거나 로깅을 수행할 필요가 있는지 확인해보세요.
41-44:clearCart()메서드는 특별히 복잡한 로직 없이 빠르게 장바구니를 비울 수 있어 좋습니다.
| private final CartRepository cartRepository; | ||
| private final ProductRepository productRepository; | ||
|
|
||
| private final HttpSession session; |
There was a problem hiding this comment.
Application Layer는 UI Layer를 몰라야 합니다. Application과 외부를 나눠주는 걸 잊으면 안 됩니다. Spring을 그게 가능하게 해주죠.
| } | ||
|
|
||
| public Cart getCart() { | ||
| Long cartId = (Long) session.getAttribute("CART_ID"); |
There was a problem hiding this comment.
두 가지 문제가 있습니다.
- Application Layer는 UI Layer, 즉 웹과 아무런 관련이 없어야 합니다.
- 웹이 Cart ID를 관리하려고 합니다.
특히 2번은 UI Layer, Controller에서 Cart ID를 관리하던 걸 Application Layer로 옮겨왔기 때문에 더 나쁩니다.
중요한 건 그 누구도 Cart ID를 관리하지 않아야 한다는 겁니다.
일반적인 흐름은 다음과 같습니다.
- UI Layer에서 Authorization 헤더를 이용해 사용자를 인증. User ID를 확인.
- Application Layer로 User ID를 전달.
- Application Layer에서 현재 사용자의 장바구니 객체를 확보.
Cart cart = cartRepository.findByUserId(userId);
| } | ||
|
|
||
| public void addItemToCart(ProductId productId, ProductOption option, int quantity) { | ||
| Cart cart = getCart(); |
There was a problem hiding this comment.
Application Service의 곳곳에서 getCart()가 쓰이는데, 사용자 인증 등이 들어오면 이 코드를 모두 고쳐야 합니다. 유지보수하기 어려운 방식이죠. 각 Layer를 구분하고, 각각 책임지는 부분을 명확히 하는 이유는 어떤 변경이 다른 모든 걸 다 고치는 상황을 피하기 위함입니다.
제가 일반적인 흐름(#9 (comment) )을 제시하긴 했지만 이걸 그대로 외우시면 안 되고, 왜 이렇게 했는지 파악하는 게 필요합니다. 이해가 안 되는 부분이 있으면 바로 디스코드 채널에 남겨주세요. 기계적으로 “A가 문제니까 그걸 피하기 위해 B를 선택한다”를 하면 “그래서 C와 D 문제가 발생한다”로 이어질 수 있어요.
| cart = mock(Cart.class); | ||
| } | ||
|
|
||
| @DisplayName("세션에 CART_ID가 존재하면 해당 ID로 장바구니를 조회한다") |
There was a problem hiding this comment.
Application Layer에서 “세션에”라는 표현을 쓰는 순간, “왜 Application 내부에서 Application 외부의 기술 요소를 묘사하려고 하지?”라고 질문해야 합니다. 테스트를 먼저 작성하면, 테스트 코드보다 테스트 계획, 테스트에 대한 설명을 먼저 작성하면 설계 문제를 매우 빠르게 발견할 수 있습니다.
| @Test | ||
| @DisplayName("DELETE /cart/line-items/{lineItemId}") | ||
| void removeProduct() throws Exception { | ||
| String lineItemId = UUID.randomUUID().toString(); |
There was a problem hiding this comment.
위에 LineItemId lineItemId = new LineItemId("lineItem-1");라고 쓴 걸 여기에 그대로 사용하셔도 됩니다. UUID가 핵심은 아니기도 하고, 테스트 코드에선 로직 대신 하드코딩을 하는 게 권장됩니다. 예를 들어 이 테스트가 실패했을 때 전부 하드코딩되어 있고, 테스트 대상만 어떤 로직을 갖고 있다면 테스트가 실패한 부분이 로직임이 더 쉽게 드러나죠.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
src/main/java/com/example/demo/exception/CartQuantityLimitException.java (1)
1-7: 예외 클래스가 올바르게 구현되었습니다.장바구니 수량 제한 예외를 처리하기 위한 예외 클래스가 적절히 구현되었습니다. RuntimeException을 상속받아 unchecked 예외로 처리한 것은 적절한 선택입니다.
추가 개선 사항으로 다음을 고려해 보세요:
public class CartQuantityLimitException extends RuntimeException { public CartQuantityLimitException() { super("장바구니에 담을 수 있는 수량을 초과했습니다."); } + + public CartQuantityLimitException(int currentQuantity, int maxQuantity) { + super(String.format("장바구니에 담을 수 있는 수량을 초과했습니다. 현재: %d, 최대: %d", currentQuantity, maxQuantity)); + } }src/main/java/com/example/demo/exception/CartNotFoundException.java (1)
1-7: 예외 클래스가 올바르게 구현되었습니다.장바구니를 찾을 수 없을 때 발생하는 예외 클래스가 적절히 구현되었습니다. RuntimeException을 상속받아 unchecked 예외로 처리한 것은 적절한 선택입니다.
추가 개선 사항으로 다음을 고려해 보세요:
public class CartNotFoundException extends RuntimeException { public CartNotFoundException() { super("장바구니를 찾을 수 없습니다."); } + + public CartNotFoundException(String userId) { + super(String.format("사용자 ID가 '%s'인 장바구니를 찾을 수 없습니다.", userId)); + } }src/main/java/com/example/demo/exception/UnauthorizedException.java (1)
1-7: 예외 클래스가 올바르게 구현되었습니다.인증이 필요할 때 발생하는 예외 클래스가 적절히 구현되었습니다. RuntimeException을 상속받아 unchecked 예외로 처리한 것은 적절한 선택입니다.
추가 개선 사항으로 다음을 고려해 보세요:
public class UnauthorizedException extends RuntimeException { public UnauthorizedException() { super("인증이 필요합니다."); } + + public UnauthorizedException(String reason) { + super(String.format("인증이 필요합니다. 사유: %s", reason)); + } }src/main/java/com/example/demo/util/AuthorizationUtils.java (1)
5-16: 인증 유틸리티 클래스가 잘 설계되었습니다.유틸리티 클래스를 인스턴스화 방지를 위한 private 생성자를 사용한 점이 좋습니다.
다만 다음 사항을 고려해보세요:
- "Bearer " 접두사를 상수로 정의하여 코드의 유지보수성을 높이기
- 메소드에 대한 JavaDoc 주석 추가하기
- 추출된 사용자 ID에 대한 추가 검증 로직 고려하기 (예: 빈 문자열 체크)
public class AuthorizationUtils { + private static final String BEARER_PREFIX = "Bearer "; + private AuthorizationUtils() { } + /** + * Authorization 헤더에서 사용자 ID를 추출합니다. + * + * @param authorization Authorization 헤더 값 + * @return 추출된 사용자 ID + * @throws UnauthorizedException 인증 정보가 유효하지 않은 경우 + */ public static String extractUserIdFromAuthorization(String authorization) { - if (authorization == null || !authorization.startsWith("Bearer ")) { + if (authorization == null || !authorization.startsWith(BEARER_PREFIX)) { throw new UnauthorizedException(); } - return authorization.substring(7); + String userId = authorization.substring(BEARER_PREFIX.length()); + if (userId.isBlank()) { + throw new UnauthorizedException(); + } + return userId; } }src/test/java/com/example/demo/controllers/LineItemControllerTest.java (2)
60-74: 상품 제거 테스트 코드가 잘 구현되었습니다.LineItem 제거 기능에 대한 테스트 케이스가 잘 구현되었습니다. 하지만 이전 리뷰 코멘트를 참고하여 하드코딩된 값을 일관되게 사용하는 것이 좋습니다.
@Test @DisplayName("DELETE /cart/line-items/{lineItemId}") void removeProduct() throws Exception { String userId = "userA"; - String lineItemId = "lineItem-1"; + LineItemId lineItemId = new LineItemId("lineItem-1"); - mockMvc.perform(delete("/cart/line-items/{lineItemId}", lineItemId) + mockMvc.perform(delete("/cart/line-items/{lineItemId}", lineItemId.id()) .header("Authorization", "Bearer " + userId)) .andExpect(status().isNoContent()); verify(cartService).removeLineItem( eq(userId), - eq(new LineItemId(lineItemId)) + eq(lineItemId) ); }
23-74: 추가적인 테스트 케이스를 고려해보세요.현재 긍정적인 시나리오에 대한 테스트만 있습니다. 다음과 같은 예외 상황에 대한 테스트도 추가하면 좋을 것 같습니다:
- 인증 헤더가 없는 경우 (401 응답)
- 잘못된 라인 아이템 ID 형식의 경우
- 존재하지 않는 라인 아이템 ID의 경우 (404 응답)
CartControllerTest의 예외 처리 테스트를 참고하면 도움이 될 것 같습니다.
src/main/java/com/example/demo/controllers/LineItemController.java (2)
36-48: 메서드 명을 좀 더 직관적으로 변경을 고려해 보세요.
현재 메서드 이름이create로 되어 있어 수행 동작(장바구니 아이템 추가)이 명확하게 드러나지 않습니다.addLineItem등의 이름을 사용하면 가독성이 높아질 수 있을 것 같습니다.
56-60: 중복된 인증 로직을 공통화하는 방안을 고려해 보세요.
create메서드와deleteLineItem메서드 모두에서extractUserIdFromAuthorization를 호출하고 있습니다. 반복 사용되는 인증 로직을 공통 메서드로 추출하거나 필터, 인터셉터 등을 도입하면 유지보수성과 확장성이 향상될 것입니다.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
src/main/java/com/example/demo/application/CartService.java(1 hunks)src/main/java/com/example/demo/controllers/CartController.java(1 hunks)src/main/java/com/example/demo/controllers/LineItemController.java(1 hunks)src/main/java/com/example/demo/exception/CartNotFoundException.java(1 hunks)src/main/java/com/example/demo/exception/CartQuantityLimitException.java(1 hunks)src/main/java/com/example/demo/exception/ErrorResponseDto.java(1 hunks)src/main/java/com/example/demo/exception/GlobalExceptionHandler.java(1 hunks)src/main/java/com/example/demo/exception/ProductNotFoundException.java(1 hunks)src/main/java/com/example/demo/exception/UnauthorizedException.java(1 hunks)src/main/java/com/example/demo/model/Cart.java(1 hunks)src/main/java/com/example/demo/model/LineItem.java(1 hunks)src/main/java/com/example/demo/repository/CartRepository.java(1 hunks)src/main/java/com/example/demo/util/AuthorizationUtils.java(1 hunks)src/test/java/com/example/demo/application/CartServiceTest.java(1 hunks)src/test/java/com/example/demo/controllers/CartControllerTest.java(1 hunks)src/test/java/com/example/demo/controllers/LineItemControllerTest.java(1 hunks)src/test/java/com/example/demo/model/CartTest.java(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/main/java/com/example/demo/exception/ErrorResponseDto.java
🚧 Files skipped from review as they are similar to previous changes (2)
- src/test/java/com/example/demo/model/CartTest.java
- src/main/java/com/example/demo/model/LineItem.java
🧰 Additional context used
🧬 Code Graph Analysis (8)
src/main/java/com/example/demo/util/AuthorizationUtils.java (1)
src/main/java/com/example/demo/exception/UnauthorizedException.java (1)
UnauthorizedException(3-7)
src/main/java/com/example/demo/controllers/CartController.java (2)
src/main/java/com/example/demo/util/AuthorizationUtils.java (1)
AuthorizationUtils(5-17)src/main/java/com/example/demo/controllers/LineItemController.java (1)
RestController(24-62)
src/test/java/com/example/demo/controllers/LineItemControllerTest.java (1)
src/test/java/com/example/demo/controllers/CartControllerTest.java (1)
WebMvcTest(21-97)
src/test/java/com/example/demo/application/CartServiceTest.java (2)
src/main/java/com/example/demo/exception/CartNotFoundException.java (1)
CartNotFoundException(3-7)src/main/java/com/example/demo/exception/ProductNotFoundException.java (1)
ProductNotFoundException(5-9)
src/main/java/com/example/demo/controllers/LineItemController.java (2)
src/main/java/com/example/demo/exception/UnauthorizedException.java (1)
UnauthorizedException(3-7)src/main/java/com/example/demo/util/AuthorizationUtils.java (1)
AuthorizationUtils(5-17)
src/main/java/com/example/demo/application/CartService.java (2)
src/main/java/com/example/demo/exception/CartNotFoundException.java (1)
CartNotFoundException(3-7)src/main/java/com/example/demo/exception/ProductNotFoundException.java (1)
ProductNotFoundException(5-9)
src/test/java/com/example/demo/controllers/CartControllerTest.java (2)
src/main/java/com/example/demo/exception/CartNotFoundException.java (1)
CartNotFoundException(3-7)src/test/java/com/example/demo/controllers/LineItemControllerTest.java (1)
WebMvcTest(23-75)
src/main/java/com/example/demo/model/Cart.java (2)
src/main/java/com/example/demo/exception/CartQuantityLimitException.java (1)
CartQuantityLimitException(3-7)src/main/java/com/example/demo/model/LineItem.java (1)
Entity(8-58)
🔇 Additional comments (58)
src/main/java/com/example/demo/exception/ProductNotFoundException.java (1)
1-9: 예외 클래스가 올바르게 구현되었습니다.상품을 찾을 수 없을 때 발생하는 예외 클래스가 잘 구현되었습니다. ProductId를 매개변수로 받아 오류 메시지에 포함시키는 것은 매우 좋은 접근 방식입니다. 이렇게 하면 디버깅과 로깅이 훨씬 효과적입니다.
src/main/java/com/example/demo/repository/CartRepository.java (1)
8-10: 적절한 리포지토리 인터페이스 설계입니다.JpaRepository를 상속하고 사용자 ID로 장바구니를 찾는 메소드를 정의하여 기능적으로 잘 구현되었습니다. Optional을 사용하여 결과가 없을 수 있는 상황을 적절히
표현한 것도 좋은 방식입니다.src/test/java/com/example/demo/controllers/LineItemControllerTest.java (3)
3-21: 적절한 임포트 구문들입니다.필요한 클래스들을 모두 임포트하고 있습니다. 모킹 및 테스트에 필요한 정적 메소드들도 올바르게 임포트되어 있습니다.
29-30: CartService 모킹 추가가 적절합니다.LineItemController가 CartService를 의존하고 있으므로 테스트에서 모킹하는 것이 적절합니다.
34-58: 상품 추가 테스트 코드가 개선되었습니다.사용자 ID, 색상, 사이즈 파라미터를 포함하도록 테스트가 업데이트되었고, 검증 부분도 이에 맞게 수정되었습니다.
src/test/java/com/example/demo/application/CartServiceTest.java (6)
23-52: 장바구니 조회 테스트가 잘 구현되었습니다.CartService의 getCart 메소드에 대한 테스트가 명확하게 작성되었습니다. 테스트 설명, 준비(given), 실행(when), 검증(then) 구조가 잘 갖추어져 있습니다.
54-63: 장바구니를 찾을 수 없는 예외 상황 테스트가 적절합니다.장바구니를 찾을 수 없는 상황에 대한 예외 처리 테스트가 적절히 구현되어 있습니다.
65-84: 상품 추가 테스트가 잘 구현되었습니다.장바구니에 상품을 추가하는 메소드에 대한 테스트가 명확하게 작성되었습니다.
86-102: 상품을 찾을 수 없는 예외 상황 테스트가 적절합니다.존재하지 않는 상품을 추가하려는 상황에 대한 예외 처리 테스트가 적절히 구현되어 있습니다.
104-118: 장바구니 비우기 테스트가 잘 구현되었습니다.장바구니를 비우는 메소드에 대한 테스트가 명확하게 작성되었습니다.
120-135: 라인 아이템 제거 테스트가 잘 구현되었습니다.장바구니에서
라인 아이템을 제거하는 메소드에 대한 테스트가 명확하게 작성되었습니다.src/main/java/com/example/demo/controllers/CartController.java (12)
3-7: 의존성 주입 정상 동작 확인
CartService및 DTO 관련 클래스를 정상적으로 import하고 있어 문제 없습니다. 수정사항에 특이점은 없어 보입니다.
8-9: HTTP 관련 의존성 추가 정상
HttpStatus와ResponseEntity관련 의존성이 정상적으로 추가되었습니다.
10-12: 경로 매핑 적절성 검토
@DeleteMapping애노테이션과@RequestHeader사용이 올바르며, RESTful API 설계에 부합합니다.
16-16: 정적 import 사용
AuthorizationUtils의 정적 메서드를 직접 import하는 방식이 코드 가독성을 높여주고 있습니다.
22-22: DI 필드 선언
private final CartService cartService;는 불변성을 유지하며, 생성자 주입 방식과 일관성을 가지므로 좋습니다.
24-26: 생성자를 통한 주입
의존성 주입 기본 패턴을 제대로 따르고 있습니다. 특별한 문제 없음.
30-32: 카트 조회 메서드
매핑된 URL과 메서드 시그니처가 직관적이며,required = false는 비로그인 사용자 시나리오를 고려한 것으로 보입니다. 다만, 무허가 접근 시UnauthorizedException발생 가능성을 염두에 두어야 합니다.
34-35: userId 추출 및 CartService 호출
extractUserIdFromAuthorization를 통해userId를 가져와cartService.getCart(userId)호출. 예외 발생 시GlobalExceptionHandler에서 처리되므로 흐름이 적절합니다.
37-47: DTO 변환 로직
CartResponseDto와LineItemResponseDto로 변환 과정이 직관적입니다. Stream API 활용도 적절하며, 각 프로퍼티 매핑이 정확한지 확인해보면 좋겠습니다.카트에 담긴
lineItem들의 필드가 누락 없이 DTO에 매핑되는지 테스트 코드를 통해 점검해 주세요.
49-50: 응답 반환 문제 없음
HttpStatus.OK와 함께 DTO 반환. 정상 동작에 문제 없어 보입니다.
53-56: 장바구니 비움 API 설계
@DeleteMapping()등록 시 URL과 메서드 명이 일치해 가독성이 좋으며,required = false를 통해 비로그인 사용자 접근 가능성을 고려한 점은 주의가 필요합니다.
58-61: 카트 비움 처리
clearCart(userId)호출 후204 No Content응답은 REST 표준에 부합합니다. 별다른 문제 없습니다.src/main/java/com/example/demo/exception/GlobalExceptionHandler.java (6)
1-8: 글로벌 예외 처리 설정
@RestControllerAdvice를 통한 전역 예외 처리 구성이 잘 되어 있습니다. 다양한 커스텀 예외를 일관된 방식으로 처리하는 점이 좋습니다.
10-11: 예외 처리 클래스 정의
GlobalExceptionHandler클래스 선언 자체는 문제 없습니다. 확장성도 좋아 보입니다.
13-17: CartNotFoundException 처리
"장바구니를 찾을 수 없습니다." 상황에 대해CART_NOT_FOUND에러 코드를 던지며 404를 반환. 사용자 입장에서 명확한 응답입니다.
19-23: ProductNotFoundException 처리
상품 미존재 시 404 반환 로직이 일관적으로 처리되고 있습니다.
25-32: CartQuantityLimitException 처리
400 (BAD_REQUEST) 코드와 함께 JSON 형식으로 에러 정보를 보내는 부분이 명확합니다. 수량 제한에 대한 사용자 친화적 예외 응답을 제공하고 있습니다.
34-38: UnauthorizedException 처리
401 (UNAUTHORIZED) 코드와 함께 응답. 인증 필요한 API에서 보편적으로 사용되는 방식이며, 현재 구조를 봤을 때 확실히 의도대로 동작합니다.src/test/java/com/example/demo/controllers/CartControllerTest.java (8)
3-5: 테스트 대상 의존성 주입
CartService,CartNotFoundException,Cart클래스 import 정상. 테스트에 필요한 핵심 구성요소가 잘 모킹/활용되고 있습니다.
10-11: MockBean 활용
@MockBean을 통해CartService를 목 객체로 주입받는 방식이 적절합니다.
14-16: 테스트 메서드 import
verify,when,delete등 모키토와 스프링 MVC 테스트 기능이 정상적으로 import되었고 적절히 사용되고 있습니다.
26-28: CartService 모킹
테스트의 명확한 의도를 위해@MockBean private CartService cartService;선언이 적절하며 setup 과정 없이 테스트 내에서when구문으로 원하는 동작을 지정하고 있습니다.
31-40: GET /cart 정상 플로우 테스트
cartService.getCart(userId)->mockMvc.perform(get("/cart"))-> 200 OK 검증 흐름이 본연의 기능을 잘 테스트하고 있습니다.
42-61: GET /cart - 인증 실패 시나리오 테스트
인증 헤더가 없을 때401 UNAUTHORIZED및json반환을 확인. 에러 코드와 메시지가 일치하는지 검사하는 점에서 테스트 완성도가 높습니다.
63-84: GET /cart - 장바구니 없음 시나리오 테스트
CartNotFoundException모킹 후 404 응답 기대 테스트. 사용자가 없는 카트에 접근할 때 정상적으로 예외 처리되는지 명확히 검증합니다.
86-95: DELETE /cart 테스트
mockMvc.perform(delete("/cart"))후NO_CONTENT응답 확인,cartService.clearCart(userId)호출을 검증하는 로직으로 기능에 충실합니다.src/main/java/com/example/demo/model/Cart.java (20)
1-2: 모델 패키지 구조
package com.example.demo.model;→ 계층 구조가 명확하며, 도메인 모델로서의 역할이 분리되어 있습니다.
3-3: CartQuantityLimitException 의존성
수량 제한 예외에 대한 의존성 import가 정확히 되어 있습니다.
4-11: JPA 관련 애노테이션 추가
@Entity,@Table, 그리고@OneToMany(cascade = CascadeType.ALL, orphanRemoval = true)설정이 일관적이며, DB 매핑 구조도 적절해 보입니다.
13-15: 필드 import
ArrayList,Collections,List활용이 적절하며, list를 외부에 제공할 때unmodifiableList로 감싸는 구현이 안전성을 높입니다.
17-19: 클래스 정의
@Entity,@Table(name="carts")로 Cart 엔티티가 정의되었습니다. 도메인과 매핑하는 테이블 명이 일치해 가독성이 좋습니다.
21-23: 기본 키 설정
@Id,@GeneratedValue(strategy = GenerationType.IDENTITY)설정이 올바르며, Long 타입을 사용하는 점도 무난합니다.
25-26: userId 컬럼
@Column(nullable = false)설정을 통해 null 불가 제약을 명시. DB 관점에서 적절합니다.
28-29: 연관 관계 설정
mappedBy = "cart"설정이 어느 쪽이 주인(Entity 주인)인지 보여주며,cascade = CascadeType.ALL과orphanRemoval설정으로 엔티티 생명주기를 일관성 있게 유지합니다.
31-31: totalQuantity 필드
장바구니의 총 아이템 수량 관리를 위한 필드로 명확히 드러납니다.
33-35: 기본 생성자
JPA 표준에 따른 기본 생성자 제공. 문제 없습니다.
36-38: 단일 인자 생성자
userId값을 초기화. 사용하는 곳에서 객체 생성을 간단히 처리할 수 있습니다.
40-44: userId, lineItems 동시 초기화
updateTotalQuantity()메서드 호출로 초기 전체 수량까지 맞추는 로직이 자연스럽습니다.
46-48: getId 메서드
식별자 조회. 정상적이며 간결합니다.
50-52: getLineItems 메서드
unmodifiableList로 반환함으로써 외부 변화로부터lineItems를 안전하게 보호하려는 의도가 좋습니다.
54-56: getTotalQuantity 메서드
필드 조회만 수행. 로직상 결함 없으며 명확합니다.
58-71: addProduct 로직
이미 존재하는LineItem이면 수량만 추가, 없으면 새로 추가 후updateTotalQuantity()호출. 장바구니 기능을 잘 캡슐화했습니다.
73-81: updateTotalQuantity 내부 구현
totalQuantity를 다시 계산 후, 20개 초과 시CartQuantityLimitException이 발생. 수량 제한로직이 단순명료합니다.
83-90: findLineItem 메서드
Stream API를 사용하여 제품과 옵션이 일치하는LineItem을 찾는 로직. 문제없으며 null 반환 방식도 명확합니다.
92-95: clearItems 메서드
장바구니 전체를 비운 뒤,updateTotalQuantity()로 합계 재계산. 일관성 보장이 잘 이뤄지고 있습니다.
97-99: removeLineItem 메서드
특정LineItemId를 가진 항목을 제거 후 수량 재계산. 불변식 유지에 충실합니다.src/main/java/com/example/demo/application/CartService.java (1)
44-47:CartNotFoundException사용을 환영합니다.
장바구니가 없을 때 의미 있는 예외를 명시적으로 던져, 유지보수성과 가독성이 향상되었습니다.
| class CartServiceTest { | ||
|
|
||
| private CartRepository cartRepository; | ||
| private ProductRepository productRepository; | ||
| private CartService cartService; | ||
| private Cart cart; | ||
|
|
||
| @BeforeEach | ||
| void setUp() { | ||
| cartRepository = mock(CartRepository.class); | ||
| productRepository = mock(ProductRepository.class); | ||
| cartService = new CartService(cartRepository, productRepository); | ||
| cart = mock(Cart.class); | ||
| } | ||
|
|
||
| @DisplayName("userId가 있을때 카트를 찾을수있다.") | ||
| @Test | ||
| void getCart() { | ||
| // given | ||
| String userId = "userA"; | ||
| when(cartRepository.findByUserId(userId)) | ||
| .thenReturn(Optional.of(cart)); | ||
|
|
||
| // when | ||
| Cart result = cartService.getCart(userId); | ||
|
|
||
| // then | ||
| assertThat(result).isSameAs(cart); | ||
| verify(cartRepository).findByUserId(userId); | ||
| } | ||
|
|
||
| @DisplayName("장바구니를 못찾으면 예외가 발생한다.") | ||
| @Test | ||
| void cannotFindCart() { | ||
| String userId = "userA"; | ||
| when(cartRepository.findByUserId(userId)) | ||
| .thenReturn(Optional.empty()); | ||
|
|
||
| assertThatThrownBy(() -> cartService.getCart(userId)) | ||
| .isInstanceOf(CartNotFoundException.class); | ||
| } | ||
|
|
||
| @DisplayName("상품을 장바구니에 추가한다") | ||
| @Test | ||
| void addItemToCart_AddsProductToCart() { | ||
| // given | ||
| String userId = "userA"; | ||
| ProductId productId = new ProductId("product-1"); | ||
| ProductOption option = new ProductOption("Red", "L"); | ||
| int quantity = 2; | ||
|
|
||
| when(cartRepository.findByUserId(userId)) | ||
| .thenReturn(Optional.of(cart)); | ||
| when(productRepository.existsById(productId)) | ||
| .thenReturn(true); | ||
|
|
||
| // when | ||
| cartService.addItemToCart(userId, productId, option, quantity); | ||
|
|
||
| // then | ||
| verify(cart).addProduct(productId, option, quantity); | ||
| } | ||
|
|
||
| @DisplayName("장바구니에 넣을 상품이 존재하지 않으면 예외가 발생한다.") | ||
| @Test | ||
| void throwExceptionWhenProductNotFound() { | ||
| String userId = "userA"; | ||
| ProductId productId = new ProductId("product-1"); | ||
| ProductOption option = new ProductOption("Red", "L"); | ||
| int quantity = 2; | ||
|
|
||
| when(cartRepository.findByUserId(userId)) | ||
| .thenReturn(Optional.of(cart)); | ||
| when(productRepository.existsById(productId)) | ||
| .thenReturn(false); | ||
|
|
||
| assertThatThrownBy( | ||
| () -> cartService.addItemToCart(userId, productId, option, quantity)) | ||
| .isInstanceOf(ProductNotFoundException.class); | ||
| } | ||
|
|
||
| @DisplayName("장바구니를 비울 수 있다.") | ||
| @Test | ||
| void clearCart() { | ||
| // given | ||
| String userId = "userA"; | ||
|
|
||
| when(cartRepository.findByUserId(userId)) | ||
| .thenReturn(Optional.of(cart)); | ||
|
|
||
| // when | ||
| cartService.clearCart(userId); | ||
|
|
||
| // then | ||
| verify(cart).clearItems(); | ||
| } | ||
|
|
||
| @DisplayName("장바구니에서 라인 아이템을 제거한다") | ||
| @Test | ||
| void removeLineItem() { | ||
| // given | ||
| String userId = "userA"; | ||
| LineItemId lineItemId = new LineItemId("lineItem-1"); | ||
|
|
||
| when(cartRepository.findByUserId(userId)) | ||
| .thenReturn(Optional.of(cart)); | ||
|
|
||
| // when | ||
| cartService.removeLineItem(userId, lineItemId); | ||
|
|
||
| // then | ||
| verify(cart).removeLineItem(lineItemId); | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
비즈니스 로직 테스트 위치에 대한 고려가 필요합니다.
이전 리뷰 코멘트에서 언급된 내용처럼 비즈니스 로직은 Application Layer보다 Aggregate(도메인 모델)에서 테스트하는 것이 좋습니다. 현재 테스트는 많은 모킹을 필요로 하고 있으며, 이는 테스트의 가치를 떨어뜨릴 수 있습니다.
또한 아래와 같은 테스트 케이스를 추가하는 것이 좋겠습니다:
- 최대 수량 제한을 초과하는 경우에 대한 테스트
- 이미 장바구니에 있는 상품과 동일한 상품을 다른 옵션으로 추가하는 경우
- 동일한 상품과 옵션을 추가할 때 수량이 합쳐지는지 테스트
🌐 Web query:
DDD에서 비즈니스 로직 테스트를 위한 최적의 위치는 어디인가요?
💡 Result:
DDD(Domain-Driven Design)에서 비즈니스 로직 테스트의 최적 위치는 **도메인 레이어(Domain Layer)**입니다. 이는 도메인 모델이 비즈니스 규칙과 로직을 캡슐화하는 핵심 계층이기 때문입니다. 아래는 구체적인 근거와 방법입니다:
1. 도메인 엔티티/값 객체 내부 테스트
- 비즈니스 로직이 도메인 객체 내부에 위치할 경우, 해당 객체의 메서드를 직접 테스트하는 것이 가장 효과적입니다[1][3][10].
// 예: 계좌 엔티티의 입금 로직 테스트 public class AccountTest { @Test void deposit_ValidAmount_IncreasesBalance() { Account account = new Account("id", BigDecimal.ZERO); account.deposit(new BigDecimal("100")); assertEquals(new BigDecimal("100"), account.getBalance()); } }
- 객체의 응집도가 높아지며, 외부 의존성 없이 순수 로직 검증이 가능합니다[1][3].
2. 도메인 서비스 테스트
- 여러 엔티티를 조율하거나 복잡한 규칙을 처리하는 로직은 도메인 서비스로 구현하고 테스트합니다[10].
// 예: 송금 유효성 검사 서비스 테스트 public class TransactionValidatorTest { @Test void isValid_InvalidCurrency_ReturnsFalse() { Account from = new Account(USD); Account to = new Account(KRW); assertFalse(validator.isValid(new Money(100, USD), from, to)); } }
- 도메인 서비스는 상태 없이 프로세스 중심으로 동작하므로, 테스트 케이스 작성이 용이합니다[10].
3. 테스트 주도 개발(TDD) 적용
- 테스트를 먼저 작성하여 도메인 모델의 설계를 검증합니다[2][4][6].
- 예: 고객 이름 설정 기능 테스트 → 도메인 모델 구현 → 리팩토링[4].
- 이 접근법은 도메인 전문가와의 협업을 통해 요구사항을 정확히 반영하는 데 도움이 됩니다[6][7].
4. 계층별 테스트 전략
- 서비스 레이어(Application Layer): 도메인 객체 조율 로직만 테스트하며, 통합 테스트로 보완[1][10].
- 인프라 레이어(Infrastructure Layer): DB/외부 시스템 연동은 격리된 환경에서 테스트[3][10].
- 표현 레이어(Presentation Layer): 사용자 인터페이스와 분리하여 테스트[10].
5. 장점과 고려사항
-
장점:
- 도메인 로직의 변경 사항을 신속히 감지할 수 있습니다[1][4].
- 의존성 주입(DI) 최소화로 테스트 복잡도 감소[1].
- 유지보수성 향상: 도메인 객체만 수정하면 테스트가 자동으로 보호됩니다[1][3].
-
주의점:
- 외부 시스템 연동이 필요한 로직은 통합 테스트로 보완해야 합니다[3][6].
- Bounded Context별로 테스트 범위를 명확히 구분해야 합니다[7].
요약
DDD에서 비즈니스 로직은 도메인 레이어에 집중되어야 하며, 해당 로직의 테스트는 도메인 객체(엔티티/값 객체/서비스)의 단위 테스트를 통해 수행됩니다. 이는 객체 지향 설계 원칙을 준수하고, 복잡한 비즈니스 규칙을 효과적으로 검증하는 최적의 접근법입니다[1][3][10].
Citations:
- 1: https://velog.io/@yyy96/DDD
- 2: https://f-lab.kr/insight/domain-driven-design-test-driven-development
- 3: https://dogfootsleep.tistory.com/43
- 4: https://lovon.tistory.com/152
- 5: https://andrew75313.tistory.com/180
- 6: http://thefarmersfront.github.io/blog/road-to-ddd/
- 7: https://happycloud-lee.tistory.com/94
- 8: https://learn.microsoft.com/ko-kr/archive/msdn-magazine/2009/february/best-practice-an-introduction-to-domain-driven-design
- 9: https://www.borntodare.me/architecture/business_logic
- 10: https://mandykr.tistory.com/79
비즈니스 로직 테스트 위치 및 추가 테스트 케이스 검토 필요
현재 CartServiceTest.java 파일에서는 서비스 계층에서 모킹(mock)을 다수 사용하여 테스트하고 있는데, 이는 DDD 원칙에 따라 비즈니스 로직이 캡슐화되어 있는 도메인 레이어에서 테스트되어야 한다는 점과 상충됩니다. 웹 검색 결과에서도 비즈니스 로직 테스트는 도메인 엔티티나 도메인 서비스를 대상으로 단위 테스트를 수행하는 것이 최선임을 확인했습니다.
다음 사항들을 고려해 수정해 보세요:
- 비즈니스 로직 테스트 위치 변경:
- 서비스 계층(Application Layer)에서의 테스트 대신, 실제 비즈니스 규칙이 구현된 도메인 레이어(엔티티, 값 객체, 도메인 서비스)를 대상으로 단위 테스트를 작성하세요.
- 추가 테스트 케이스 제안:
- 최대 수량 제한을 초과하는 경우에 대한 테스트
- 이미 장바구니에 있는 상품과 동일한 상품을 다른 옵션으로 추가하는 경우에 대한 테스트
- 동일한 상품과 옵션을 추가할 때 수량이 병합되어 증가되는 로직에 대한 테스트
이와 같이 수정하면 테스트의 가치를 높이고, 도메인 로직의 변경 사항을 보다 명확하게 검증할 수 있을 것입니다.
| public void addItemToCart(String userId, ProductId productId, ProductOption option, int quantity) { | ||
| Cart cart = getCart(userId); | ||
| if (!productRepository.existsById(productId)) { | ||
| throw new ProductNotFoundException(productId); | ||
| } | ||
| cart.addProduct(productId, option, quantity); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
수량(quantity)에 대한 추가 검증이 필요합니다.
quantity가 0 이하일 경우 예외 처리가 이루어지지 않으므로, 도메인 상태가 잘못될 위험이 있습니다. quantity가 유효 범위(예: 1 이상)인지 확인하는 로직을 추가해 주세요.
public void addItemToCart(String userId, ProductId productId, ProductOption option, int quantity) {
Cart cart = getCart(userId);
if (!productRepository.existsById(productId)) {
throw new ProductNotFoundException(productId);
}
+ if (quantity <= 0) {
+ throw new IllegalArgumentException("수량은 0보다 커야 합니다.");
+ }
cart.addProduct(productId, option, quantity);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public void addItemToCart(String userId, ProductId productId, ProductOption option, int quantity) { | |
| Cart cart = getCart(userId); | |
| if (!productRepository.existsById(productId)) { | |
| throw new ProductNotFoundException(productId); | |
| } | |
| cart.addProduct(productId, option, quantity); | |
| } | |
| public void addItemToCart(String userId, ProductId productId, ProductOption option, int quantity) { | |
| Cart cart = getCart(userId); | |
| if (!productRepository.existsById(productId)) { | |
| throw new ProductNotFoundException(productId); | |
| } | |
| if (quantity <= 0) { | |
| throw new IllegalArgumentException("수량은 0보다 커야 합니다."); | |
| } | |
| cart.addProduct(productId, option, quantity); | |
| } |
| public void removeLineItem(String userId, LineItemId lineItemId) { | ||
| Cart cart = getCart(userId); | ||
| cart.removeLineItem(lineItemId); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
LineItem이 존재하지 않을 경우의 예외 처리 확인이 필요합니다.
removeLineItem 실행 시 해당 lineItemId가 장바구니에 없을 경우 예외 처리가 어떻게 이뤄지는지 확인해 주세요. 도메인 내부에서 예외를 발생시키지 않는다면, 적절한 예외를 던지는 로직을 추가하는 것을 고려해 보세요.
🏁 Script executed:
#!/bin/bash
# LineItemNotFoundException 같은 맞춤 예외가 정의되어 있는지 검색
rg "LineItemNotFoundException"Length of output: 30
위 스크립트의 실행 결과를 공유해 주시거나 수동으로 해당 메서드 내부에 예외 처리를 위한 코드(예: throw 문)가 포함되어 있는지 확인해 주세요.
🏁 Script executed:
#!/bin/bash
# Cart 클래스 내의 removeLineItem 메서드 구현부에서 예외 처리(throw 등) 여부를 확인하기 위한 검색
rg -A 15 "public void removeLineItem" -n .Length of output: 1883
예외 처리 추가 필요
현재 Cart.removeLineItem(LineItemId lineItemId) 메서드는 존재하지 않는 LineItem에 대해 예외 처리를 하지 않고, 단순히 removeIf로 처리하고 있습니다. 이로 인해, removeLineItem 실행 시 해당 LineItem이 존재하지 않더라도 명시적으로 예외를 던지지 않아 애플리케이션의 다른 로직에서 문제 상황 인지가 어려울 수 있습니다.
- 파일:
src/main/java/com/example/demo/model/Cart.javaremoveLineItem메서드가LineItem부재에 대해 예외를 발생시키는 로직이 없습니다.
- 파일:
src/main/java/com/example/demo/application/CartService.java- 서비스 계층에서
getCart호출 시CartNotFoundException은 발생하지만, LineItem 부재에 대한 별도의 예외 확인 로직은 없습니다.
- 서비스 계층에서
도메인 규칙에 따라 LineItem이 존재하지 않을 경우 예외를 발생시켜야 한다면, 해당 메서드에 적절한 예외 처리 로직(예: LineItemNotFoundException과 같은 맞춤 예외 추가)를 구현하는 것을 고려해 주세요.
|
|
||
| public void addItemToCart(ProductId productId, ProductOption option, int quantity) { | ||
| Cart cart = getCart(); | ||
| public void addItemToCart(String userId, ProductId productId, ProductOption option, int quantity) { |
There was a problem hiding this comment.
본격적으로 하실 거면 UserId VO를 잡아주세요.
| public void addItemToCart(ProductId productId, ProductOption option, int quantity) { | ||
| Cart cart = getCart(); | ||
| public void addItemToCart(String userId, ProductId productId, ProductOption option, int quantity) { | ||
| Cart cart = getCart(userId); |
There was a problem hiding this comment.
특별한 경우가 아니면 다른 곳과 마찬가지로 Repository에서 바로 얻을 겁니다. 별도의 메서드로 얻을 수 있는 혜택이 적고, 추상화 수준도 안 맞습니다.
| String authorization) { | ||
|
|
||
| Cart cart = cartService.getCart(); | ||
| String userId = extractUserIdFromAuthorization(authorization); |
There was a problem hiding this comment.
이 부분은 인증을 도입하지 않은 상태에서 어정쩡하게 가져가는 게 문제입니다.
- Bearer엔 Access Token이 들어가지 User ID가 들어가지 않습니다.
- 인증 처리가 먼저 이뤄지고, Controller에선 이렇게 복잡하게 처리하지 않습니다.
실제로 인증 처리를 하고 싶다면 인증을 넣어야 하고, 그게 아니면 설계를 고려해서 임의의 코드를 써줘야 합니다. 제가 기대했던 건 후자에 대해 다음 둘 중 하나를 하는 거였습니다.
- UI Layer에서 User ID를 하드코딩.
- Application Layer에서 Cart ID를 하드코딩.
애매하게 UI Layer에서 Cart ID를 하드코딩하면 나중에 진짜 구현이 들어왔을 때 다 갈아엎어야 해서 그 부분을 이야기하고 싶었어요.
마찬가지로, 지금 코드는 인증이 들어오면 싹 다 갈아엎어야 합니다. 앞으로 변화에 대해 최소한의 수정으로 대응하는 것, 유지보수성이 높아지는 것, 이런 게 관심사의 분리가 주는 혜택이죠.
There was a problem hiding this comment.
인증이 없는 상태에서는 그 상황을 고려해서 코드를 써야 하는데, 이 부분은 대개 Filter나 Intercept를 써서 처리하고, Contoroller 코드는 이런 모양이 됩니다. 꼭 이 모양이어야 하는 건 아니고, 여러 방법이 있지만 그냥 눈에 띄는 거 하나 아무거나 가져온 겁니다.
다시 강조하지만, 여기서 인증 처리가 필요한 건 아닙니다. 각 Layer의 책임 범위를 명확히 하고, 유지보수성이 높은 설계와 코드를 작성하는 게 필요하다는 겁니다. 하드코딩을 하더라도 최소한의 수정으로 처리하려면, 올바르게 배치 = 설계하는 게 필요합니다. 구현은 그 다음 문제죠.
구현부터 가면 계속 늪으로 빠질 수 있으니 고민하고 있는 부분을 먼저 공유해 주세요. 이미 많은 사람들이 만든 베스트 프랙티스가 많기 때문에 독자연구로 빠질 필요가 없습니다.
| ).toList() | ||
| ); | ||
|
|
||
| return new ResponseEntity<>(responseDto, HttpStatus.OK); |
There was a problem hiding this comment.
ResponseEntity는 영원히 안 써도 됩니다. 아주 옛날에는 특수한 상황에서 쓸 일이 있기도 했지만, 지금은 안 쓰는 걸 강력히 추천합니다.
| @GetMapping | ||
| public CartResponseDto getCart() { | ||
| public ResponseEntity<CartResponseDto> getCart( | ||
| @RequestHeader(name = "Authorization", required = false) |
There was a problem hiding this comment.
아주 특수한 경우가 아니면 Controller에서 ReqeustHeader를 쓸 일이 없습니다. 대개는 더 앞에서 Filter나 Intercept로 처리합니다. 인증/인가는 매우 보편적이라 보다 앞에서 공통으로 처리하는 게 필요합니다.
지금은 임시 구현을 너무 구체적으로 하려고 하는데, 이도 저도 아닌 힘 빼기가 될 위험이 있습니다. 설계에 더 집중해 주세요. 결론보다 과정이 더 중요합니다. 말 없이 코드만 계속 바꾸는 건 안 좋은 신호예요.
Summary by CodeRabbit
New Features
Tests