[#26] 장바구니 도메인 추가 #44
Conversation
jjeda
commented
Dec 28, 2019
jjeda
commented
- 장바구니를 위한 Redis config 파일 추가
- cart, carItem 도메인 추가
- CartRedisRepository 추가
- Cart 컨트롤러 및 서비스 추가
- 장바구니를 위한 Redis config 파일 추가 - cart, carItem 도메인 추가 - CartRedisRepository 추가 - Cart 컨트롤러 및 서비스 추가
- CRUD를 위한 CartService 클래스 완성 - CartService 단위테스트 및 Redis 연둥 후 통합테스트 완료
| return ResponseEntity.ok(cartService.getCart(String.valueOf(accountDto.getEmail()))); | ||
| } | ||
|
|
||
| @PutMapping("/add") |
There was a problem hiding this comment.
add, remove같은 의미들은 메소드에 담겨있으니 URL설계를 다시 해보시는것도 좋을 것 같습니다~
| private final CartRedisRepository cartRedisRepository; | ||
|
|
||
| public Cart initCart(String id) { | ||
| if (!cartRedisRepository.existsById(id)) { |
There was a problem hiding this comment.
이거는 동시성문제가 생길 수 있겠네요. !cartRedisRepository.existsById(id)가 true라서 장바구니가 존재하지 않는다고 판단했는데 그 사이에 다른 스레드에서 save를 할 가능성이 있습니다~ (물론 한 계정의 장바구니에 대해 중복요청이 올 가능성은 거의 없지만요)
| public class CartService { | ||
| private final CartRedisRepository cartRedisRepository; | ||
|
|
||
| public Cart initCart(String id) { |
- PUT mapping URI 설계 변경 - init 메서드와 addItem 메서드 통합 - 동시성 이슈 관련 수정
| return ResponseEntity.ok(cartService.addItem(String.valueOf(accountDto.getEmail()), cartItem)); | ||
| } | ||
| @PutMapping | ||
| public ResponseEntity modifyCart(@CurrentUser AccountDto accountDto, @RequestBody CartItem cartItem, |
There was a problem hiding this comment.
이런식으로 나누기보다는 /carts/items 이런식의 도메인구조면 더 역할나누기가 수월할 것 같습니다~
| @@ -0,0 +1,5 @@ | |||
| package me.jjeda.mall.cart.domain; | |||
|
|
|||
| public enum CartModifyType { | |||
There was a problem hiding this comment.
요청의 내용을 TYPE으로 분기하게 되면 API의 설계가 잘못됐을 가능성도 생각해볼 수 있습니다. 위의 리뷰를 참고하셔서 수정해보시면 좋을 것 같습니다~
| return getCart(id); | ||
| } | ||
|
|
||
| @Transactional(readOnly = true) |
There was a problem hiding this comment.
SQL이 1개만 나가게되는데 트랜잭션을 걸어준 이유가 있을까요?
There was a problem hiding this comment.
트랜잭션에 대한 이해가 부족했네요..ㅎㅎ
| public Cart removeItem(String id, CartItem cartItem) { | ||
| Cart cart = getCart(id); | ||
| final Cart cart = getCart(id); |
There was a problem hiding this comment.
final을 걸어주는건 괜찮지만 이 변수에만 걸어준 이유가 있을까요? 컨벤션은 일관적이면 좋을 것 같아서요~
There was a problem hiding this comment.
addItem 메서드 수정하다가 기계적으로 바꿨네요..
여기서는 동시성 문제가 없는 듯하여 일괄적인 컨벤션을 위해 삭제하겠습니다!
|
개인적으로 느끼기에는 현재 클래스관계에 너무 꽂히셔서 몰두하시는것으로 보이는데 아키텍처를 설계하면서 제일 주의할 점은 오버엔지니어링을 주의해야합니다. 지금 하고있는 작업이 정말 자신의 코드를 사용하는 개발자를 편하게 해줄 수 있을 것인지, 쉽게 해결할 수 있는 것을 어렵게 돌아가고있는게 아닌지를 고민하면서 하시면 좋을 것 같네요~ |
- 아이템 추가/삭제 URI 재설계 - 불필요한 코드 제거
- 동시성 문제 해결을 위한 트랜잭션 경계설정
| redisOperations.watch(key); | ||
| redisOperations.multi(); | ||
| redisOperations.opsForHash().putAll(key,map); | ||
| return redisOperations.exec(); |
There was a problem hiding this comment.
오호 WATCH 는 버전정보만 비교하는건가보네요~
자동으로 DISCARD 될 줄알았는데 다시 읽어보니 아닌 것같군요
| cart = Cart.of(id); | ||
| final String key = String.format("cart:%s", id); | ||
| // "_class", "id" 필드를 제외하고 "cartItemList[n]" 의 필드개수 6개로 나누어주면 상품개수 | ||
| int cartItemSize = (int)((redisTemplate.opsForHash().size(key) - 2) / 6); |
There was a problem hiding this comment.
2, 6 같은 수는 상수로 만들면 더 소스를 이해하기 좋을 것 같습니다
- WATCH -> DISCART 로직 추가
