-
Notifications
You must be signed in to change notification settings - Fork 0
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
도서 대출 연장 기능 추가 및 테스트 #14
base: main
Are you sure you want to change the base?
Conversation
* @return 연장 여부 | ||
*/ | ||
public boolean isExtended() { | ||
return extendCount > 0; |
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.
2회 연장 가능해지면 연장여부와 더이상 연장하지 못하는지 여부가 논리적으로 다른 뜻이겠군요
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.
연장을 한 번만 허용할 수 있게 하려다보니 유연하지 못한 구조가 돼버렸네요...ㅎㅎ 다시 작성했습니다!
|
||
LocalDate from = this.getPeriod().getFrom(); | ||
LocalDate extendedTo = this.getPeriod().getTo().plusWeeks(1); | ||
Period extendedPeriod = new Period(from, extendedTo); |
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.
Period 의 from, to 를 자꾸 공개 하는군요. 기간을 늘리는 연산 자체는 Period 의 책임에 가보는건 어때요?
period.pushBack(7 /* days */);
int halfPeriod = this.getMember().getLevel().getBorrowPeriod() * 7 / 2; | ||
LocalDate targetDate = this.getPeriod().getFrom().plusDays(halfPeriod); | ||
|
||
return now.isEqual(targetDate) || now.isAfter(targetDate); |
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.
특정일이 기간안에 포함중인지, 또는 해당 기간이 특정일로부터 시작전인지 역시 Period 에 물어보는 형식으로 가봅시다
return !period.isAfter(now);
* @return 현재 도서에 대한 예약이 있는지 여부 | ||
*/ | ||
public boolean haveReservation() { | ||
return this.getInventory() != null && this.getInventory().getReservationCount() > 0; |
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.
예약이 있는지를 판단하는 규칙은 Inventory 한테 맡겨볼까요?
return findInventory() // 얜 뭘까요? 만들어보세요
.map(Inventory::isReservable)
.orElse(false) // null 일 땐 예약 못함
- 회원의 등급에 따라 연장 가능 횟수를 할당하도록 변경 -> 연장여부가 아닌 회원의 등급 별 연장 가능 횟수에 따라 연장 가능여부를 응답
객체지향적이지 못한 코드 작성한 것을 반성합니다...😭 |
@@ -35,4 +35,8 @@ public static Period of(@NonNull LocalDate from, Level level) { | |||
return new Period(from, level); | |||
} | |||
|
|||
public Period getExtended(Level level) { |
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.
Period 는 약간 범용적인 개념이고 Level 은 좀 더 도메인 개념이네요
지금처럼 이 둘이 직접 협업하는 것도 좋고, Period 가 더 일반적인 쓰임일 수 있게 배려해둘 수도 있습니다.
어떻게 생각하세요? ㅎㅎ
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.
Period
가 범용적인 개념이지만 대출(연장) 도메인 내에서 협력하는 경우가 더 많을 것 같다고 생각했습니다. 그래서 getExtended(int addWeek)
같이 일반적으로 쓰일 수 있는 매개변수보다 회원 등급을 매개변수로 받아 회원 등급에 따라 대출이나 연장 기간을 생성하는 로직이 더 적합하다고 판단했습니다.
그리고 회원 등급 별 대출, 대출 연장에 대한 비즈니스 규칙(대출 기간, 연장 기간 등)을 Level
에서 한 번에 관리하는 것이 더 응집력 있다고 생각했습니다.
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.
좋습니다! ㄱㄱ
@@ -35,4 +35,8 @@ public static Period of(@NonNull LocalDate from, Level level) { | |||
return new Period(from, level); | |||
} | |||
|
|||
public Period getExtended(Level level) { | |||
return new Period(from, this.to.plusWeeks(level.getExtendPeriod())); |
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.
불변 전략 좋네요 ㅎㅎ 한 가지 메서드 명에 get- 이 붙는게 나쁘진 않지만 조금 편한 말투로 가능할까요?
참고가 필요하다면 Java 내에 시간을 다루거나 아님 불변성을 유지하는 표준 객체들의 메서드를 한번 참고해보세요
- getExtended() -> createExtended() 변경
public Period getExtended(Level level) { | ||
return new Period(from, this.to.plusWeeks(level.getExtendPeriod())); | ||
public Period createExtended(Level level) { | ||
return new Period(from, to.plusWeeks(level.getExtendPeriod())); | ||
} |
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.
- createExtended
- addExtension
- withExtension
이렇게 고민했었는데 매개변수를 받아서 연장된 Period 인스턴스를 새로 만든다는 의미에 createExtended
가 가장 적절하다고 생각해서 작성했습니다.
토모님은 이 메서드명을 어떻게 지으실지 궁금합니다 ㅎㅎ
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.
저라면.. 그냥 extend?
Period 의 역할이니 한번 읽어보면 period create extended 랑 period extend 어느쪽 문장이 편한가요?
혹은 도메인 한정적이어도 된다면 renew 도 좋겠네요 ㅎㅎ
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.
Period 의 역할이니 한번 읽어보면 period create extended 랑 period extend 어느쪽 문장이 편한가요?
오.. 이렇게 읽어보니 period extend가 더 편합니다.
저는 기간 연장된 객체를 반환한다 라고 생각해서 extend()
는 command 성격이라 배제를 했었습니다.. ㅎㅎ
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.
그래서 get
을 붙이셨군요 ㅎㅎ
예를들어, LocalDate#plusWeeks(int)
메서드는 어떻게 생각하시나요?
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.
불변객체일 경우는 다르군요..! LocalDate.plusWeeks()
같은 경우는 LocalDate
가 불변객체이기 때문에 상태 값을 변경하려면 새로운 인스턴스를 만드는 방법밖에 없어서 저렇게 사용한 것 같습니다. 그렇다면 Period
도 같은 경우겠네요!
LocalDate now = LocalDate.now(); | ||
|
||
return targetDate.isEqual(now) || targetDate.isBefore(now); | ||
return now.isAfter(targetDate); |
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.
now 를 객체 내부에서 직접 얻으면 객체가 결정적이지 못하게 됩니다.
무슨 뜻일까요? 해결할 수 있나요?
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.
항상 동일한 입력 값에 동일한 출력이 나온다는 뜻입니다. (순수함수)
위 코드로는 now
가 시간이 지나면서 다른 결과를 응답해줄 수 있기 때문에 결정적이지 못한 메서드입니다.
메서드에서 LocalDate
를 매개변수로 받는다면, 메서드를 호출할 때 파라미터로 오늘 날짜를 넣어야 하는데 다른 날짜를 넣어서 유효성 검사를 통과하여 검증이 제대로 이뤄지지도 않을 수도 있을 것 같아서 now
를 내부에서 직접 얻었습니다.
해결방법으로
- 호출하는 곳에서 유효성 검사 진행
- 제 3의 객체에서 유효성 검사 진행
- 객체 내부에 매개변수가 오늘 날짜인지 판별하는 private 메서드를 선언하여 유효성 검사 진행
2번이나 3번이 괜찮아보이는데,
만약 매개변수 now
가 오늘 날짜가 아닌 다른 날짜로 넘어왔지만 연장이 가능한 날짜라면 위 방법으로 유효성 검사를 진행했을 때 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.
ㅋㅋㅋㅋ 그럼 상상력이 등장할 때죠.
자 정리해봅시다. 먼저 연장가능하다 는 것은 Period 에 어울리는 역할일까요?
기간아 연장가능하니? 인지 대출기간아 연장가능하니? 인지 살펴봅시다.
기간이 곧 대출기간인가? 맞다면 이름을 좀 더 구체적으로 지을 필요가 있겠습니다.
따로 대출기간을 다루는 책임은 다른 객체에 있다면 기간은 좀 더 범용적으로 동작하는 방식을 고려해야 좋습니다.
연장 말고 기간에 입장에서 무엇을 검사하고 싶었나 하는거죠.
더.. 얘기하면 뭔가 상상이 줄어들 수 있으니 한번 고민해보세요
- Book -> Inventory로 책임 변경
.orElse(false); | ||
} | ||
|
||
public Optional<Inventory> findInventory() { |
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.
👍
- BorrowPeriod (대출 기간) 생성 - Period (기간) 범용적인 도메인으로 변경
} | ||
|
||
public boolean isExtendable() { | ||
public LocalDate getMidDate() { |
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.
이 메서드가 범용적인 Period에 어울리는지는 저도 조금 애매하다고 생각하지만 BorrowPeriod
와의 협력에서 중간 날짜를 가지는 메세지가 필요해서 작성했습니다.
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.
앗 커밋대상이 잘못 됐습니다
- BorrowPeriod (대출 기간) 생성 - Period (기간) 범용적인 도메인으로 변경
import java.time.LocalDate; | ||
|
||
@EqualsAndHashCode | ||
public class BorrowPeriod { |
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.
이 선택은 어떤 결과가 될까요 ㅋㅋㅋ
도서 대출 연장 조건에 따른 연장 로직 작성
도서 대출 연장 테스트 작성