-
Notifications
You must be signed in to change notification settings - Fork 301
Step2 #721
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
base: youlalala
Are you sure you want to change the base?
Step2 #721
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.
안녕하세요 유라님 😄
미션 잘 진행해주셨네요! 몇가지 코멘트 남겨두었으니 확인부탁드려요 :)
궁금하거나 고민이 되는 부분이 있으시다면 언제든 pr 코멘트 또는 dm으로 요청 부탁드립니다.
감사합니다 🙇♂️
|
||
private final int width; | ||
private final int height; | ||
private final int sizeInBytes; |
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 final int sizeInBytes; | |
private final int sizeInBytes; |
👀
if (width * 2 != height * 3) { | ||
throw new InvalidCoverImageException("비율은 3:2여야 합니다."); | ||
} | ||
if (extension == null) { |
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.
null인 케이스도 있겠지만 확장자가 ImageExtension
에 포함되지 않는 경우도 예외처리되면 좋겠네요!
|
||
import java.util.List; | ||
|
||
public class Sessions { |
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 List<Session> sessions; | ||
|
||
public Sessions() { | ||
} |
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 List<Session> sessions; | |
public Sessions() { | |
} | |
private final List<Session> sessions; | |
public Sessions() { | |
sessions = new ArrayList<>(); | |
} |
이렇게 보완이 되어야할 것 같네요 😄
throw new CannotEnrollSessionException("수강 조건이 맞지 않습니다."); | ||
} | ||
|
||
currentEnrolledCount++; |
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.
수강 수를 넣을 수도 있지만 누가 들었는지 수강생을 저장해보는건 어떨까요? NsUser
를 활용해보면 좋을 것 같아요 :)
@@ -0,0 +1,5 @@ | |||
package nextstep.payments.domain; | |||
|
|||
public interface EnrollmentPolicy { |
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 final LocalDate endDate; | ||
private final CoverImage coverImage; | ||
private final SessionStatus status; | ||
private final EnrollmentPolicy enrollmentPolicy; |
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.
(반영하지 않으셔도 됩니다.) Session
이라는 큰 추상적 표현을 생각해보면 Session
을 인터페이스로 두고, "유료강의"와 "무료강의"라는 구현체를 두는 형태를 고민해볼 수 있을 것 같아요 😄
public boolean canEnroll(int currentEnrolledCount, Payment payment) { | ||
return payment.isSameAmount(0L); | ||
} |
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 PaidEnrollmentPolicy implements EnrollmentPolicy { | ||
|
||
private final int maxEnrolledCount; | ||
private final int price; |
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.
가격은 어떻게보면 Session
이 들고 있어야하는 정보가 아닐까요?
import static org.junit.jupiter.api.Assertions.assertFalse; | ||
import static org.junit.jupiter.api.Assertions.assertTrue; | ||
|
||
public class EnrollmentPolicyTest { |
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.
테스트코드를 작성할 때 EnrollmentPolicy
하위에 있는 구현체들에 대해 EnrollmentPolicyTest
로 몰아넣을수도 있지만, 테스트하는 대상은 각 구현체들로 구분지어져있기 때문에 개인적으로는 FreeEnrollmentPolicyTest
, PaidEnrollmentPolicyTest
와 같이 클래스별로 테스트 클래스를 만드는 것을 선호합니다 😄
안녕하세요!
각 클래스 설명입니다.
Course - 과정
Session - 강의
Sessions - 강의 일급 객체
SessionStatus - 강의 상태 : 준비중, 모집중, 종료
CoverImage - 강의 커버 이미지
ImageExtension - 이미지 확장자
EnrollmentPolicy - 강의 정책 인터페이스로 등록 가능한지 판단함
PaidEnrollmentPolicy - 유료 강의 정책
FreeEnrollmentPolicy - 무료 강의 정책
감사합니다:)