[4기 - 소재훈] SpringBoot Part2 Weekly Mission 제출합니다.#852
Open
jay-so wants to merge 159 commits intoprgrms-be-devcourse:jay-sofrom
Open
[4기 - 소재훈] SpringBoot Part2 Weekly Mission 제출합니다.#852jay-so wants to merge 159 commits intoprgrms-be-devcourse:jay-sofrom
jay-so wants to merge 159 commits intoprgrms-be-devcourse:jay-sofrom
Conversation
…ntroller, Service, Domain 이동
hanjo8813
reviewed
Jul 20, 2023
| private void selectCustomer() { | ||
| console.printCustomerSelectMenu(); | ||
| //선택 - Id, CreateAt, All | ||
| switch (CustomerSelectMenu.of(console.inputCommand())) { |
There was a problem hiding this comment.
정확히 어떤걸 의미하는지 나타내기 위해 이름을 가진 변수로 빼주는게 좋을것 같아요~
Comment on lines
+79
to
+83
| if (!customerListResponse.getCustomerResponseList().isEmpty()) { | ||
| console.printCustomerSelectByCreatedAt(customerListResponse); | ||
| } else { | ||
| console.printErrorMessage("현재 저장된 고객이 존재하지 않습니다."); | ||
| } |
There was a problem hiding this comment.
early return을 사용해보는건 어떨까요?
그리고 가독성을 높이기 위해 조건문의 순서를 바꿔주면 좋을것 같습니다.
Suggested change
| if (!customerListResponse.getCustomerResponseList().isEmpty()) { | |
| console.printCustomerSelectByCreatedAt(customerListResponse); | |
| } else { | |
| console.printErrorMessage("현재 저장된 고객이 존재하지 않습니다."); | |
| } | |
| if (customerListResponse.getCustomerResponseList().isEmpty()) { | |
| console.printErrorMessage("현재 저장된 고객이 존재하지 않습니다."); | |
| break; | |
| } | |
| console.printCustomerSelectByCreatedAt(customerListResponse); |
Comment on lines
+88
to
+92
| if (!customerListResponse.getCustomerResponseList().isEmpty()) { | ||
| console.printCustomerSelectAll(customerListResponse); | ||
| } else { | ||
| console.printErrorMessage("현재 저장된 고객이 없습니다."); | ||
| } |
Comment on lines
+35
to
+37
| public CustomerListResponse findByCreateAt() { | ||
| return customerService.findByCreateAt(); | ||
| } |
There was a problem hiding this comment.
현재 메소드 네이밍은 생성일로 특정 고객을 찾는다는 생각이 들게합니다.
모든 고객을 생성일 순으로 조회하는 것이니, 메소드 명은 아래처럼 바꾸는게 더 적합해 보이네요.
Suggested change
| public CustomerListResponse findByCreateAt() { | |
| return customerService.findByCreateAt(); | |
| } | |
| public CustomerListResponse findAllOrderByCreatedAt() { | |
| return customerService.findAllOrderByCreatedAt(); | |
| } |
Comment on lines
+71
to
+74
| String email = "valid@example.com"; | ||
|
|
||
| assertThat(new Customer(UUID.randomUUID(), "ValidName", email, LocalDateTime.now()).getEmail(), is(equalTo(email))); | ||
| } |
There was a problem hiding this comment.
어떤 동작인지 읽기가 힘드네요,, ㅜ
항상 given-when-then 패턴을 지켜주세요..!
|
|
||
| //해피 케이스 테스트 - Hamcrest 테스트 | ||
| @Test | ||
| @DisplayName("고객 생성 테스트") |
There was a problem hiding this comment.
이전 PR 리뷰 피드백이 되지 않은것 같습니다.
#775 (comment)
모든 DisplayName에서 멘토님께서 늘 강조하시는 테스트 설명 규칙을 꼭 지켜주세요.
참고 : #755 (review)
Comment on lines
+91
to
+99
| UUID customerId = UUID.randomUUID(); | ||
| LocalDateTime createdAt = LocalDateTime.now(); | ||
|
|
||
| Customer customer = new Customer(customerId, name, email, createdAt); | ||
|
|
||
| assertThat(customer.getCustomerId(), is(equalTo(customerId))); | ||
| assertThat(customer.getName(), is(equalTo(name))); | ||
| assertThat(customer.getEmail(), is(equalTo(email))); | ||
| assertThat(customer.getCreateAt(), is(equalTo(createdAt))); |
Comment on lines
+52
to
+53
| //then | ||
| assertThrows(IllegalArgumentException.class, () -> new FixedVoucher(discount)); |
Comment on lines
+25
to
+28
| @ActiveProfiles("test") | ||
| @SpringBootTest | ||
| @Transactional | ||
| class VoucherServiceTest { |
There was a problem hiding this comment.
이전 테스트에서 mocking을 사용하셨었는데 이렇게 변경하신 이유는 뭘까요?
@SpringBootTest가 단위테스트에 적합할까요?
단위테스트의 두가지 방식에 대해서도 고민해보세요. (고전파 vs 런던파)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📌 과제 설명
👩💻 요구 사항과 구현 내용
🧑🏻💻 프로그램 작동 설명
1️⃣ 메인화면
2️⃣ Voucher(바우처 관련 프로그램 화면) - (메인 화면에서 voucher 입력 시)
3️⃣ 바우처 생성(Create)
4️⃣ 바우처 조회 작업(Select)
5️⃣ 바우처 변경 작업(Update)
6️⃣ 바우처 삭제 작업(Delete)
7️⃣ Customer(고객 관련 프로그램 화면) - (메인 화면에서 customer 입력 시)
8️⃣ 고객 생성(Create)
9️⃣ 고객 조회 작업(Select)
🔟 고객 변경 작업(Update)
🕚 고객 삭제 작업(Delete)
✅ 피드백 반영사항
✅ PR 포인트 & 궁금한 점
-> 😭😭 설정 조건들이 많아, 모든 테스트 케이스(해피 테스트 케이스, 엣지 테스트 케이스)를 작성하지 못했습니다. (VoucherJdbcRepository,CustomerService 미작성)
-> 3차 미션 진행 중, 혹은 후에 테스트 케이스를 추가하도록 노력하겠습니다!
-> 피드백을 남겨주신 후 빠르게 확인하여 3차 미션에서 반영하도록 노력하겠습니다!