이제 와서 고쳐보는 2024 내 코드

2025년도 두 달이나 지나 벌써 3월입니다.

 

작년 10월 항해 플러스 백엔드 교육을 통해 많은 인사이트를 얻었고 사용하지 않으면 까먹을게 뻔하니 틈틈이 사내 SI용 템플릿을 개발하고 있었습니다. 당연히 학습과는 달리 실무에 적용하며 많은 어려움을 겪고 있어 다시 한 번 이전에 작성한 코드를 보며 또 다른 인사이트를 얻어보려 합니다.

과제 선정

항해 당시 선택할 수 있었던 과제는 콘서트 예매 사이트와 E-Commerce 사이트가 있었고 비록 팀 미션은 아니지만 상호 피드백을 위해 학습 매니저님과 팀원들과의 논의 끝에 선정했던 과제는 E-Commerce로 진행하게 되었습니다. Spring Boot로 API 서버만 구현했었고 주요 기능으로는 잔액 충전 및 조회 / 상품 조회 / 장바구니 / 주문 및 결제가 있습니다.

아키텍처 구조

3주차에 다양한 아키텍처 패턴에 대한 발제가 있었는데 그 중 예제 코드와 함께 많은 설명을 해주신 clean + layerd 아키텍처를 선택했고 다음과 같이 도메인에 집중되는 패키지 구조로 시작했습니다.

(interfaces => application => domain <= infrastructure)

• application
• domain
• infra
• interfaces

SI 기업에 재직중이기에 전통적인 Controller, Service, Repository로만 구성된 레이어드 아키텍처만 사용했었고 API 설계에 대해 심도 있는 고민은 해본 적이 없던 상황이라 기본 구조 잡고 데이터 흐름을 이해하는데 1주일을 모두 보냈던 것 같습니다.

 

이제 주요 기능들에 대해 하나하나 뜯어보며 과거의 저를 부사수라 생각하고 최대한 물고 뜯고 맛보겠습니다.

잔액 충전 및 조회

먼저 User쪽 기능인 잔액 충전과 조회 기능입니다.

 

UserController

@RestController
@RequiredArgsConstructor
@RequestMapping("/users")
public class UserController {

    private final UserFacade userFacade;

    @GetMapping("/{userId}/balance")
    public CommonResponse<UserDto.BalanceResponse> getUserBalance(
			    @PathVariable Long userId
    ) {
        var userInfo = userFacade.getBalance(userId);
        var response = UserDto.BalanceResponse.from(userInfo);

        return CommonResponse.success(response);
    }

    @PostMapping("/{userId}/balance")
    public CommonResponse<UserDto.BalanceResponse> chargeUserBalance(
            @RequestBody UserDto.ChargeRequest request,
            @PathVariable Long userId
    ) {
        var userCommand = UserDtoMapper.toCommand(request, userId);
        var userInfo = userFacade.chargeBalance(userCommand);
        var response = UserDto.BalanceResponse.from(userInfo);

        return CommonResponse.success(response);
    }

}

 

회원 가입이나 로그인과 같은 인증 관련 로직은 없기 때문에 순수하게 잔액 충전 및 조회만 존재합니다. 단순 API단이고 클라이언트에서 검증은 하겠지만 파라미터에 대한 유효성 검증이 없는 부분이 조금 아쉬워 보입니다.

@PostMapping("/{userId}/balance")
public CommonResponse<UserDto.BalanceResponse> chargeUserBalance(
        @Valid @RequestBody UserDto.ChargeRequest request,
        @PathVariable @Positive Long userId
) {
    ...
}

UserServiceImpl

Facade의 경우 단순 호출만 하고 있기 때문에 바로 Service입니다.

@Service
@RequiredArgsConstructor
public class UserServiceImpl implements UserService {

	private final UserReader userReader;

	@Override
	@Transactional
	public void useAmount(Long userId, Long amount) {
		User user = userReader.getUser(userId);
		user.useAmount(amount);
	}

	@Override
	@Transactional
	public UserInfo.Main chargeBalance(UserCommand.ChargeRequest command) {
		User user = userReader.getUser(command.getUserId());
		user.chargeBalance(command.getAmount());

		return UserInfo.Main.from(user);
	}

	@Override
	@Transactional(readOnly = true)
	public UserInfo.Main getUser(Long userId) {
		User user = userReader.getUser(userId);

		return UserInfo.Main.from(user);
	}
}

// UserReaderImpl
@Override
public User getUser(Long userId) {
	return userRepository.findById(userId)
		.orElseThrow(() -> new EntityNotFoundException("유효하지 않는 UserId입니다."));
}

 

Service 코드에서 딱히 개선점은 보이지 않지만 굳이 요즘 생각으로 본다면 비즈니스 로직은 Entity에 위임하더라도 명색이 Service인데 getUser시에 User가 아닌 Optional 객체로 받아 Service에서 처리했으면 domain 계층에서 모든 로직 처리가 이루어지는 깔끔한 그림이 좋지 않았을까? 라는 생각이 드네요.

// UserReaderImpl
@Override
public Optional<User> getUser(Long userId) {
	return userRepository.findById(userId);
}

// UserServiceImpl
@Override
@Transactional
public void useAmount(Long userId, Long amount) {
	User user = userReader.getUser(userId)
			.orElseThrow(() -> new IllegalArgumentException("사용자를 찾을 수 없습니다."));
	...
}

상품 조회

상품 목록과 최근 가장 많이 팔린 상위 다섯 개의 상품 목록을 반환하는 기능입니다.

 

ProductController

@RestController
@RequiredArgsConstructor
@RequestMapping("/products")
public class ProductController {

    private final ProductFacade productFacade;

    @GetMapping
    public CommonResponse<ProductDto.ProductResponse> getProducts() {
        var productInfos = productFacade.getProducts();
        var response = ProductDto.ProductResponse.of(productInfos);

        return CommonResponse.success(response);
    }

    @GetMapping("/top-selling")
    public CommonResponse<ProductDto.ProductResponse> getTopSelling() {
        var productInfos = productFacade.getTopSelling();
        var response = ProductDto.ProductResponse.of(productInfos);

        return CommonResponse.success(response);
    }

}

 

파라미터로 받는 부분도 없어 유효성 검사는 필요 없어 보이지만 상품 목록 조회의 경우 많은 데이터를 반환할 게 뻔하기에 페이징을 사용하지 않은 부분이 아쉽습니다.

@GetMapping
public CommonResponse<Page<ProductDto.ProductResponse>> getProducts(
        @PageableDefault(sort = "updatedAt") Pageable pageable
) {
    var productPage = productFacade.getProducts(pageable);
    ...
}

ProductFacade

@Service
@RequiredArgsConstructor
public class ProductFacade {

	private final ProductService productService;
	private final OrderService orderService;

	@Transactional(readOnly = true)
	public List<ProductInfo.Main> getProducts() {
		return  productService.getProducts();
	}

	@Transactional(readOnly = true)
	public List<ProductInfo.Main> getTopSelling() {
		var productIds = orderService.getTopSelling();

		return productService.getProductsByIds(productIds);
	}

}

 

흐름을 제어하는 Facade 이기에 단순한 생각으로 트랜잭션을 설정한 것 같은데 위 코드 보다는 서비스에서 조금 더 세밀하게 트랜잭션을 제어했으면 어땠을까 하는 아쉬움이 있습니다.

 

ProductService

@Service
@RequiredArgsConstructor
public class ProductServiceImpl implements ProductService{

	private final ProductReader productReader;
	private final ProductStore productStore;
	private final ProductDeleter productDeleter;

	@Override
	@Cacheable(value = "productList", cacheManager = "redisCacheManager")
	@Transactional(readOnly = true)
	public List<ProductInfo.Main> getProducts() {
		var products = productReader.getProducts();

		return products.stream()
			.map(ProductInfo.Main::from)
			.toList();
	}

	@Override
	@Transactional(readOnly = true)
	public List<ProductInfo.Main> getProductsByIds(List<Long> productIds) {
		var products = productReader.getProductsByIds(productIds);

		return products.stream()
			.map(ProductInfo.Main::from)
			.toList();
	}

	@Override
	@Cacheable(value = "product", key = "#productId", cacheManager = "redisCacheManager")
	@Transactional(readOnly = true)
	public ProductInfo.Main getProduct(Long productId) {
		var product = productReader.getProduct(productId);

		return ProductInfo.Main.from(product);
	}

	@Override
	@Transactional(readOnly = true)
	public void checkStock(Long productId, Long quantity) {
		var product = productReader.getProduct(productId);
		product.checkStock(quantity);
	}

	@Override
	@DistributedLock(key = "#productId")
	public void reduceStock(Long productId, Long quantity) {
		var product = productReader.getProduct(productId);
		product.reduceStock(quantity);
	}

	@CacheEvict(value = {"product", "productList"}, allEntries = true)
	public void updateProduct(Product product) {
		productStore.store(product);
	}

	@CacheEvict(value = {"product", "productList"}, allEntries = true)
	public void deleteProduct(Long productId) {
		productDeleter.delete(productId);
	}

}

 

updateProduct나 deleteProduct시 해당 productId에 해당하는 캐시만 비우면 될 것 같은데 그 부분은 개선이 되면 좋을 것 같습니다. 사실 캐싱과 분산락은 시간이 지났어도 실무에서 적용 할 내용이 아예 없기 때문에 학습이 부족한 터라 더 많은 회고가 어려운게 조금 아쉽습니다…

@CacheEvict(value = "product", key = "#product.id")
@CacheEvict(value = "productList", allEntries = true)
public void updateProduct(Product product) {
    ...
}

@CacheEvict(value = "product", key = "#productId")
@CacheEvict(value = "productList", allEntries = true)
public void deleteProduct(Long productId) {
    ...
}

장바구니

결제를 원하는 상품들을 모아 이후 주문 및 결제에 사용할 장바구니 기능입니다.

 

CartController

@RestController
@RequiredArgsConstructor
@RequestMapping("/carts")
public class CartController {

	private final CartFacade cartFacade;

	@GetMapping("/{cartId}")
	public CommonResponse<CartDto.CartResponse> getCart(@PathVariable Long cartId) {
		var cartInfo = cartFacade.getCart(cartId);
		var response = CartDto.CartResponse.from(cartInfo);

		return CommonResponse.success(response);
	}

	@PostMapping("/{cartId}")
	public CommonResponse<String> addToCart(
		@PathVariable Long cartId,
		@RequestBody CartDto.AddToCartRequest request
	) {
		var cartCommand = CartDtoMapper.toCommand(request, cartId);
		cartFacade.addToCart(cartCommand);

		return CommonResponse.success("OK");
	}

}

 

이전 지적 사항과 마찬가지로 그 당시에 모든 Controller에 유효성 검사가 이루어지지 않은게 아쉽습니다.

CartFacade

@Service
@RequiredArgsConstructor
public class CartFacade {

	private final CartService cartService;
	private final ProductService productService;

	@Transactional
	public void addToCart(CartCommand.AddToCartRequest command) {
		var product = productService.getProduct(command.getProductId());
		productService.checkStock(product.getId(), command.getQuantity());

		cartService.addToCart(command);
	}

	@Transactional(readOnly = true)
	public CartInfo.Main getCart(Long cartId) {
		return cartService.getCart(cartId);
	}

}

 

command 객체에 상품 ID를 담고 있음에도 굳이 쓰지도 않을 product를 조회해 checkStock을 하는 부분이 이해가 잘 안되기 때문에 없어도 되는 라인 같습니다. 또한 Product와 마찬가지로 Facade단에서 트랜잭션을 제어하고 있습니다.

@Transactional
public void addToCart(CartCommand.AddToCartRequest command) {
    productService.checkStock(product, command.getQuantity());

    cartService.addToCart(command);
}

 

CartService

@Service
@RequiredArgsConstructor
public class CartServiceImpl implements CartService {

	private final CartReader cartReader;
	private final CartItemReader cartItemReader;
	private final CartDeleter cartDeleter;
	private final CartItemStore cartItemStore;

	@Override
	public void clearCart(Long cartId) {
		cartDeleter.delete(cartId);
	}

	@Override
	public void addToCart(CartCommand.AddToCartRequest request) {
		var existCartItem = cartItemReader.getCartItem(request.getCartId(), request.getProductId());
		if (existCartItem.isPresent()) {
			var cartItem = existCartItem.get();
			cartItem.updateQuantity(cartItem.getQuantity() + request.getQuantity());
			cartItemStore.store(cartItem);
			return;
		}
		var cartItem = CartItem.toEntity(request);
		cartItemStore.store(cartItem);
	}

	@Override
	public CartInfo.Main getCart(Long cartId) {
		var cart = cartReader.getCart(cartId);
		var cartItems = cartItemReader.getCartItems(cartId)
			.stream()
			.map(CartItemInfo.Main::from)
			.toList();

		return CartInfo.Main.of(cart, cartItems);
	}
}

 

Facade에서는 AddToCartRequest를 command라는 변수명으로 사용해놓고 갑자기 service에서는 request로 사용하는 부분이 혼선이 생길 수 있을 것 같아 보이고 cartItem.updateQuantity 부분은 product에서의 처리 처럼 엔티티 내부에서 처리하는게 조금 더 깔끔해 보입니다.

@Override
	public void addToCart(CartCommand.AddToCartRequest command) { // 이름 통일
		...
		if (existCartItem.isPresent()) {
			var cartItem = existCartItem.get();
			cartItem.increaseQuantity(command.getQuantity()); // 엔티티 내부로 캡슐화
			...
		}
		...
	}

 

주문 및 결제

이제 잔액, 상품, 장바구니를 모두 핸들링해서 가장 어려웠던 주문 및 결제입니다.

컨트롤러의 경우 주문 생성 뿐이고 똑같이 유효성 검증 부분이 마음에 안드니 넘어가겠습니다.

OrderFacade

@Service
@RequiredArgsConstructor
public class OrderFacade {

	private final OrderService orderService;
	private final ProductService productService;
	private final CartService cartService;
	private final UserService userService;

	public void createOrder(OrderCommand.CreateOrderRequest command) {
		var cart = cartService.getCart(command.getCartId());
		var cartItems = cart.getCartItems();
		cartItems.forEach(cartItem -> productService.checkStock(cartItem.getProductId(), cartItem.getQuantity()));

		var productIds = cartItems.stream()
			.map(CartItemInfo.Main::getProductId)
			.toList();
		var products = productService.getProductsByIds(productIds);
		var totalAmount = cartItems.stream()
			.mapToLong(cartItem -> {
				var price = products.stream()
					.filter(p -> p.getId().equals(cartItem.getProductId()))
					.map(ProductInfo.Main::getPrice)
					.findFirst()
					.orElseThrow(() -> new EntityNotFoundException("Product not found for id: " + cartItem.getProductId()));
				return price * cartItem.getQuantity();
			})
			.sum();

		var user = userService.getUser(cart.getUserId());
		userService.useAmount(user.getId(), totalAmount);

		orderService.createOrder(user.getId(), totalAmount);
		cartItems.forEach(cartItem -> {
			var price = products.stream()
				.filter(p -> p.getId().equals(cartItem.getProductId()))
				.map(ProductInfo.Main::getPrice)
				.findFirst()
				.orElseThrow(() -> new EntityNotFoundException("Product not found for id: " + cartItem.getProductId()));

			orderService.createOrderItem(user.getId(), cartItem.getProductId(), cartItem.getQuantity(), price);
		});

		cart.getCartItems().forEach(cartItem -> {
			productService.reduceStock(cartItem.getProductId(), cartItem.getQuantity());
		});

		cartService.clearCart(cart.getId());
		orderService.completeOrder(user.getId());
	}

}

 

흐름 제어만 하는 Facade 내부에 너무 많은 로직이 들어가 있어 사실 정신도 없고 벌써 손대기도 싫을 정도로 혐오스럽습니다. 상황과 선택에 따라 다르겠지만 일단 가격 때문에 매번 prducts를 순회하며 id 일치 여부와 가격을 뽑아 내는 부분을 줄이고 주문 생성과 재고 차감을 같이 처리하면 조금은 깔끔해질 것 같습니다.

public void createOrder(OrderCommand.CreateOrderRequest command) {
    ...
    var productIds = cartItems.stream().map(CartItemInfo.Main::getProductId).toList();
    var products = productService.getProductsByIds(productIds);
    // 미리 상품별 가격정보 map 생성
    var priceMap = products.stream()
        .collect(Collectors.toMap(ProductInfo.Main::getId, ProductInfo.Main::getPrice));

    // 총 금액 계산 
    // 기존에 만들어둔 map을 통해 불필요 stream 제거
    var totalAmount = cartItems.stream()
        .mapToLong(cartItem -> {
            var price = Optional.ofNullable(priceMap.get(cartItem.getProductId()))
                .orElseThrow(() -> new EntityNotFoundException("Product not found for id: " + cartItem.getProductId()));
            return price * cartItem.getQuantity();
        }).sum();
    ...
    // 주문 생성 및 주문 항목 처리
    // 기존에 만들어둔 map을 통해 불필요 stream 제거
    // 주문 생성과 재고 차감을 묶어 불필요 stream 제거
    orderService.createOrder(user.getId(), totalAmount);
    cartItems.forEach(cartItem -> {
        var price = priceMap.get(cartItem.getProductId());
        orderService.createOrderItem(user.getId(), cartItem.getProductId(), cartItem.getQuantity(), price);
        productService.reduceStock(cartItem.getProductId(), cartItem.getQuantity());
    });
    ...
}

 

지금보니 stream 단순화 외에도 총 금액 계산과 같은 로직은 Facade에서 처리하는게 그리 바람직해 보이지 않네요.

OrderService

@Service
@RequiredArgsConstructor
public class OrderServiceImpl implements OrderService {

	private final OrderStore orderStore;
	private final OrderReader orderReader;
	private final OrderItemStore orderItemStore;
	private final OrderItemReader orderItemReader;
	private final ApplicationEventPublisher eventPublisher;

	@Override
	@Transactional(readOnly = true)
	public void createOrder(Long userId, Long totalAmount) {
		var order = Order.toEntity(userId, totalAmount);

		orderStore.store(order);
	}

	@Override
	@Transactional(readOnly = true)
	public void createOrderItem(Long orderId, Long productId, Long amount, Long price) {
		var orderItem = OrderItem.toEntity(orderId, productId, amount, price);

		orderItemStore.store(orderItem);
	}

	@Override
	@Cacheable(value = "topSellingProducts", cacheManager = "redisCacheManager")
	@Transactional(readOnly = true)
	public List<Long> getTopSelling() {
		return orderItemReader.getTopSelling();
	}

	@Override
	@Transactional
	public void completeOrder(Long id) {
		var order = orderReader.getOrder(id);

		order.complete();

		eventPublisher.publishEvent(OrderCompletedEvent.of(order.getUserId(), order.getId(), order.getTotalAmount()));
	}
}

 

어떤 생각으로 create 메서드에 readOnly를 다 때려박아 놨는지 참 인상 깊습니다… 또한 주문 완료 후 이벤트 발행시 같은 트랜잭션에 묶이기 보다는 별도의 publish 메서드를 만들어 분리해주는게 이후 확장성에 유리할 것 같습니다.

 

지난 코드를 돌아보며

많은 시간이 지나지도 않았고 실무에서 배운 점들을 많이 써보진 못했음에도 처음 해보는 셀프 코드 리뷰를 통해 생각보다 많은 부분이 보여 상당히 놀랬습니다. 개발을 처음 시작하던 때에 시간이 지나 과거의 내 코드를 보고 부끄럽거나 개선 점이 보이면 조금이라도 성장했다는 증거라는 말을 장난으로 강사님께 들었던 적이 있었는데 막 체감이 되지는 않지만 그래도 조금은 의미 있는 시간이었습니다.

앞으로도 기존 코드를 회고하며 성장하는 시간을 자주 갖기 위해서는 작년 항해 코드 외에도 추가 사이드 프로젝트를 통해 회고할 수 있는 코드의 양을 늘려 조금 더 고민하고 리팩토링하는 과정을 통해 꾸준히 발전하는 개발자가 되어야 할 것 같습니다.