Skip to content
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

feature(domain): create domain(Session, Student, Students) #282

Open
wants to merge 6 commits into
base: bsumgh
Choose a base branch
from

Conversation

bsumgh
Copy link

@bsumgh bsumgh commented Dec 1, 2023

step2 PR 요청합니다.

Copy link

@csh0034 csh0034 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2단계 구현하시느라 수고많으셨습니다 😃
현재 기능구현이 한 커밋에 모두 되어있는데
기능별로 나눠서 커밋을 해보시면 좋을것 같아요.
추가로 몇가지 코멘트 남겨두었는데
확인후에 다시 리뷰요청 부탁드립니다!

src/main/java/nextstep/qna/domain/step2/Course.java Outdated Show resolved Hide resolved
src/main/java/nextstep/qna/domain/step2/Course.java Outdated Show resolved Hide resolved
src/main/java/nextstep/qna/domain/step2/Session.java Outdated Show resolved Hide resolved
src/main/java/nextstep/qna/domain/step2/SessionDate.java Outdated Show resolved Hide resolved
src/main/java/nextstep/qna/domain/step2/Session.java Outdated Show resolved Hide resolved
src/main/java/nextstep/qna/domain/step2/Session.java Outdated Show resolved Hide resolved
src/main/java/nextstep/qna/domain/step2/Session.java Outdated Show resolved Hide resolved
src/main/java/nextstep/qna/domain/step2/Payment.java Outdated Show resolved Hide resolved
src/main/java/nextstep/qna/domain/step2/Session.java Outdated Show resolved Hide resolved
Copy link

@csh0034 csh0034 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

코멘트 반영 잘해주셨네요 😄
몇가지 개선해보면 좋을것 같아 추가로 코멘트 남겨두었습니다.
확인후에 리뷰요청 부탁드려요!

import java.sql.Timestamp;

public class SessionDate {
private Timestamp startDate;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Date 하위타입인 Timestamp 보다 java1.8 에 추가된 LocalDateTime 을
사용해보면 좋을것 같아요!

}

public NsUser add(NsUser nsUser) {
students.stream()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

contains 메서드를 활용해볼수 있지 않을까요?
추가로 NsUser 에 equals hashCode 가 구현되어있지 않은데
정상동작하는걸까요? 🤔

@@ -0,0 +1,49 @@
package nextstep.session.domain;

public class CoverImage {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

별도 테이블로 구성되도록 해주셨네요.
강의는 강의 커버 이미지가 있어야하므로 하나의 테이블에서 VO 를 선언해보는것도
좋을것 같아요.

크기(용량), 타입, 가로세로 를 가지는도록 하고 각각 원시값 wrapping 을 해보면 좋을것 같습니다.


private void checkImage () {
String extension = name.split("\\.")[1];
if(!("gif".equals(extension) || "png".equals(extension) || "jpg".equals(extension) || "jpeg".equals(extension))) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

확장자에 대해선 상수 또는 Enum 을 사용해볼수 있겠네요!

throw new IllegalArgumentException("수강신청은 모집중일때만 가능합니다.");
}
if (type == SessionType.CHARGE) {
if (checkMaxStudent()) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

들여쓰기를 하나만 해보면 좋을것 같아요!

private int price;
private int maxStudent;
private CoverImage coverImage;
private Students students = new Students(new ArrayList<>());
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Students 에 인자없는 생성자를 추가하여 new ArrayList<>() 를 할당해볼수도 있겠네요!

if(!("gif".equals(extension) || "png".equals(extension) || "jpg".equals(extension) || "jpeg".equals(extension))) {
throw new IllegalArgumentException("gif, png, jpg, jpeg 파일만 업로드 가능합니다.");
}
if(size > 1024 * 1024) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

매직넘버들의 경우엔 상수로 표현해보면 좋을것 같습니다.

private Long courceId;
private SessionDate sessionDate;
private SessionType type;
private int price;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

수강료와 최대 수강인원 등은 원시값 wrapping 을 적용해보면 좋을것 같습니다.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants