-
Notifications
You must be signed in to change notification settings - Fork 151
[4기 한승원] Mission 1 PR 제출합니다 #264
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
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.
수고하셨습니다 승원님 ! 간단하게 리뷰남겼으니 확인해보시면 좋을 것 같습니다 😀
public void changeFirstName(String firstName) { | ||
this.firstName = firstName; | ||
} | ||
|
||
public void changeLastName(String lastName) { | ||
this.lastName = 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.
이름을 각각 수정하는 하는 요구사항이 있을까요..?? 이름을 수정해야 한다면 한꺼번에 수정하지 않을까 싶습니다. 그렇다면 VO 로 묶는 것도 고려해보시면 좋을 것 같아요 😁
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.
이름/성 각각 수정을 할수있도록 둔 것은
개명을 한다고 생각하면 성은 그대로 두고 이름만 바뀌고 반대로 성씨만도 바뀌는 경우도 있어서입니다~
VO로 묶는 것은 말씀해주신대로 고려해볼게요!!
@Override | ||
public boolean equals(Object o) { | ||
if (this == o) { | ||
return true; | ||
} | ||
if (o == null || getClass() != o.getClass()) { | ||
return false; | ||
} | ||
Customer customer = (Customer)o; | ||
return (id == customer.id) && | ||
(Objects.equals(firstName, customer.firstName)) && | ||
(Objects.equals(lastName, customer.lastName)); | ||
} | ||
|
||
@Override | ||
public int hashCode() { | ||
return Objects.hash(id, firstName, 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.
@Getter 는 롬복을 사용하셨는데, hashcode, equals 는 따로 사용하지 않으셨네요 ! 혹시 롬복을 사용하는 기준이 따로 있으신가요 ??
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.
@EqualsAndHashCode
를 사용할 수도 있지만 그경우 모든 필드를 기반으로 하는데
양방향 연관관계를 가지는 entity의 경우 무한 호출로 이어질 수 있습니다. 그래서 equals/hashcode를 재정의 하거나 @EqualsAndHashCode(of="id")
로 아이디만 거는 것이 좋을 거 같습니다. 그런데 현재 id로 데이터베이스에서 생성해주는 값을 사용하기에 준영속상태의 객체들은 null을 가지고 있을 수 있고 그러면 비교 시에 같다고 나올 수 있습니다. 그래서 equals, hashcode를 재정의했습니다.
지금의 경우라면 직접 재정의하는 것과 롬복을 사용해 모든 필드를 거는 것이 별상관은 없겠네요!!
@Nested | ||
@DisplayName("고객 조회에 대한 테스트") | ||
class read_test { | ||
|
||
@Nested | ||
@DisplayName("존재하는 고객에 대해 식별자(id)로 검색하는 경우") | ||
class existing_customer { | ||
|
||
@Test | ||
@DisplayName("정상적으로 데이터를 읽어 그 데이터를 반환한다 ") | ||
void save() { | ||
//Given | ||
Customer customer = new Customer("승원", "한"); | ||
customerRepository.save(customer); | ||
long testId = customer.getId(); | ||
|
||
// When | ||
Customer result = customerRepository.findById(testId).get(); | ||
|
||
// Then | ||
assertThat(result, is(equalTo(customer))); | ||
} | ||
} | ||
|
||
@Nested | ||
@DisplayName("존재하지 않는 고객에 대해 식별자(id)로 검색하는 경우") | ||
class non_existing_customer { | ||
|
||
@Test | ||
@DisplayName("Optional.empty를 반환한다") | ||
void save() { | ||
//Given | ||
// When | ||
Optional<Customer> readCustomer = customerRepository.findById(-1L); | ||
|
||
// Then | ||
assertThat(readCustomer, is(equalTo(Optional.empty()))); | ||
} | ||
} | ||
} |
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.
이렇게 Nested 형태로 구현하신 이유가 있으실까요..??
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.
고객 조회/추가/수정/삭제 등 각 기능에 대한 성공/실패 에 대해 다양한 테스트를 진행한다고 할 때
계층을 두면 기능 내에서 원하는 테스트 메서드를 찾거나 추가할 때도 쉬울 것이라고 생각했습니다.
그리고 클래스 별로 별도의 초기화 과정이 필요하다면 다른 테스트에 영향주지 않고 구성할 수 있는 점도 이점이라 생각했습니다
jpa: | ||
generate-ddl: true | ||
hibernate: | ||
ddl-auto: create-drop | ||
database: H2 | ||
show-sql: 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.
create-drop 으로 설정하신 이유가 있으실까요..??
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.
아 테스트를 위해서 기존테이블을 삭제하고 생성하기 위해서 그렇게 두었는데
실제 운영되는 디비라고 생각하면 ddl-auto를 사용하지 않고 직접 쿼리를 적용하거나 validate/none으로 변경할것 같습니다!
📌 과제 설명
Customer 엔티티를 생성하고 그에 따른 CRUD 기능 테스트 구현했습니다.
👩💻 요구 사항과 구현 내용
✅ PR 포인트 & 궁금한 점
어색하거나 의견 주실 부분 있으면 알려주세요!😀