Skip to content
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,18 @@ repositories {
}

dependencies {
implementation ("org.springframework.boot:spring-boot-starter-data-jpa")
implementation("org.springframework.boot:spring-boot-starter-web")
implementation("org.springframework.boot:spring-boot-starter-validation")

compileOnly ("org.projectlombok:lombok")
annotationProcessor ("org.projectlombok:lombok")
Copy link
Member

Choose a reason for hiding this comment

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

아마 아주 특별한 일이 일어나지 않는다면 Lombok을 쓸 일은 없을 겁니다. record가 도입된 후로는 영원히 봉인해도 괜찮을 정도라고 생각해요. 저는 최근에 이것의 존재를 아예 잊고 있었습니다.


developmentOnly("org.springframework.boot:spring-boot-devtools")

testImplementation("org.springframework.boot:spring-boot-starter-test")
testCompileOnly ("org.projectlombok:lombok")
testAnnotationProcessor ("org.projectlombok:lombok")
}

tasks.withType<Test> {
Expand Down
54 changes: 54 additions & 0 deletions src/main/java/com/example/demo/application/CartService.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
package com.example.demo.application;

import com.example.demo.model.Cart;
import com.example.demo.model.LineItemId;
import com.example.demo.model.Product;
import com.example.demo.model.ProductId;
import com.example.demo.model.ProductOption;
import com.example.demo.repository.CartRepository;
import com.example.demo.repository.ProductRepository;
import org.springframework.stereotype.Service;
import org.springframework.transaction.annotation.Transactional;

@Service
@Transactional
public class CartService {

private final CartRepository cartRepository;
private final ProductRepository productRepository;

public CartService(CartRepository cartRepository, ProductRepository productRepository) {
this.cartRepository = cartRepository;
this.productRepository = productRepository;
}

public void addItemToCart(Long cartId, ProductId productId, ProductOption option, int quantity) {
Cart cart = cartRepository.findById(cartId)
.orElseThrow(() -> new IllegalArgumentException("장바구니가 존재하지 않습니다."));

Choose a reason for hiding this comment

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

medium

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("장바구니가 존재하지 않습니다."));

Copy link
Member

Choose a reason for hiding this comment

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

여기도 Gemini의 지적이 올바릅니다.

Choose a reason for hiding this comment

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

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)
Copy link
Member

Choose a reason for hiding this comment

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

product가 쓰이지 않습니다. 존재만 확인하려고 했다면 exists를 써야 합니다.

.orElseThrow(() -> new IllegalArgumentException("상품이 존재하지 않습니다."));

Choose a reason for hiding this comment

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

medium

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("상품이 존재하지 않습니다."));

Copy link
Member

Choose a reason for hiding this comment

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

여기도 Gemini의 지적이 올바릅니다.

Choose a reason for hiding this comment

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

Glad to hear that suggestion was helpful!


cart.addProduct(productId, option, quantity);
}

public void removeItemFromCart(Long cartId, LineItemId lineItemId) {
Copy link
Member

Choose a reason for hiding this comment

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

Item은 뭔가요? LineItem이라면 그냥 명확히 removeLineItem이라고 하거나 그냥 remove라고 하거나 하는 게 더 낫습니다. 이 이름을 보면 “Item”이라는 제가 모르는 무언가가 또 있는 건지 어떤 건지 고민하게 됩니다. 유비쿼터스 언어가 깨진 모습인 거죠.

Cart cart = cartRepository.findById(cartId)
.orElseThrow(() -> new IllegalArgumentException("장바구니가 존재하지 않습니다."));

cart.removeLineItemById(lineItemId);
}

public void clearCart(Long cartId) {
Cart cart = cartRepository.findById(cartId)
.orElseThrow(() -> new IllegalArgumentException("장바구니가 존재하지 않습니다."));

cart.clearItems();
}

@Transactional(readOnly = true)
public Cart getCart(Long cartId) {
return cartRepository.findById(cartId)
.orElseThrow(() -> new IllegalArgumentException("장바구니가 존재하지 않습니다."));
}

}
42 changes: 38 additions & 4 deletions src/main/java/com/example/demo/controllers/CartController.java
Original file line number Diff line number Diff line change
@@ -1,14 +1,48 @@
package com.example.demo.controllers;

import com.example.demo.application.CartService;
import com.example.demo.controllers.dto.CartResponseDto;
import com.example.demo.controllers.dto.LineItemResponseDto;
import com.example.demo.model.Cart;
import lombok.RequiredArgsConstructor;
import org.springframework.http.HttpStatus;
import org.springframework.http.ResponseEntity;
import org.springframework.web.bind.annotation.DeleteMapping;
import org.springframework.web.bind.annotation.GetMapping;
import org.springframework.web.bind.annotation.PathVariable;
import org.springframework.web.bind.annotation.RequestMapping;
import org.springframework.web.bind.annotation.RestController;

@RestController
@RequestMapping("/cart")
@RequestMapping("/carts")
Copy link
Member

Choose a reason for hiding this comment

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

장바구니는 session과 마찬가지로 하나입니다. 아주 특별하게 여러 장바구니를 동시에 다루는 경우라면 복수형을 쓸 수도 있겠지만, 그런 일은 없습니다.

@RequiredArgsConstructor
public class CartController {
@GetMapping
String detail() {
return "";

private final CartService cartService;

@GetMapping("/{cartId}")
Copy link
Member

Choose a reason for hiding this comment

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

타인의 장바구니를 훔쳐보는 경우가 아니라면 이런 접근을 허용하면 안 됩니다.

public CartResponseDto getCart(@PathVariable Long cartId) {

Cart cart = cartService.getCart(cartId);

return new CartResponseDto(
cartId,
cart.getTotalQuantity(),
cart.getLineItems().stream()
.map(lineItem -> new LineItemResponseDto(
lineItem.getProductId().id(),
lineItem.getProductOption().color(),
lineItem.getProductOption().size(),
lineItem.getQuantity()
)
).toList()
);
}


@DeleteMapping("/{cartId}")
public ResponseEntity<Void> deleteCart(@PathVariable Long cartId) {
cartService.clearCart(cartId);
return new ResponseEntity<>(HttpStatus.NO_CONTENT);
}
}
33 changes: 30 additions & 3 deletions src/main/java/com/example/demo/controllers/LineItemController.java
Original file line number Diff line number Diff line change
@@ -1,17 +1,44 @@
package com.example.demo.controllers;

import com.example.demo.application.CartService;
import com.example.demo.controllers.dto.LineItemRequestDto;
import com.example.demo.model.LineItemId;
import com.example.demo.model.ProductId;
import com.example.demo.model.ProductOption;
import lombok.RequiredArgsConstructor;
import org.springframework.http.HttpStatus;
import org.springframework.web.bind.annotation.DeleteMapping;
import org.springframework.web.bind.annotation.PathVariable;
import org.springframework.web.bind.annotation.PostMapping;
import org.springframework.web.bind.annotation.RequestBody;
import org.springframework.web.bind.annotation.RequestMapping;
import org.springframework.web.bind.annotation.ResponseStatus;
import org.springframework.web.bind.annotation.RestController;

@RestController
@RequestMapping("/cart/line-items")
@RequestMapping("/carts/{cartId}/line-items")
Copy link
Member

Choose a reason for hiding this comment

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

기본 코드에서 제시한 틀에서 벗어나지 마세요. 이런 방식은 타인의 장바구니에 상품을 추가하는 일이 벌어질 위험이 있습니다.

@RequiredArgsConstructor
public class LineItemController {

private final CartService cartService;

@PostMapping
@ResponseStatus(HttpStatus.CREATED)
void create() {
//
public void create(@PathVariable Long cartId, @RequestBody LineItemRequestDto requestDto) {

cartService.addItemToCart(
cartId,
Copy link
Member

Choose a reason for hiding this comment

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

여기서 cartId를 넘겨줘야 하는 이유가 없습니다. 가장 큰 문제는 UI Layer에서는 Cart ID를 영원히 알 수 없다는 거죠. #9 (comment)

new ProductId(requestDto.getProductId()),
new ProductOption(requestDto.getColor(), requestDto.getSize()),
requestDto.getQuantity());

Choose a reason for hiding this comment

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

medium

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) {


}

@DeleteMapping("/{lineItemId}")
@ResponseStatus(HttpStatus.NO_CONTENT)
public void deleteLineItem(@PathVariable Long cartId, @PathVariable String lineItemId) {
cartService.removeItemFromCart(
cartId, new LineItemId(lineItemId));
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
package com.example.demo.controllers.dto;

import java.util.List;

public class CartResponseDto {
Copy link
Member

Choose a reason for hiding this comment

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

record를 쓸 수 없는 상황이 아니라면 DTO는 모두 record로 만드세요. record는 Kotlin이나 다른 언어에 비해 훨씬 훌륭한, 매우 강력한 무기 중 하나입니다. 저는 record가 Java의 미래라고 말하곤 해요.

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;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
package com.example.demo.controllers.dto;

public class LineItemRequestDto {
private String productId;
Copy link
Member

Choose a reason for hiding this comment

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

Request DTO엔 validation을 추가해 주세요. 애너테이션을 적당히 붙이면 유효성 검사가 너무 간단히 수행돼서 안 할 이유가 없습니다.

private String color;
private String size;
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;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
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;
}
}
94 changes: 94 additions & 0 deletions src/main/java/com/example/demo/model/Cart.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
package com.example.demo.model;

import jakarta.persistence.CascadeType;
import jakarta.persistence.Entity;
import jakarta.persistence.GeneratedValue;
import jakarta.persistence.GenerationType;
import jakarta.persistence.Id;
import jakarta.persistence.OneToMany;
import jakarta.persistence.Table;

import java.util.ArrayList;
import java.util.Collections;
import java.util.List;

@Entity
@Table(name="carts")
public class Cart {

@Id
@GeneratedValue(strategy = GenerationType.IDENTITY)
private Long id;

@OneToMany(mappedBy = "cart", cascade = CascadeType.ALL, orphanRemoval = true)
private List<LineItem> lineItems = new ArrayList<>();

private int totalQuantity;

public Cart() {
}

public Cart(List<LineItem> lineItems) {
this.lineItems = new ArrayList<>(lineItems);
lineItems.forEach(lineItem -> lineItem.setCart(this));
calculateTotalQuantity();
}

public List<LineItem> getLineItems() {
return Collections.unmodifiableList(lineItems);
}

public int getTotalQuantity() {
return totalQuantity;
}

public void addProduct(ProductId productId, ProductOption productOption, int quantity) {

LineItem lineItem = findLineItem(productId, productOption);

if (lineItem != null) {
lineItem.addQuantity(quantity, productOption);
Copy link
Member

Choose a reason for hiding this comment

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

addQuantity는 quantity만 넘어가야 합니다. 뭔가를 같이 넣는 순간 무언가 잘못 되고 있다는 걸 캐치할 수 있어야 합니다. 우리는 changeScore(score, name);이라고 쓰지 않는다는 걸 알고 있고, 여기서도 동일한 원칙을 적용해야 합니다.

calculateTotalQuantity();
checkValidQuantity();
Copy link
Member

Choose a reason for hiding this comment

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

아주 특별한 경우가 아니면 calculateTotalQuantity에서 checkValidQuantity까지 처리해야 합니다. 아마 모든 calculateTotalQuantity 메서드 사용하는 곳은 바로 뒤에 checkValidQuantity가 따라올 겁니다. 실수로 checkValidQuantity를 빼먹는 순간 불변식은 깨질 거고요. 열심히 Aggregate를 만들었어도 불변식이 깨지면 의미가 없습니다.

return;
}

lineItem = new LineItem(productId, productOption, quantity);
lineItems.add(lineItem);
lineItem.setCart(this);
Copy link
Member

Choose a reason for hiding this comment

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

4주차에 없던 코드가 6주차에 들어오면 안 됩니다. JPA를 사용하는 이유는 단순합니다. 4주차 코드 자체는 전혀 고치지 않고 애너테이션 몇 개만 붙였더니 DB에 영속화가 되는 거죠.

calculateTotalQuantity();
checkValidQuantity();
Copy link
Member

Choose a reason for hiding this comment

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

여기서 이렇게 둘을 한 세트로 쓰고 있다는 사실을 발견하는 순간, 둘을 합칠 계획을 세워야 합니다.

}

public void checkValidQuantity() {
if (totalQuantity > 20) {
throw new IllegalArgumentException("담을수 있는 수량을 초과했습니다.");

Choose a reason for hiding this comment

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

medium

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("담을수 있는 수량을 초과했습니다.");

Copy link
Member

Choose a reason for hiding this comment

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

Genimi의 지적이 올바릅니다. 4주차 때의 예제 문서를 보시면 아시겠지만, 이 예외가 도메인 모델링할 때 만들어져야 합니다.

}
}

private void calculateTotalQuantity() {
this.totalQuantity = lineItems.stream()
.mapToInt(LineItem::getQuantity)
.sum();
}


private LineItem findLineItem(ProductId productId, ProductOption productOption) {
return lineItems.stream()
.filter(lineItem ->
lineItem.isSameProduct(productId,productOption))
.findFirst()
.orElse(null);
}

public void clearItems() {
lineItems.clear();
calculateTotalQuantity();
}

public void removeLineItemById(LineItemId lineItemId) {
Copy link
Member

Choose a reason for hiding this comment

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

ById는 불필요합니다. 이 표현은 대단히 나쁜데, 메서드에 이런 걸 시시콜콜하게 써주지 않으면 자동으로 관련 코드를 생성할 수 없는 경우 = Spring Data JPA를 쓰면서 Repository의 구현을 내가 만들고 싶지 않은 상황을 제외하면 그 어느 곳에서도 이걸 쓰면 안 됩니다. 저는 AddIntegerWithInteger 같은 메서드 이름은 절대로 쓰고 싶지 않습니다. 그저, 기계에게 integer와 integer를 더한다는 걸 아주 명확히 알려줘야 하는 가장 바닥에서 아주 아주 예외적으로 쓰는 것 정도만 가까스로 용납해줄 수 있는 거죠. 이런 흉물스러운 이름은 Repository 밖으로 절대 못 나오게 하시고, 그 정도가 너무 심하면 반드시 JPQL로 봉인해야 합니다.

if (lineItemId == null) return;
Copy link
Member

Choose a reason for hiding this comment

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

여기서 null 검사를 하는 일은 매우 드믑니다. UI 레이어에서 쳐내는 게 쉽기도 하고, 이게 null이라고 무슨 문제가 생기는 것도 아니기도 하기 때문이죠.

lineItems.removeIf(lineItem ->lineItem.getId().equals(lineItemId));

Choose a reason for hiding this comment

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

medium

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();
        }

calculateTotalQuantity();
}
}
Loading