-
Notifications
You must be signed in to change notification settings - Fork 169
[4기 - 전선희] SpringBoot Part2 Weekly Mission 첫 번째 PR 제출합니다! #764
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
[4기 - 전선희] SpringBoot Part2 Weekly Mission 첫 번째 PR 제출합니다! #764
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.
선희님 2주차 과제도 고생하셨습니다~!! 😄
1주차가 마무리 되었다면 멘토님께 이야기 드려서 Merge 부탁드려요~
1주차에서 한번 밖에 리뷰하지 않은 것 같아서 리뷰가 늘어났네요..😅 간단하게 참고만 해주세요~!!
리팩터링이랑 테스트 코드 마무리 까지 화이팅하시길 바랍니다~~ 👍
p.s 테스트 코드 작성이 어렵다면 동료들의 도움을 받아보는 것은 어떨까요~?
| @ComponentScan( | ||
| basePackages = {"com.prgrms.spring"} | ||
| ) | ||
| static class Config { |
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.
Config를 따로 외부로 빼지 않고 내부 클래스로 두신 이유가 궁금합니다~
| @Bean | ||
| public DataSource dataSource() { | ||
| var dataSource = DataSourceBuilder.create() | ||
| .url("jdbc:mysql://localhost/voucher_system") |
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를 테스트 용도로 사용했을때 어떤 문제가 발생할 수 있을까요?
| try { | ||
| jdbcVoucherRepository.insert(newFixedAmountVoucher); | ||
| jdbcVoucherRepository.insert(newPercentAmountVoucher); | ||
| } catch (BadSqlGrammarException e) { | ||
| logger.error("Got BadSqlGrammarException error code -> {}", e.getSQLException().getErrorCode(), e); | ||
| } |
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.
어떤 의도로 테스트에서 예외를 잡아서 로그를 찍으신건가요?
| @DisplayName("아이디로 바우처를 조회할 수 있다.") | ||
| public void testFindById() { | ||
| var voucher = jdbcVoucherRepository.findById(newPercentAmountVoucher.getVoucherId()); | ||
| assertThat(voucher.isEmpty(), is(false)); |
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.
위의 테스트 처럼 있는지 뿐만 아니라 가져온 바우처가 동일한 Id의 바우처인지도 확인하면 좋지 않을까요? 필수는 아닙니다만, 저는 '비어 있지 않다' 보다 '조회하고자 한 결과가 맞음'을 보여주는게 좋다고 생각해 봤어요~
| <root level="error"> | ||
| <appender-ref ref="FILE"/> | ||
| </root> |
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 createCustomer() { | ||
| consoleView.showGetName(); | ||
| String name = consoleView.getCustomerName(); | ||
| consoleView.showGetEmail(); | ||
| String email = consoleView.getCustomerEmail(); | ||
| var request = CustomerCreateRequestDto.of(name, email); | ||
| Customer customer = customerService.createCustomer(request); | ||
| if (customer == null) { | ||
| consoleView.showErrorMsg(Error.CREATE_CUSTOMER_EXCEPTION); | ||
| logger.error("고객 등록 실패"); | ||
| return; | ||
| } | ||
| consoleView.showSuccessMsg(Success.CREATE_CUSTOMER_SUCCESS); | ||
| } |
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.
솔직히 흐름이 눈에 잘 들어오지 않는 것 같습니다~! 입출력 / 서비스 요청 / 결과 후 처리 로 나눠 볼 수 있을 것 같은데 메서드를 분리해보는 것은 어떠신가요?
| customerList.forEach(customer -> outputList.add(String.format("Name : %s \nEmail : %s\n", customer.getName(), customer.getEmail()))); | ||
| consoleView.showAllCustomers(outputList); |
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.
출력 포맷이 Controller 내에서 변경되는 것은 좋지 않다고 생각하는데 선희님 생각은 어떤지 궁금합니다~
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.
저도 동의합니다..! 서비스 단에서 처리하는 게 좋을 것 같아욤
| consoleView.showGetName(); | ||
| String name = consoleView.getCustomerName(); | ||
| consoleView.showGetEmail(); | ||
| String email = consoleView.getCustomerEmail(); | ||
| var request = CustomerCreateRequestDto.of(name, email); |
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.
consoleView에서 request를 받아오는 것은 어떻게 생각하시나요?
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.
그게 좋을 것 같아요!!
| Voucher voucher = voucherService.createVoucher(type, discount); | ||
| if (voucher == null) { | ||
| consoleView.showErrorMsg(Error.CREATE_VOUCHER_EXCEPTION); | ||
| logger.error("바우처 생성 실패"); | ||
| return; | ||
| } |
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.
어떤 이유로 생성에 실패했는지 알 수 있나요?
| @Override | ||
| public long getDiscount() { | ||
| return discount; | ||
| } |
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.
피드백과 좋은 자료 정말 감사합니다! 제가 아직 많이 부족한 건지 보내주신 아티클이 잘 이해가 안 가는 것 같아요..!ㅠ😭 시간 갖고 더 공부해보도록 하겠습니다!!
kakao-gray-great
left a comment
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 run(String... args) throws Exception { | ||
| boolean isExecute = true; | ||
| while (isExecute) { | ||
| logger.info("프로그램 시작."); |
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.
무의미한 로그 같습니다,,!
| MenuType menuType = null; | ||
| try { | ||
| menuType = MenuType.matchType(consoleView.getMenu()); | ||
| } catch (IllegalStateException e) { | ||
| logger.error("input error!"); | ||
| consoleView.showErrorMsg(Error.VALIDATION_WRONG_TYPE); | ||
| continue; | ||
| } | ||
| switch (menuType) { | ||
| case EXIT: | ||
| isExecute = false; | ||
| break; | ||
| case CREATE_VOUCHER: | ||
| voucherController.createVoucher(); | ||
| break; | ||
| case LIST_VOUCHER: | ||
| voucherController.getAllVoucher(); | ||
| break; | ||
| case CREATE_CUSTOMER: | ||
| customerController.createCustomer(); | ||
| break; | ||
| case LIST_CUSTOMER: | ||
| customerController.getAllCustomers(); | ||
| break; | ||
| } |
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.
해당 로직을 분리해보면 depth를 줄일 수 있겠네요!
| consoleView.showErrorMsg(Error.VALIDATION_WRONG_TYPE); | ||
| continue; | ||
| } | ||
| switch (menuType) { |
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.
선희님은 switch와 else if를 사용할 때, 어떠한 기준으로 선택하시나요?
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.
else if는 범위를 체크하거나 (<, >), 체크해야 할 조건이 여러개 있을 때, case가 적을 때 주로 사용하는 것 같습니다.
switch는 그 외에 사용하는 것 같아요!
| import org.springframework.stereotype.Component; | ||
|
|
||
|
|
||
| @AllArgsConstructor |
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.
@AllArgsConstructor를 붙이신 이유는 무엇인가요?
@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.
@AllArgsConstructor는 모든 필드에 대해, @RequiredArgsConstructor는 초기화 되지 않은 final이나 notnull과 같은 필수 필드에 대한 생성자를 만들어줍니다!
음 근데 이 상황에서는 All이나 Required나 상관이 없을 것 같아욤..!
| consoleView.showVoucherTypes(); | ||
| VoucherType type = VoucherType.matchType(consoleView.getVoucherType()); | ||
| consoleView.showVoucherPrompt(type); | ||
| Long discount = consoleView.getVoucherDiscount(); | ||
| Voucher voucher = voucherService.createVoucher(type, discount); | ||
| if (voucher == null) { | ||
| consoleView.showErrorMsg(Error.CREATE_VOUCHER_EXCEPTION); | ||
| logger.error("바우처 생성 실패"); | ||
| return; | ||
| } | ||
| consoleView.showSuccessMsg(Success.CREATE_VOUCHER_SUCCESS); |
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 String discountUnit = "$"; | ||
|
|
||
| public static FixedAmountVoucher newInstance(UUID voucherId, long discount) { | ||
| if (discount <= 0) throw new IllegalArgumentException("유효하지 않은 할인 금액"); |
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.
검증하는 로직의 메소드를 따로 분리해볼 수 있겠네요
| import java.util.List; | ||
|
|
||
| public interface Output { | ||
| void showMenu(); |
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.
CusomerRepository는 개행을 하셨는데, 여긴 개행이 없네요 ㅎ
개행을 하실 때, 기준이 있으실까요?
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.
아뇨,, 개행까진 고려하지 못했습니다..! 이런 것들도 신경써야 하군요..!!💡👍
| @AllArgsConstructor(access = AccessLevel.PROTECTED) | ||
| public class JdbcCustomerRepository implements CustomerRepository { | ||
|
|
||
| private static final Logger logger = LoggerFactory.getLogger(JdbcCustomerRepository.class); |
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.
log는 어떤 layer에서 남겨야된다고 생각하시나요?
| import org.springframework.boot.test.context.SpringBootTest; | ||
|
|
||
| @SpringBootTest | ||
| class ApplicationTests { |
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.
해당 테스트 코드는 필요 없지 않을까요 ㅎ
| @TestInstance(TestInstance.Lifecycle.PER_CLASS) | ||
| public class VoucherRepositoryTest { | ||
|
|
||
| private static final Logger logger = LoggerFactory.getLogger(VoucherRepositoryTest.class); |
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.
서비스를 배포하고 운영했을 때 발생하는 오류를 해결하고 유지보수하기 위함인 것 같아요!
테스트 코드에서는 필요없을 것 같습니다 !ㅎ.ㅎ
📌 과제 설명
5주차 강의에서 배운 내용들을 활용하여 Voucher와 Customer의 db 연동, 테스트 코드를 작성하였습니다!
(7/4 커밋부터 2차 과제입니다!)
👩💻 요구 사항과 구현 내용
(기본) 바우처 관리 애플리케이션
(심화) 바우처 지갑을 만들어보세요.
✅ 피드백 반영사항
✅ PR 포인트 & 궁금한 점
테스트 코드가 아직 익숙치 않아 많이 작성하지 못했습니다..! 추후에 더 작성하여 올리겠습니다😭