[4기 - 박세연] SpringBoot Part2 Weekly Mission 제출합니다. #777

parksey · 2023-07-08T12:39:40Z

📌 과제 설명

회원에 대한 CRUD기능 구현 및 많은 단위 테스트 작성을 하였고, JdbcTemplate을 통해 dao를 구현 하였습니다.

👩‍💻 요구 사항과 구현 내용

입력 기능 추가

CLI (Command linke inteface)한 환경에서 실행되도록 한다.

[ 프로그램 실행 구문 ]

=== Manage Program ===
Type customer to select customer menu.
Type voucher to select customer menu.
Type exit to exit the program.

[ voucher 실행 구문 ]

=== Voucher Manage ===
Type exit to exit the program.
Type create to create a new voucher.
Type list to list all vouchers.

[ customer 실행 구문 ]

=== Customer Manage ===
Type exit to exit the program.
Type create to create a new customer.
Type list to list all customers.

바우처 추가 기능

JdbcTemplate을 사용해서 구현
CRUD 모든 기능 추가

회원 추가 기능

CRUD기능을 통해 회원를 변경할 수 있다.

exit: 종료
create: 회원 생성
delete: 회원 삭제
delete_all: 회원 전체 삭제
find_all: 회원 전체 조회
find_detail: 이메일을 통한 상세 검색
update: 유저 정보 업데이트

구현 기능

1. 입력

회원과 바우처 선택하는 기능

2. 바우처

JdbcTemplate를 사용하여 Repostiroy 관리
CRUD 기능

3. 회원

4. 그 외

테스트 코드 작성

✅ 피드백 반영사항

적절한 네이밍

클래스 명이 의도와 다른 것들을 바꿨습니다.
- Ex) ReadException -> InputValidator
- InputException: RuntimeException을 상속받는 custom클래스

불필요한 this

this를 통해 더 명확하게 알 수 있을 거라 생각했는데, 혹시 가독성 측면에서 지양하는 건가요?

Response 인터페이스

반환하는 모든 response 클래스에 붙여 동일한 동작을 할 수 있도록 하려고 적용했습니다!

✅ PR 포인트 & 궁금한 점

각 객체가 가져야 하는 기능을 가지고 있는 지가 궁금합니다!
- 특히 DTO와 domain object간 매핑하는 기능을 DTO에 포함 시켰는데 이렇게 해도 되는 것인지 아님 다른 Mapper클래스로 분리하는 것이 좋은지 궁금합니다
- 객체가 생성을 하게 될 때, new 내부에는 생성하는 기능만을 가지고 있다고 생각하여 예외 처리가 있을 경우 전부 of메서드를 통해 하게 했는데, 생성자 내부에서 해도 상관 없는지 궁금합니다.

joseph-100

과제하시느라 고생 하셨습니다.

joseph-100 · 2023-07-12T04:09:37Z

src/main/java/org/weekly/weekly/voucher/dto/response/VouchersResponse.java

+    public VouchersResponse(List<Voucher> vouchers) {
+        this.result = vouchers.stream()
+                .map(VoucherCreationResponse::new)
+                .collect(Collectors.toUnmodifiableList());


toList() 로 대체 가능합니다.

넵 변경하겠습니다.

joseph-100 · 2023-07-12T04:09:55Z

src/main/java/org/weekly/weekly/VoucherManagementController.java

+    public void start() {
+        boolean isExit = false;
+
+        while(!isExit) {


가능하면 ! 조건에 부정을 두지 않는 것이 가독성에 좋습니다. isRunning = true 로 하고, false 일 때 반환하는것이 좋을거 같아요.

좋은 의견 감사합니다!

joseph-100 · 2023-07-12T04:10:13Z

src/main/java/org/weekly/weekly/VoucherManagementController.java

+        return true;
+    }
+
+    private boolean processVoucherMenuSelection(VoucherMenu selectMenu) {


리턴값을 사용하지 않는데 반환하는 이유가 있나요?

console 기능에서 customer에 대한 기능이 추가될 때 못 바꾼 것 같습니다 바로 고칠게요!

joseph-100 · 2023-07-12T04:10:37Z

src/main/java/org/weekly/weekly/ui/CommandLineApplication.java

-    private final CommandWriter commandWriter;
+    private final SystemWriter commandWriter;

    @Autowired


생성자에 Autowired 애노테이션이 어떤 의미가 있나요?

생성자에 Autowired 어노테이션을 붙이면서 DI를 명시하는 것입니다.
여러 생성자가 있을 경우 Autowired는 필수이며, 스프링 버전이 올라가면서 생성자가 단일인 경우 생략이 가능하지만 여러 개인 경우 필수로 알고 있습니다!

joseph-100 · 2023-07-12T04:11:37Z

src/main/java/org/weekly/weekly/ui/writer/SystemWriter.java

+        println(PrintMessageType.NAME_INPUT.getMessage());
+    }
+
+    public void printReuslt(String result) {


오타 있습니다.

수정하겠습니다

joseph-100 · 2023-07-12T04:12:01Z

src/main/java/org/weekly/weekly/voucher/domain/PercentDiscount.java

-import org.weekly.weekly.util.DiscountType;
-
 public class PercentDiscount implements Discount{
    private final int PERCENT = 100;


상수로 둘거면 static 을 두세요.

넵 알겠습니다

joseph-100 · 2023-07-12T04:12:40Z

src/main/java/org/weekly/weekly/voucher/domain/Voucher.java

 package org.weekly.weekly.voucher.domain;

+import org.weekly.weekly.util.ExceptionMsg;
+import org.weekly.weekly.voucher.exception.VoucherException;


미사용 import 제거해 주세요.

제거했습니다!

joseph-100 · 2023-07-12T04:13:02Z

src/main/java/org/weekly/weekly/voucher/domain/Voucher.java

        }

-        this.amount -= afterAmount;
+        this.amount += afterAmount;


이 케이스가 어떤건지 잘 이해가 되지 않습니다.

afterAmount가 0보다 큰 경우에는 바우처 금액을 다 사용한 경우이기 때문에 그대로 반환했지만
afterAmount가 0보다 작은 경우, 바우처로 할인할 수 있는 금액이 제공받은 금액(beforeAmount)보다 크기 때문에 바우처 금액을 남기는 것을 표현했습니다.

joseph-100 · 2023-07-12T04:14:52Z

src/test/java/org/weekly/weekly/voucher/JdbcVoucherRepositoryTest.java

+    }
+
+    @Test
+    @Order(1)


테스트는 독립적으로 실행될 수 있게 해야 변경과 유지보수에 용이 합니다.

넵! 테스트가 많아 질 수록 서로 연관 관계를 겪게 되어 불편하다고 느끼고 있었는데, 좋은 의견 감사합니다!
다만 하나 궁금한 점은 테스트에서 사용하는 db의 경우 schema만 적용하여 data.sql은 적용하지 않고 각각의 테스트를 진행하나요?
작성하다 보니 굳이 필요하다는 생각이 안 들어서 질문드립니다.

joseph-100 · 2023-07-12T04:17:03Z

src/test/java/org/weekly/weekly/util/DiscountMapTest.java

테스트는 명확하게 해야 보기 더 쉽습니다. "할인번호를_입력하면_통과" 같은 경우는 고정할인, 퍼센트할인 테스트를 분리하는게 좋습니다.

주의하겠습니다!

Hunkik

과제 고생하셨습니다.

저는 생성자 내부에 validation 메서드를 호출하게 자주 씁니다
DTO와 매퍼는 선택이라고 생각합니다. 저는 개인적으로 DTO는 정말 데이터 전송용 객채로만으로 자주쓰긴 합니다.

나중에 이력서볼때 같이 라이브코딩 하시면좋을것같아여. 코드는 많이 좋아졌습니다!

Hunkik · 2023-07-13T13:14:49Z

src/main/java/org/weekly/weekly/VoucherManagementController.java

+    private void handleVoucherCreation() {
+        VoucherCreationRequest voucherCreationRequest = commandLineApplication.createVoucherFromInput();
+        Response response = voucherController.createVoucher(voucherCreationRequest);
+        logger.info("{}{}", PrintMessageType.CREATE_VOUCHER_SUCCESS.getMessage(),response.getResult());
+        commandLineApplication.printResult(response);
+    }
+
+    private void handleVoucherSearch() {
+        Response response = voucherController.getVouchers();
+        logger.info("{}{}", PrintMessageType.FIND_ALL_VOUCHER_SUCCESS.getMessage(), response.getResult());
+        commandLineApplication.printResult(response);
+    }


메서드 안에 너무 많은 책임이 있는것같습니다. Request생성, 요청, 결과출력까지 너무 수정에 취약합니다.

확실히 책임이 많긴 하네요. 그래서 handle이라는 메서드 명을 줘서 전반적인 처리를 하는 것처럼 보이게 하긴 했지만 어떻게 해결을 해야할지 애매하네요,,

Hunkik · 2023-07-13T13:16:17Z

src/main/java/org/weekly/weekly/VoucherManagementController.java

+        if (CustomerMenu.CREATE.equals(selectMenu)) {
+            handleCustomerCreation();
+            return false;
+        }
+
+        if (CustomerMenu.DELETE.equals(selectMenu)) {
+            handleCustomerDelete();
+            return false;
+        }
+
+        if (CustomerMenu.DELETE_ALL.equals(selectMenu)) {
+            handleCustomerDeleteAll();
+            return false;
+        }
+
+        if (CustomerMenu.FIND_ALL.equals(selectMenu)) {
+            handleCustomerFindAll();
+            return false;
+        }
+
+        if (CustomerMenu.FIND_DETAIL.equals(selectMenu)) {
+            handleFindDetail();
+            return false;
+        }
+
+        if (CustomerMenu.UPDATE.equals(selectMenu)) {
+            handleUpdateCustomer();
+            return false;
+        }


switch로도 한 번 바꿔보시는거 추천드립니다. 가독성 더 좋은걸로 선택하시는것도 괜찮아보여요

예전 우테코 프리코스에서 else를 사용하지 말라는 그런 조건이 있어서 유지하고 있었는데, 굳이 꼭 그럴 필요는 없이 가독성이 더 좋은 것을 선택해 보겠습니다!

Hunkik · 2023-07-13T13:23:26Z

src/main/java/org/weekly/weekly/customer/controller/CustomerController.java

+    public CustomerResponse findDetailCustomer(CustomerUpdateRequest updateRequest) {
+        return customerService.findDetailCustomer(updateRequest);
+    }
+
+    public CustomersResponse findAllCustomer() {
+        return customerService.findAllCustomer();
+    }
+
+    public CustomerResponse updateCustomer(CustomerUpdateRequest updateRequest) {
+        return customerService.updateCustomer(updateRequest);


Response를 공유해서 쓰는건 비효율적입니다. 추후 Create와 Update가 각각 다른 값을 반환하는데 Response가 값이 같으면 요청하는쪽에서 혼란이 올 수 있습니다.

처음에는 ResponseEntity처럼 공통된 값을 반환하는 것을 생각했는데 오히려 혼란이 올 수 있다는 생각을 못했네요. 수정하겠습니다!

Hunkik · 2023-07-13T13:23:49Z

src/main/java/org/weekly/weekly/customer/domain/Customer.java

+    }
+
+    public static Customer of(UUID uuid, String name, String email) {
+        CustomerValidator.validateEmailFormat(email);


validation을 생성시에 넣는건 개인적으로 좋아하는 패턴입니다. 저는 생성자 내부에서 자주 씁니다 ㅎㅎ

넵! api에서도 값을 가져올때 생성자자에서 처리를 안 하다 보니까 생성자에서 예외처리를 해야하나 라고 고민하고 있었는데 좋은 의견 감사합니다!

Hunkik · 2023-07-13T13:34:54Z

src/main/java/org/weekly/weekly/customer/service/CustomerService.java

+    public CustomerResponse findDetailCustomer(CustomerUpdateRequest updateRequest) {
+        String email = updateRequest.email();
+        Customer customer = validateCustomerExistAndGet(email);
+        return CustomerResponse.of(customer);
+    }


find도 @transaction(readOnly = true)를 해주면 좋습니다!( db마다 다르긴합니다.)

해당 구문에서는 어짜피 select만 하기 때문이겠네요
넵!

Hunkik · 2023-07-13T13:39:30Z

src/main/java/org/weekly/weekly/ui/exception/InputException.java

+import org.weekly.weekly.util.ExceptionMsg;
+
+public class InputException extends RuntimeException{
+


클래스 시작시 공백 통일해주세요! (자기만의 컨벤션 지키면서 개발하기)

넵 고치겠습니다!

Hunkik · 2023-07-13T13:59:13Z

src/main/java/org/weekly/weekly/ui/reader/ConsoleWrap.java

 import java.io.Console;

 @Component
+@ConditionalOnProperty(value="command.read", havingValue = "console")


제가 안써봤는데, 혹시 어떤코드인지 알 수 있을까요:?

spring.config.activate.on-profile: default command: write: system read: scanner

application.yaml에 이렇게 되어있습니다.

해당 경우 @Profiles와 @ConditionalOnProperty 중 어떤 것을 사용할 까 선택해야 했습니다.

먼저
[ ConditionalOnProperty란 ]

어노테이션에서 적용한 profiles와 매칭이 되는 클래스만 빈에 등록 했다면, ConditionalOnProperty의 경우 특정 값에서 동작한다는 것을 명시하고 yaml에서 맞는 값을 적용하면 동작이 되도록 합니다.

결국, 제가 생각하기에는 주체가 클래스에 있냐, yaml에 있냐를 통해 조절한다고 느꼈습니다.
(클래스마다, 하나의 이름과 같은 프로퍼티를 적용하기 때문)

[ 문제 상황 ]

Profiles는 동작하는 profile에 맞게 빈에 등록하기 때문에 profile의 이름이 변경되면 해당 프로파일을 참고하는 모든 클래스를 찾아서 다시 바꿔줘야 한다는 생각이 있었습니다.

어떤 환경에서 동작을 하게 할 것인지, 한번에 파악하기가 힘들었습니다.
예를 들어,

default: System클래스와 Scanner클래스

dev: System클래스와 Console클래스

local: System클래스와 Scanner클래스

등으로 이렇게 구성이 복잡하게 연결되어 있다고 가정했었는데,
코드 자체를 Profiles로 보면 Profiles("!dev"), Profiles({"dev", "test"}) 처럼 적용되다 보니 위와 같은 문제가 발생했습니다.

[ 해결 ]
따라서, 위와 같은 문제를 해결하기 위해 주체를 클래스로 적용하여 각 Profile마다 다르게 동작할 수 있도록 적용하였습니다.

감사합니다! 저도 하나 배워가네요

Hunkik · 2023-07-13T14:00:03Z

src/main/java/org/weekly/weekly/ui/writer/SystemWriter.java

+    private final Logger logger = LoggerFactory.getLogger(SystemWriter.class);
+    private void println(String msg) {
+        System.out.println(msg);
+    }
+    private void print(String msg) {System.out.print(msg);}
+


공백이 좀 보기 힘든것같습니다

고치겠습니다!

Hunkik · 2023-07-13T14:04:34Z

src/main/java/org/weekly/weekly/voucher/domain/DiscountType.java

+
+

공백이 두개입니다!

넵 확인했습니다!

parksey added 26 commits July 3, 2023 01:05

refactor: VoucherController의 동작 역할 분리

61ad3c6

refactor: voucher 생성 및 예외처리 책임 변경

21a8a67

test: voucher에 대한 테스트 코드 변경

f1a7a71

feat: 전체 데이터 조회 반환하는 데이터 형식 및 parsing방법 변경

03456cb

docs: 2주차 미션 기능명세서 작성

685e7db

feat: db관련 의존성 추가

d7c6407

feat: 스키마 및 설정 추가

30c2ff5

fix: VoucherRepository insert 반환값 변경

30b77b0

test: JdbcVoucherRepository 테스트 코드 추가

4b977a2

feat: Jdbc를 통한 Voucher dao 구현 (CRUD), Voucher Repo 통합 테스트 추가

2d01fdf

test: Customer 테스트 코드 추가

59ff2d2

feat: Customer예외 처리 기능 추가

9b020b2

feat: Customer와 Voucher메뉴 선택 분기 추가

1be3ab2

refactor: SystemWriter 클래스의 공통 기능 분리

21652c0

refactor: PrintMessage enum클래스의 클래스명 변경 -> PrintMessageType

1a2c307

feat: 사용자 요청에 따른 dto 및 기능 추가

b69816e

feat: 사용자 입력에 따른 VoucherManageController 기능 추가

a0d1d8b

feat: 동작에 따른 customerservice 기능 추가

7dac4e6

feat: CustomerRepository에 대한 테스트 코드 작성

9e55e66

fix: VoucherRepository 존재하는 pk에 대한 중복 insert 예외 처리 추가

269c9fb

test: Customer 업데이트 테스트 코드 추가

ea61bc3

docs: 기능 명세서 심화 기능 추가

169600b

feat: Customer 동작에 따른 CRUD service 기능 추가

6232845

refactor: 불필요한 this연산자 제거 및 controller 반환값 추가

581db86

feat: Util 클래스 public 생성자 생성시 error throw 하도록 변환

9981e62

fix: 회원 생성시 날짜 오류 수정 및 Manage의 exit만 종료하게 변경

1e2b909

parksey requested a review from joseph-100 July 8, 2023 12:39

parksey added 🔥 매운맛 Team-Joseph labels Jul 8, 2023

joseph-100 reviewed Jul 12, 2023

View reviewed changes

Hunkik reviewed Jul 13, 2023

View reviewed changes

parksey merged commit 17d053c into prgrms-be-devcourse:parksey Jul 14, 2023

parksey mentioned this pull request Jul 15, 2023

[4기 - 박세연] SpringBoot Part3 Weekly Mission 제출합니다. #836

Closed

23 tasks

		import org.weekly.weekly.util.ExceptionMsg;

		public class InputException extends RuntimeException{

[4기 - 박세연] SpringBoot Part2 Weekly Mission 제출합니다. #777

[4기 - 박세연] SpringBoot Part2 Weekly Mission 제출합니다. #777

Uh oh!

Conversation

parksey commented Jul 8, 2023

📌 과제 설명

👩‍💻 요구 사항과 구현 내용

입력 기능 추가

바우처 추가 기능

회원 추가 기능

구현 기능

1. 입력

2. 바우처

3. 회원

4. 그 외

✅ 피드백 반영사항

적절한 네이밍

불필요한 this

Response 인터페이스

✅ PR 포인트 & 궁금한 점

Uh oh!

joseph-100 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hunkik left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

parksey Jul 14, 2023 •

edited

Loading