-
Notifications
You must be signed in to change notification settings - Fork 54
Conversation
- DAO는 redis를 사용하도록 구성 - 고객이 메뉴를 장바구니에 넣을 시 해당 현제 메뉴가 해당 매장의 메뉴인지 확인한 후 입력
- redis에 '@transactional'을 적용할 수 있도록 트랜잭션 매니저 빈 등록 - Key를 enum으로 관리 - 리뷰 반영
- 총 가격 계산 - 동일 메뉴를 넣으려할때 버그 수정 - 장바구니 최대 개수 10개로 수정
- 주문 신청 기본 정보 조회 기능 추가 - 임시로 CartService 추가
- 가격 계산로직을 캐싱에 의존하도록 변경 - 테스트코드 삭제(로직 완성시 다시 작성할 예정)
- 장바구니 가격 계산 로직 분리 - redis 트랜잭션에서 discard를 명시적으로 선언 - redis '@transactional' 삭제 - 사용하지 않는 세션용 변수 제거 - ItemDTO를 불변객체로 지정
public Long totalPrice(List<OrderItemDTO> items) { | ||
long totalPrice = 0L; | ||
for (OrderItemDTO item : items) { | ||
MenuDTO menuInfo = menuService.getMenuInfoWithOptios(item.getMenuId()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optios
오타로 보입니다~
Long orderItemId = item.getId(); | ||
log.debug("order item id : {}", orderItemId); | ||
|
||
orderMapper.addOrderItemOptions(item.getOptions(), orderItemId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
반복문을 돌면서 쿼리를 날리는 것은 성능에 아주 좋지 않습니다. 최대한 벌크 insert를 해주시면 좋을 것 같네요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
벌크 insert를 여러 테이블에 동시에 가능한가요? 최대한 한번의 쿼리로 제작하고 싶었는데, 해당 로직의 경우는 1번의 주문에 N개의 아이템이 있고, 1개의 아이템에는 또 M개의 옵션이 있습니다.
그렇기 때문에 1 : N : M관계라서 1 : N은 벌크 Insert를 진행할 수 있었지만 N : M일때는 서로 insert하는 테이블이 달라 어려움이 생겼습니다
이 경우에는 어떤 식으로 처리해야 할까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
그래서 해당 로직을 1:N, 1:M으로 두개로 나눠서 반복문으로 진행하였습니다
@@ -73,4 +73,16 @@ | |||
AND menu_group_id = #{menuGroupId} | |||
</select> | |||
|
|||
<select id="findMenuWithOptionsById" resultMap="menuResult"> | |||
SELECT * |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
*
은 사용하지 않는게 좋습니다. 이 로직에서는 어떤 값을 필요로하는지 명확하게 해주시면 좋을 것 같아요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resultMap을 사용해서 필요한 컬럼을 명시적으로 선언하고 이 값들을 전부 가져오는 것도 안될까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
기왕이면 쿼리방식으로 데이터를 가져올때는 쿼리 하나하나의 목적이 명확한 것이 좋습니다. 쿼리 하나하나 작성할때마다 튜닝해야할게 달라지기도 하고요. 컬럼이 아주 많을 경우엔 대역폭의 낭비도 발생할 것 같네요
주문 아이디로 주문 상세조회 추가 자신의 주문인지 검증하는 로직 만들어야함 페이징도 만들어야함
Builder 패턴을 사용하여 만들도록 변경 사용하지 않는 서비스 제거
- 주문 전 유효성 검사 추가 - 유효성 검사를 Controller에서 담당하도록 코드 이동
src/main/java/com/delfood/error/exception/order/TotalPriceMismatchException.java
Outdated
Show resolved
Hide resolved
MenuDTO menuInfo = menuService.getMenuInfoWithOptios(item.getMenuId()); | ||
long optionsPrice = 0L; | ||
MenuDTO menuInfo = menuService.getMenuInfo(item.getMenuId()); | ||
for (OrderItemOptionDTO orderItemOption : item.getOptions()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이런 반복문도 stream 연산으로 바꿔보시면 좋을 것 같습니다~ 스트림도 적응되면 엄청 편합니다
@@ -47,7 +47,7 @@ public long getItemsBill(HttpSession session, @RequestBody List<OrderItemDTO> it | |||
* @param orderId 주문번호 | |||
* @return | |||
*/ | |||
@GetMapping("{orderId}") | |||
@GetMapping("orderBill/{orderId}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
orders/{orderId}/bills
가 낫지 않을까요?
<select id="findPositionByShopId" resultType="com.delfood.dto.address.Position"> | ||
SELECT building_center_point_x_coordinate xPos, building_center_point_y_coordinate yPos | ||
FROM ADDRESS | ||
WHERE building_management_number = (SELECT SHOP.address_code |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기서 서브쿼리가 좋을지, 조인이 좋을지 생각해보시는 것도 좋을 것 같습니다~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
조인하기에는 주소 DB 양이 너무 많아서.. 서브쿼리가 더 나을거라고 생각했습니다. innoDB 조인은 nested loop join이라 주소DB는 조인하기에는 부담이 큰거같아요.
회원 주문 조회시 페이징 처리 추가 주문번호로 주문 조회시 유효성 검사 추가
@Transactional | ||
public void insertPayment(PaymentDTO paymentInfo) { | ||
long result = paymentMapper.insertPayment(paymentInfo); | ||
if (result != 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 조건문도 걷어내면 좋을 것 같네요 (다른 곳도 전부입니다)
return orderService.getOrder(orderId); | ||
OrderDTO orderInfo = orderService.getOrder(orderId); | ||
String memberId = SessionUtil.getLoginMemberId(session); | ||
if (memberId.equals(orderInfo.getMemberId()) == false) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
세션에서 가져온건데 유저랑 다를일은 없을 것 같네요. 이거는 처리해주지 않아도 되는 예외같습니다
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 회원이 주문한 것이 아닌 다른 회원의 주문일 때 문제가 될 수 있어서 처리하였습니다.
일단 전체적인 주문 흐름을 간단하게 만들어보고 점차 살을 붙여나갈 예정입니다.