-
Notifications
You must be signed in to change notification settings - Fork 54
Conversation
- DAO는 redis를 사용하도록 구성 - 고객이 메뉴를 장바구니에 넣을 시 해당 현제 메뉴가 해당 매장의 메뉴인지 확인한 후 입력
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.
수고하셨습니다. 리뷰남깁니다
private static String generateKey(String id, String key) { | ||
return id + ":" + key; | ||
} |
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.
장바구니 말고도 redis 저장소를 이용할 때 정형화된 키 제작 방식을 사용하기 위해 만들어두었습니다.
public boolean deleteByMemberIdAndIndex(String memberId, long index) { | ||
return redisTemplate.delete(RedisKeyFactory.generateCartKey(memberId)); | ||
} |
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.
index를 파라미터로 받는데 index를 사용하는 부분은 없네요.
private CartDao cartDao; | ||
|
||
@Autowired | ||
private ObjectMapper objectMapper; |
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.
여기선 ObjectMapper를 사용하지 않는 것 같습니다~
package com.delfood.utils; | ||
|
||
public class RedisKeyFactory { | ||
private static String KEY_CART = "cart"; |
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.
여기도 사용되지 않는 키로 보입니다
} | ||
|
||
public static String generateCartKey(String memberId) { | ||
return generateKey(memberId, "cart"); |
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.
여기서 사용하려고 상수를 정의했는데 그냥 값을 사용한 것으로 보이네요~
public class CartDao { | ||
|
||
@Autowired | ||
private RedisTemplate<String, Object> redisTemplate; |
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.
이 클래스에선 RedisTemplate
의 내부 구현을 알아보시는게 좋을 것 같네요. 각 오퍼레이션마다 값이 어떻게 직렬화되고, 역직렬화되는지, 그리고 어떤 오퍼레이션이 어떤 특징을 가지고있는지, 어떻게 사용해야 성능에 좋은지 공부해보시면 좋을 것 같습니다. 그러면 전체적인 구현이 더 간결해지고 성능도 개선할 수 있을 것 같네요
- redis에 '@transactional'을 적용할 수 있도록 트랜잭션 매니저 빈 등록 - Key를 enum으로 관리 - 리뷰 반영
- 총 가격 계산 - 동일 메뉴를 넣으려할때 버그 수정 - 장바구니 최대 개수 10개로 수정
- 가격 계산로직을 캐싱에 의존하도록 변경 - 테스트코드 삭제(로직 완성시 다시 작성할 예정)
- 장바구니 가격 계산 로직 분리 - redis 트랜잭션에서 discard를 명시적으로 선언 - redis '@transactional' 삭제 - 사용하지 않는 세션용 변수 제거 - ItemDTO를 불변객체로 지정
@@ -20,9 +20,9 @@ | |||
@Autowired | |||
private ObjectMapper objectMapper; | |||
|
|||
@Value("${spring.redis.cartExpireSecond}") | |||
@Value("${redis.expire.second.cart}") |
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.
property는 큰 개념에서 작은개념으로 가는 것이 좋습니다. 그리고 장바구니 저장소를 레디스가 아니라 다른 저장소를 사용할 수도 있으니 확장성 또한 고려해서 만들어보는게 좋습니다.
|
||
redisTemplate.exec(); | ||
return result; |
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.
이 값은 사용하지 않으면 지워주는 것이 좋을 것 같습니다.
* @param item 가격을 계산할 아이템 | ||
* @return | ||
*/ | ||
public long menuPrice(ItemDTO item) { |
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.
내부의 상태값을 사용하는게 아니므로 static으로 선언해줘도 될 것 같네요.
@@ -34,7 +34,7 @@ | |||
@Value("${spring.redis.password}") | |||
private String redisPwd; | |||
|
|||
@Value("${redis.expire.second.default}") | |||
@Value("${default.expire.second}") |
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.
이렇게 전역적인 만료시간 기본값을 이렇게 주입해줄 필요는 없을 것 같네요. 다른 곳에 쓰이는 값이라면 더 적절한 이름이 있을 것 같습니다.
항상 프로퍼티가 많이 늘어날 것을 생각하면서 프로퍼티도 적절한 계층 및 값 설계가 필요합니다~
[#50] mock 결제 서비스 제작
장바구니 기능 구현