-
Notifications
You must be signed in to change notification settings - Fork 151
[4기 신재윤] Weekely Mission 1,3 PR 제출합니다 #273
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: Shin-Jae-Yoon
Are you sure you want to change the base?
[4기 신재윤] Weekely Mission 1,3 PR 제출합니다 #273
Conversation
.gitignore
Outdated
*.hprof | ||
|
||
# log file | ||
logs/ |
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.
줄바꿈~
build.gradle
Outdated
} | ||
|
||
dependencies { | ||
implementation 'org.springframework.boot:spring-boot-starter-data-jpa' |
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.
알아보기 쉽게 주석으로 표시해주시면 고마울 것 같아요!
return new Name(firstName, lastName); | ||
} | ||
|
||
private void checkNameLength(String firstName, String lastName) { |
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.
@pattern으로 정규표현식을 쓰지 않고 로직을 직접 작성한 이유가 있나요??
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.
-
Pattern 객체 내부를 봤을 때 new Pattern()이라는 점과 정규표현식의 생성비용이 크다고 알고 있어서 반드시 필요한 부분에서만 사용하는 것이 좋다고 판단했습니다 !
-
현재는 문자 내용에 대한 검증보단 길이에 대한 검증만 하고 있어서 pattern까지는 필요하지 않겠구나 생각했습니다 !!
@Column(name = "item_id") | ||
private Long itemId; | ||
|
||
private 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.
@column을 활용해 명시적으로 표현을 해주시면 좋을 것 가타요!
@Column(name = "order_status", nullable = false) | ||
private OrderStatus orderStatus; | ||
|
||
@Lob |
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.
(답변) @lob 애노테이션은 DB의 varchar(255) 이상인 경우, 즉, 문자열의 길이가 길어지는 경우 사용할 수 있는 형식입니다 ! 메모의 경우 길이가 길어질 수 있다고 생각하여 다음과 같이 지정했씀니다 :)
src/main/resources/application.yml
Outdated
|
||
h2: | ||
console: | ||
enabled: true |
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.
줄바꿈~
assertThat(all).hasSize(1) | ||
.contains(customer2); | ||
} | ||
} |
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.
줄바꿈~
|
||
@Embeddable | ||
@Getter | ||
@NoArgsConstructor |
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.
@NoArgsConstructor(access = AccessLevel.PROTECTED)와 생성자에 대한 부분을 수정하였습니다.
명확하게 알려주셔서 감사합니다 :)
재윤님 과제하느라 고생하셨습니다~ |
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.
리뷰할 게 크게 없었던 것 같아요! 재윤님 과제 고생하셨습니다~
// then | ||
assertThat(customer.getName()).isEqualTo(name); |
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에 제대로 업데이트가 되었는지 검증하려면, 해당 customer id로 조회한 값으로 검증하는 것이 맞지 않을까요?!
// given | ||
Customer customer = new Customer("재윤", "신"); | ||
repository.save(customer); | ||
Name name2 = new Name(); |
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.
수상한 놈 처리했습니다. 감사합니다
INVALID_FIRST_NAME_REQUEST("유효하지 않은 이름 길이입니다.\n"), | ||
INVALID_LAST_NAME_REQUEST("유효하지 않은 성 길이입니다.\n"); |
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.
재윤님 코드 잘봤습니다!
작성하시느라 고생하셨어요
저도 공통된 질문 빼고 나머지만 작성을 하였어요! 그리고 미처 못했는데 OwnerItem에서 양쪽 전부 Lazy를 사용하신 이유가 뭔가요?
코드가 전반적으로 깔끔했고, Name에서도 그렇고 코드 하나 작성하려 할 때 많은 생각이 담겨있는 거 같아 인상 깊었습니다!!
|
||
@Getter | ||
@RequiredArgsConstructor | ||
public enum ErrorMessage { |
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.
enm
에서 @RequiredArgsConstructor
를 사용하신 이유가 따로 있으신가요??
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 내의 필드들은 final 필드라서 그에 대한 생성자가 필요한데, 그에 맞게 필요한 것만 추가하도록 @requiredargsconstructor를 사용하여 코드를 줄이고 가독성을 높였습니다 !
@DisplayName("고객 등록 실패 - 이름 길이") | ||
@ParameterizedTest | ||
@CsvSource({ | ||
", 신", "1234567891011121314151617181920, 신" |
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.
이부분은 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.
재윤님 과제 하시느라 고생많았습니다!
저두 다른 분들과 겹쳐서 그나마 한부분에 대해서 의견을 남깁니다...!
@Test | ||
void contextLoads() { | ||
} | ||
|
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.
쓰지않는 테스트는 지우는게 좋을 것 같습니다!
상위 test/java 폴더를 우클릭하고 Run All Tests를 하면 그사이에 섞여서 보이게 됩니다!
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.
수상한 놈 제거했습니다. 감사합니다
📌 과제 설명
👩💻 요구 사항과 구현 내용
Mission 1
Mission 2
Mission 3