-
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 Part1 Weekly Mission 첫 번째 PR 제출합니다. #679
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.
이슬님 리뷰 확인 부탁 드려요! 전반적으로 깔끔하게 잘 작성해주셨네요 !
- 입력과 출력을 각각 클래스로 정의할 지, 아니면 하나의 클래스에 모두 작성할 지 고민이 많았습니다! 생각해보니 입력의 기능이 너무 적은 것 같아 Console 이라는 클래스에 입출력 모두 작성하였습니다! 이 부분에 대해 입력과 출력을 분리하는 것이 좋은 지 멘토님들의 의견을 듣고 싶습니다!!😊
- 음 요것도 어느정도 개인의 철학, 스타일에 가까운 영역이라고 생각되긴 합니다만..! 저도 이슬님처럼 Console class 에서 모두 관리했을 것 같아요! 이슬님 생각과 같은 생각이에요. 지금은 기능이 크지않으니 Console class에서 관리하고, 추후에 기능이 커졌을 때 쪼개도 문제없다고 생각합니다. 그럼에도 불구하고 객체의 역할과 책임을 더 명확하게 하고 싶다면 나누는것도 좋겠지요 ?
- 객체에 대한 책임이 적절하게 나누어 졌는지 궁금합니다!🤔
- 주로 예외 책임에 대해서 리뷰 남겨두었어요! 리뷰 확인해보시고 이슬님의 생각도 함께 반영해보시면 좋을 것 같습니다.
public int discount(int originalPrice) { | ||
return originalPrice - 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.
p1;
originalPrice - amount; 값이 음수가 나올 경우엔 어떻게 처리하나요 ?
FixedAmountVoucher 도메인 객체가 빈약한 것 같아요.
요 글 한번 읽어보고 이슬님 생각을 녹여 조금 더 풍성한 도메인으로 만들어보세요~ (깊게 이해하실 필요 x. 이런 개념이 있구나~ 정도로만 소화해보세요!)
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.
음수 예외 처리는 안되어있습니다.... 음수 처리 말고도 가능한 예외에 대해서 생각해보고 처리 하도록 하겠습니다!!
저도 DDD를 공부하면서 제가 작성한 도메인이 너무 빈약한 것 같다고 생각했습니다. 이 부분에 대해서 생각해서 더 풍성하게 만들어보겠습니다. 인사이트 감사합니다😊
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 FixedAmountVoucher implements Voucher { | ||
|
||
private final UUID id; |
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;
(단순 궁금증) voucher 식별값을 uuid로 설정한 이유는 무엇인가용 ?
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를 안쓰는 상황에서는 따로 id값을 1씩 증가시켜서 식별값을 설정하였습니다. 강의에서 리더님이 UUID를 사용하시는 것을 보며 제가 했던 방식보다 더 간결하다고 생각해서 UUID를 사용하였습니다!
public int discount(int originalPrice) { | ||
return originalPrice * (amount / 100); | ||
} |
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;
FixedAmountVoucher와 마찬가지로 PercentDiscountVoucher도 도메인 객체에서 가져가야할 책임을 조금 더 고려해보아도 좋을 것 같아요~
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.
바우처 예외처리 로직 추가 했습니다😎
import java.util.List; | ||
import java.util.stream.Collectors; | ||
|
||
public class VoucherDto { |
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.
p4;
Request와 Response를 VoucherDto class 안에 Nested class 로 나타내신것은
데이터 사용시 VoucherDto.Request, VoucherDto.Response 와 같이 명확하게 데이터 용도를 나타내고 싶으셔서 이렇게 하신걸까요 ? 그런것이라면 sealed interface 를 아래처럼 사용해서 데이터를 표현할 수도 있을 것 같아요!
public class VoucherDto { | |
public sealed interface VoucherDto permits VoucherDto.Request, VoucherDto.Response { | |
record Request( | |
String discountType, | |
int discountAmount | |
) implements VoucherDto { | |
@Builder | |
public Request { | |
} | |
} | |
record Response( | |
List<String> voucherName | |
) implements VoucherDto { | |
@Builder | |
public Response { | |
} | |
} | |
} |
} | ||
|
||
private boolean isRunning() { | ||
String command = Console.selectCommand(); |
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;
command를 enum으로 관리해보는건 어떨까요 ?
command를 관리하는 enum에서 잘못된 값이 들어왔을 경우에 대한 예외 책임도 가져갈 수 있을 것 같구요. 그렇게 되면 default 도 불필요해지면서 코드도 간결해지지 않을까요 ?
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.
enum으로 관리하는 것 너무 좋은 것 같습니다! command를 enum으로 리팩토링 해보겠습니다 :)
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.
command enum으로 변경 했습니다😎
return toDto(vouchers); | ||
} | ||
|
||
private static Voucher toEntity(String discountType, int discountAmount) { |
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;
discountType도 enum으로 관리해도 좋을 것 같아요~ 대응되지 않는 타입이 들어올 경우에 대한 예외 책임도 enum class에서 가져가도록 해볼 수 있겠죵?
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;
추가로 Voucher 인터페이스로 추상화를 시켜주었지만 VoucherService에서 각 Voucher들을 생성해주고 있는데요.
좀 더 유연한 구조를 갖기 위해 다른 객체에게 책임을 주는 건 어떨까요?
VoucherService는 어떤 Voucher인지 신경쓰지 않고 할인 등 기능을 사용할 수 있도록이요!
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; discountType도 enum으로 관리해도 좋을 것 같아요~ 대응되지 않는 타입이 들어올 경우에 대한 예외 책임도 enum class에서 가져가도록 해볼 수 있겠죵?
넵!! 위에서 말씀하신 것처럼 enum으로 관리할 수 있는 것들은 enum으로 빼는 것이 확실히 좋은 것 같습니다!! 예외 책임도 모두 enum에서 관리될 수 있도록 하겠습니다😄
p1; 추가로 Voucher 인터페이스로 추상화를 시켜주었지만 VoucherService에서 각 Voucher들을 생성해주고 있는데요. 좀 더 유연한 구조를 갖기 위해 다른 객체에게 책임을 주는 건 어떨까요? VoucherService는 어떤 Voucher인지 신경쓰지 않고 할인 등 기능을 사용할 수 있도록이요!
저도 VoucherService
에서 Voucher를 생성하는 부분에서 고민이 많았는데, 서브멘토님 말씀대로 다른 곳에서 처리하는 게 더 좋은 것 같습니다!! 어떤 객체가 책임을 가져야 할 지 고민해보고 VoucherService
에서 분리해보겠습니다!😊
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을 enum으로 변경 했습니다😎
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.
서비스에 대한 책임 분배 (DTO 변환, Voucher 생성)
바우처 생성을 담당하는 클래스를 만들어 서비스에서 분리했습니다!
return toDto(vouchers); | ||
} | ||
|
||
private static Voucher toEntity(String discountType, int discountAmount) { |
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;
discountAmount에 연관된 코드를 전체적으로 살펴봤는데, -가 들어오는 경우에 대한 예외처리 책임을 가지고 있는 클래스가 전혀 없는 것 같아요~ 지금 어플리케이션을 구동하는데 있어 문제될 부분은 없지만, 예외 처리를 어디서 담당하면 좋을지 고민해보면 좋을 것 같아요!
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.
discountAmount에 숫자 아닌 값 입력 예외 처리 했습니다!
들어오는 형식(int, String)에 대한 검증은 View의 책임이라고 생각해서 콘솔에 구현하였습니다!
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.
이슬님 미션하시느라 수고하셨습니다.
전체적인 패키지나 코드가 깔끔해서 좋았어요!
4계층(application, domain 등등)으로 만드신 점이 인상깊네요!
몇가지 코멘트 남겼는데 확인해주세요!
입력과 출력을 각각 클래스로 정의할 지, 아니면 하나의 클래스에 모두 작성할 지 고민이 많았습니다! 생각해보니 입력의 기능이 너무 적은 것 같아 Console 이라는 클래스에 입출력 모두 작성하였습니다! 이 부분에 대해 입력과 출력을 분리하는 것이 좋은 지 멘토님들의 의견을 듣고 싶습니다!!😊
요곤 구현하기 나름일 듯 한데요. 굳이 나눠서 이점을 볼 게 없다면 이슬님이 하신대로 하나의 객체에서 구현해줄 것 같아요!
객체에 대한 책임이 적절하게 나누어 졌는지 궁금합니다!🤔
현재 프로젝트가 작지만 잘 나누신 것 같아요. 대신 도메인을 좀 더 풍성하게 만들어보시면 좋을 것 같습니다!
@@ -0,0 +1,21 @@ | |||
# 기능 명세서 |
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 FixedAmountVoucher(UUID id, int amount) { | ||
this.id = id; | ||
this.amount = 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.
p1;
만약 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.
확실히 도메인이 빈약한 것 같습니다!! 도메인에 관련된 검증 로직 등을 추가해서 더 풍성하게 만들어보겠습니다 :)
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.
바우처 예외처리 로직 추가 했습니다😎
|
||
import java.util.UUID; | ||
|
||
public interface Voucher { |
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;
Just Ask : Voucher를 인터페이스로 만드신 이유가 있을까요?
추상클래스도 사용해볼 수 있을 것 같긴 한데 이슬님의 생각이 궁금합니다.
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.
저도 인터페이스랑 추상클래스 중에 어떤 것을 사용해야할까 고민이 많았습니다... 자식클래스가 부모클래스를 구체화한다는 것에는 추상클래스를 사용하는 것이 맞다고 생각이 듭니다.
저는 추상클래스를 사용한다면 인터페이스와 달리 하위클래스에게 상속할 수 있는 구체적으로 표현된 필드나 메소드가 있어야 하지 않을까라는 생각을 했습니다. 그래서 바우처마다 공통적인 필드나 메소드를 추상클래스에 구현하였는데, 추상클래스를 사용하는 과정에서 제대로 사용을 못해 빨간줄이 계속 떠서 결국 인터페이스로 바꾸어서 마저 구현을 하였습니다!
@Override | ||
public String toString() { | ||
return voucherName.stream() | ||
.map(voucher -> voucher + System.lineSeparator()) | ||
.collect(Collectors.joining()); | ||
} | ||
} | ||
|
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;
DTO는 어떻게 보면 데이터를 전달해주기 위한 객체인데요. 만약 콘솔 출력 형식이 변경된다면
해당 메서드도 변경이 되어야 할 것 같아요!
DTO에서 출력 형식을 결정하는 것이 아닌 View쪽에서 알아서 출력하도록 책임을 줘보는 건 어떨까요? 좀 더 유연해질 수 있을 것 같아요.
(사실 DTO라 크게 문제 없어보이긴 합니다. 개인적인 의견이예요!)
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.
아하! 콘솔 출력 형식이 변경될 경우 메서드도 변경된다는 생각을 못했습니다. View 쪽에서 형식처리해서 출력하는 것이 더 좋을 것 같습니다!! 좋은 인사이트 감사합니다😄
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.
View에게 출력 형식에 대한 책임 위임 했습니다!
|
||
import java.util.List; | ||
|
||
public interface VoucherRepository { |
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; Just Ask
VoucherRepository
를 Domain 패키지에 넣으신 이유가 있을까요?
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.
VoucherRepository
는 외부에 의존하지 않기 때문에 infrastructure 패키지가 아닌 Domian 패키지에 넣었습니다!!
private VoucherDto.Response toDto(List<Voucher> vouchers) { | ||
List<String> vourcherList = vouchers.stream() | ||
.map(voucher -> voucher.getClass().getSimpleName()) | ||
.collect(Collectors.toList()); | ||
return new VoucherDto.Response(vourcherList); | ||
} | ||
} |
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; Just Ask
DTO를 만드신 이슬님의 생각이 궁금합니다~
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로 레포지토리를 변경할 것을 대비하여 entity가 직접 다른 계층에 전달되는 것을 방지하고, 필요한 정보만 뽑아 view에 전달하기 위함입니다!
return toDto(vouchers); | ||
} | ||
|
||
private static Voucher toEntity(String discountType, int discountAmount) { |
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;
추가로 Voucher 인터페이스로 추상화를 시켜주었지만 VoucherService에서 각 Voucher들을 생성해주고 있는데요.
좀 더 유연한 구조를 갖기 위해 다른 객체에게 책임을 주는 건 어떨까요?
VoucherService는 어떤 Voucher인지 신경쓰지 않고 할인 등 기능을 사용할 수 있도록이요!
@Override | ||
public List<Voucher> findAll() { | ||
return new ArrayList<>(storage.values()); | ||
} | ||
} |
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.
방어적 복사 사용하신 점 좋네요! 💯
@Repository | ||
public class MemoryVoucherRepository implements VoucherRepository { | ||
|
||
private static final Map<UUID, Voucher> storage = new ConcurrentHashMap<>(); |
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; Just Ask
ConcurrentHashMap
을 사용하신 이유가 있을까요?
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.
HashMap
보다Thead-safe 하다고 들어서 사용하였습니다!
public class FixedAmountVoucher implements Voucher { | ||
|
||
private final UUID id; | ||
private final int 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.
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.
변경 범위를 최소화 하기 위한 방법으로 BigDecimal
이 떠오릅니다! 이 밖에 다른 방법도 있을까요,,??
@yuminhwan 아하 혹시 String 으로 Domain까지 받아와서 Domain에서 변경사항에 맞게 형변환 해주라는 말씀일까요?? 이 부분에 대해서 계속 고민 중인데 감을 잘 못잡겠습니다😂
📌 과제 설명
'기본' 먼저 구현하였습니다! 추후에 '심화' 구현해서 마저 올리겠습니다😊
(기본) 바우처 관리 애플리케이션
logback
설정을해서 에러는 파일로 기록된다.jar
파일을 생성한다.(심화) 파일을 통한 데이터관리 기능과 고객 블랙 리스트 명단 관리기능
👩💻 요구 사항과 구현 내용
기능 명세서는 docs. 에 작성하였습니다!
feat: 바우처 도메인 생성
feat: 바우처 생성 기능 추가
feat: 만들어진 Voucher 리스트 조회 기능 추가
feat: View 기능 추가
feat: 예외 처리
feat: 로그 출력
클래스 다이어그램
✅ PR 포인트 & 궁금한 점