-
Notifications
You must be signed in to change notification settings - Fork 171
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[4기 - 박이슬] Springboot Part2 Weekly Mission 첫 번째 PR 제출합니다. #742
Conversation
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.
이슬님 part2 미션하시느라 수고하셨습니다!
저번 PR에 테스트만 추가된 듯해서 크게 코멘트를 달게 없었네요! 후에 JDBC도 추가되면 재리뷰 주세요!
- 리뷰하다가 push되어서 코멘트가 아마 이상하게 달릴 것 같은데 내용위주로 봐주세요!
테스트코드를 제대로 작성하였는지 궁금합니다! 특히 VoucherService 테스트 같은 경우 BDD 단위 테스트 방식으로 작성하였는데 저런 방식으로 작성하는 것이 맞는지 궁금합니다.
말씀 주신 BDD방식이 계층적으로 테스트 코드 만드는 걸 말씀주시는 것 같은데요.
저도 잘은 모르지만 given - when - then을 메서드 단위에서 분리하는 것이 아니라 class 단위로 나뉘는 것으로 알고있어서 when도 class로 만들어줘야 할 것 같아요!
자세한 건 아래 자료보면 이해가 되실 것 같습니다.
참고 자료 : https://johngrib.github.io/wiki/junit5-nested/
https://helloworld.kurly.com/blog/try-bdd/
VoucherService의 createVoucher 메서드에서 VoucherFactory 를 사용하여 바우처를 생성합니다. 이때 바우처는 랜덤 id 값이 할당되기 때문에 생성된 바우처를 알 수 없어 테스트코드를 작성할 수 없었습니다. 이런 경우 제가 createVoucher를 잘못 설계한 것일까요? 테스트 코드 작성을 위해 해당 메소드를 어떤 식으로 고쳐야 좋은지, 아니면 해당 메소드는 테스트 코드를 작성하지 않아도 되는 것인지 멘토님들에게 여쭤보고 싶습니다!
요곤 게더에서 말씀드려서 패스하겠습니다~
현재 저는 '정액 할인 바우처', '정률 할인 바우처' 두 개로 나뉘어져 있습니다. 이 상태로 DB 테이블을 만든다면 두 개의 테이블이 생성되야 합니다. 하지만 제 생각에는 두 개의 테이블이 아닌 하나의 바우처 테이블에 모두 저장되어야 한다고 생각합니다. 예를 들어 학생 테이블의 경우, 1반 학생 테이블, 2반 학생 테이블 이런 식으로 나누어지지 않고 학생 테이블 안에서 반이라는 속성으로 구분됩니다. 이와 같이 바우처도 1개의 테이블 속에서 할인 유형이라는 속성으로 구분되어야 된다고 생각하는데, 이런 경우 바우처 설계를 다시 해야할까요??
요곳도 바뀐점 보니깐 추상화가 잘 된 것 같아요! 저도 배워갑니다~~
int discountedAmount = calculateDiscountedAmount(originalPrice); | ||
return originalPrice - discountedAmount; | ||
} | ||
|
||
private int calculateDiscountedAmount(int originalPrice) { | ||
BigDecimal percent = BigDecimal.valueOf(amount).divide(BigDecimal.valueOf(MAX_PERCENT)); | ||
BigDecimal discountedAmount = percent.multiply(BigDecimal.valueOf(originalPrice)); | ||
return discountedAmount.intValue(); |
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.
p1;
저번 PR 코멘트에서 말씀드렸던 부분이 BigDecimal에 대해서 말씀드린 건 아니지만 잘 사용하신 것 같아요!
이슬님이 소수점에 계산에 BigDecimal을 사용하는 이유가 궁금합니다!
추가로 저번 피드백을 드린 점은 int보단 좀 더 넓은? 자료형인 double을 사용해도 좋을 것 같아서 남겼습니다! 아니면 해당 amount를 원시값으로 두는 것이 아닌 포장해서 써도 좋을 것 같네요!
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.
BigDecimal
를 사용한 이유는 정확한 나눗셈 연산을 하기 위해서입니다! 1 - (amount / 100)
을 할 경우 예상했던 결과와 다른 결과가 나왔습니다. int는 물론, double을 사용할 때도 마찬가지였습니다. BigDecimal
을 사용했을 땐 원하던 결과를 얻을 수 있었습니다!
원시값을 포장하면 타입에 구애받지 않게 되는군요! 좋은 방법 같습니다👍 감사합니다!!
|
||
class MemoryVoucherRepositoryTest { | ||
|
||
MemoryVoucherRepository memoryVoucherRepository = new MemoryVoucherRepository(); |
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.
p1;
해당 테스트들을 함께 돌렸을 때 저장 -> 조회 테스트가 돌아가면 성공할까요~?
테스트 실행 순서에 영향이 안가도록 바꿔보면 좋을 것 같아요!
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.
087dc0d <- 리뷰 반영 했습니다!
Voucher result = memoryVoucherRepository.save(voucher); | ||
|
||
// then | ||
assertThat(result.getId()).isEqualTo(voucher.getId()); |
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.
p3;
실제로 저장이 되었는지 검증하는 것이 추가되면 좋을 것 같아요!
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.
087dc0d <- 리뷰 반영 했습니다!
class FixedAmountVoucherTest { | ||
|
||
@Nested | ||
@DisplayName("할인 금액을 검증한다.") |
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.
p5;
개인적으로 할인 금액을 검증하기 보단 정액 할인 쿠폰을 생성한다.가 좀더 맞는 표현일 것 같아요~
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.
29d1e45 <- 반영했습니다👍
public enum DiscountType { | ||
|
||
FIX("fix"), | ||
PERCENT("percent"); | ||
FIX("fix", (amount, discountType) -> new Voucher(new FixedAmountDiscountPolicy(amount), discountType)), | ||
PERCENT("percent", (amount, discountType) -> new Voucher(new PercentDiscountPolicy(amount), discountType)); | ||
|
||
private final String name; | ||
private final BiFunction<Integer, DiscountType, Voucher> function; |
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.
p3;
Enum에다 생성하는 로직까지 넣어주셨군요 👍
넣어줌으로써 상태와 행위가 한 곳으로 모이게 되었네요!
대신 생성하는 function 필드 이름을 좀더 의미있게 바꾸시면 더 좋을 것 같아요.
생성하는 느낌이 들도록..?
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.
3775367 <- 필드명이 조금 길지만... 명확하게 나타내도록 바꾸었습니다!
public class Voucher { | ||
|
||
UUID getId(); | ||
private final UUID id; | ||
private final DiscountPolicy discountPolicy; | ||
private final DiscountType discountType; | ||
|
||
int discount(int originalPrice); | ||
public Voucher(DiscountPolicy discountPolicy, DiscountType discountType) { | ||
this.id = UUID.randomUUID(); | ||
this.discountPolicy = discountPolicy; | ||
this.discountType = discountType; | ||
} |
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.
p2;
오.. 할인정책을 추상화시킨 점 매우 좋네요 💯 배우고 갑니다 ㅎ
개인적인 생각으론 DiscountType
에 따라 DiscountPolicy
이 결정되기 때문에 DiscountPolicy
자체가 DiscountType
을 나타내줄 수 있다고 생각해서 Voucher
가 굳이 DiscountType
을 필드로 가질 필요는 없어보여요.
해당 Voucher가 어떤 타입인지를 나타내주고 싶다면 DiscountType
에게 물어서 얻어오면 생성하는 부분도 더 깔끔해질 듯해요!
요부분은 팀원들에게 공유해도 많은 도움이 될 것 같네요! 🚀
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.
DiscountType
필드를 제거하고 어떻게 타입을 알려줘야 하나 고민이 많았는데, 게더에서 멘토님 설명 듣고 바로 해결됐습니다 👍
✅ 피드백 반영사항
🔥 추가 구현 내용 (feat 내용만)
✅ PR 포인트 & 궁금한 점
|
📌 과제 설명
👩💻 요구 사항과 구현 내용
test: MemoryVoucherRepository 테스트 추가
test: DiscountType 테스트 추가
test: 할인 유형에 맞는 바우처 생성 테스트 추가
test: 정액 할인 바우처 테스트 추가
test: 정률 할인 바우처 테스트 추가
test: VoucherService 바우처 조회 테스트 추가
- 바우처를 모두 조회한다.
✅ 피드백 반영사항
refactor: discountType을 enum으로 변경
feat: 바우처 예외처리 로직 추가
refactor: List대신 복수형으로 네이밍 변경
refactor: View에게 출력 형식에 대한 책임 위임
refactor: 서비스에 대한 책임 분배 (DTO 변환, Voucher 생성)
refactor: command enum으로 변경
fix: discountAmount에 숫자 아닌 값 입력 예외 처리
✅ PR 포인트 & 궁금한 점
VoucherService
테스트 같은 경우 BDD 단위 테스트 방식으로 작성하였는데 저런 방식으로 작성하는 것이 맞는지 궁금합니다.VoucherService
의createVoucher
메서드에서VoucherFactory
를 사용하여 바우처를 생성합니다. 이때 바우처는 랜덤 id 값이 할당되기 때문에 생성된 바우처를 알 수 없어 테스트코드를 작성할 수 없었습니다. 이런 경우 제가createVoucher
를 잘못 설계한 것일까요? 테스트 코드 작성을 위해 해당 메소드를 어떤 식으로 고쳐야 좋은지, 아니면 해당 메소드는 테스트 코드를 작성하지 않아도 되는 것인지 멘토님들에게 여쭤보고 싶습니다!추가로 민환 멘토님께 1차 pr 코드리뷰에 대해 여쭤보고 싶은 게 있습니다!!
변경 범위를 최소화 하기 위한 방법으로 amount를
int
가 아닌String
으로 받다가 Domain에서 변경사항에 맞게 형변환하라는 말씀일까요?? 이 부분에 대해서 어떻게 처리를 해야할 지 감이 잘 안잡힙니다😭 약간의 힌트를 주시면 감사하겠습니다!!!