[럿고] 2단계 - 점진적인 리팩터링 미션 제출합니다. #19

ksy90101 · 2020-10-19T11:16:16Z

안녕하세요. 휴!
이번 미션에서는

비지니스 로직 도메인으로 이동
setter 제거
dto 생성 및 dto 레이어간 통신
서비스 테스트 mock 제거 및 통합테스트로 진행

이렇게 4개를 중심적으로 했습니다.
비지니스 로직을 이동시키는 작업은 너무 어렵네요 ㅠㅠ
아울러 모두 통합테스트로 진행하다 보니 setup 하는 코드들이 너무 더럽네요... ㅎㅎ
피드백을 반영하면서 계속해서 리팩토링 해보도록 하겠습니다!
잘 부탁드리겠습니다~

Hue9010

리뷰가 늦었습니다. 🙏
피드백과 간단한 퀴즈?남겼는데 확인 부탁드려요.

Hue9010 · 2020-10-21T10:39:25Z

build.gradle

@@ -16,6 +16,7 @@ dependencies {
    implementation 'org.springframework.boot:spring-boot-starter-actuator'
    implementation 'org.springframework.boot:spring-boot-starter-data-jpa'
    implementation 'org.springframework.boot:spring-boot-starter-web'
+    implementation 'org.springframework.boot:spring-boot-starter-validation'


Hue9010 · 2020-10-21T10:45:41Z

src/main/java/kitchenpos/dto/MenuProductRequest.java

+public class MenuProductRequest {
+    @NotNull(message = "상품명을 입력해주세요.")
+    private final Long productId;
+    @NotNull(message = "수량을 입력해주세요.")


validation을 건다면 notnull이 아니라 0 또는 1 이상일 것 같네요.

헉... 꼼꼼하지 못했네요 ㅠㅠ

Hue9010 · 2020-10-21T10:52:22Z

src/main/java/kitchenpos/dto/MenuProductResponse.java

+    public static MenuProductResponse of(final MenuProduct menuProduct) {
+        return new MenuProductResponse(menuProduct.getSeq(), menuProduct.getMenuId(), menuProduct.getProductId(),
+                menuProduct.getQuantity());
+    }
+
+    public static List<MenuProductResponse> ofList(final List<MenuProduct> menuProducts) {
+        return menuProducts.stream()
+                .map(MenuProductResponse::of)
+                .collect(Collectors.toList());
+    }


Effective Java 아이템 1. 생성자 대신 정적 팩터리 메서드를 고려하라.

from: 매개변수를 하나 받아서 타입의 인스턴스를 반환하는 형변환 메서드 Date d = Date.from(instance) of : 여러 매개변수를 받아 적합한 타입의 인스턴스를 반환하는 집계 매서드 set<Rank> faceCards = EnumSet.of(JACK,QUEEN,KING) valueOf : from과 of의 더 자세한버전 BigInteger prime = BigInteger.valueOf(Integer.MAX_VALUE); instance or getInstance : 매개변수로 명시한 인스턴스를 반환하지만 같은 인스턴스임을 보장하지는 않는다. StatckWalker luke = StackWalker.getInstance(option); create or newInstance 매번 새로운 객체를 반환한다. getType : getInstance와 같으나 생성할 클래스가 아닌 다른 클래서의 팩터리 메서드를 정의할때 쓴다. FileStore fs = Files.getFileStore(path) newType : new Type과 같으나 생성할 클래스가아닌 팩터리 메서드를 정의할 때 쓴다. BufferedReader br = Files.newBufferedReader(path); type : getType과 newType의 간결한버전 List<Complaint> litany = Collections.List(lagacyLitany);

위 규칙은 어디까지나 이펙티브 자바에서도 추천하는 명명규칙일 뿐 절대 법칙은 아니기때문에 약간의 변형을 하거나 지킬 필요는 없지만 특별한 이유가 없다면 많이들 사용하는 명명규칙을 이용하는 걸 추천드려요.

헉... 그렇다면 혹시 ofList가 아니라 List를 쓰는게 더 좋다고 생각하시는건가요!?

from을 생각하긴 했지만 이런 부분은 럿고가 기준을 가지고 하시면 돼요. 혹은 팀에서 정한 컨벤션이 있다면 그걸 따르거나요.
지킬 필요는 없다고 했는데 생각해보니 Java9에 추가된 List.of가 대표적일 것 같네요.
https://docs.oracle.com/javase/9/docs/api/java/util/List.html

Hue9010 · 2020-10-21T10:56:23Z

src/main/java/kitchenpos/dto/TableChangeEmptyRequest.java

+import java.beans.ConstructorProperties;
+
+public class TableChangeEmptyRequest {
+    private final boolean empty;


private final boolean empty; private final Boolean empty;

여기서 퀴즈 위 두개의 차이가 무엇일까요? (그냥 옆에서 간단히 물었다고 생각하고 바로 생각나는데로 간단히..ㅎㅎ)

기본형과 Wrapper 클래스로써 null을 허용할것인지 아닐것인지가 차이가 있을꺼 같습니다!...ㅎㅎ

넵 맞습니다. 그러면 요청측에서 empty를 비어 보내면 어떻게 될까요?
Boolean은 요청한 값이 없으니 null이, boolean은 false가 담깁니다. 후자의 상황으로 예상치 못한 버그가 생길 수도 있고, 디폴트 값으로 false를 가지게 의도하고 사용하기도 합니다.
개인적으로는 명시적이지 못해서 값이 없을 때의 디폴트 값이 false라도 Boolean 타입으로 만들고 디폴트 값을 코드상에 명시적으로 드러나게 구현하는 편입니다. 이것보다는 의도하지 않았는데 false가 담기는 상황을 조심해야 합니다. 마찬가지로 int, Integer도 0, null의 차이가 생겨서 아래 답변 주신 null이 안들어 갈 것같다는게 맞긴하지만 0이 입력된다는게 더 정확할 것 같네요.

그러고보니 몇달 전에 회사분이랑 얘기하다가 0, 1도 Boolean에 true/false로 매핑해주는데(아마 boolean도 될 겁니다.) 이게 확실한가 찾아봤던 적이 있는데
Spring @RequestParam mapping Boolean based on 1 or 0 instead of true or false
위 답글에도 있지만 더 정확히는 StringToBooleanConverter를 보면 컨버터에서 어떻게 무슨 값들을 Boolean에다가 매핑해주는지 알 수 있답니다. 😉

Hue9010 · 2020-10-21T11:04:01Z

src/main/java/kitchenpos/dto/TableChangeGuestsRequest.java

+
+public class TableChangeGuestsRequest {
+    @Min(value = 0, message = "0명 미만이 될수 없습니다.")
+    private final int numberOfGuests;


위의 boolean, Boolean이랑 같은 질문이 될 것 같네요.

private final int numberOfGuests; private final Integer numberOfGuests;

이 두가지의 차이가 무엇일까요? 🤔

추가로 Min annotation의 자바독을 보면 특정 설명이 적혀있는데

{@code null} elements are considered valid.

이 사항까지 고려하신건지 궁금합니다.

기본형과 Wrapper 클래스로써 null을 허용할것인지 아닐것인지가 차이가 있을꺼 같습니다!...ㅎㅎ
아울러 기본형이기 때문에 null이 안들어올거 같아서 사용하지 따로 null Check를 하지는 않았습니다!

Hue9010 · 2020-10-21T11:16:59Z

src/main/java/kitchenpos/dto/OrderCreateRequest.java

+    private final Long orderTableId;
+
+    @NotEmpty(message = "주문 항목이 비어있을수 없습니다.")
+    private final List<OrderLineItemRequest> orderLimeItems;


오타가 있는 것 같네요. orderLimeItems 😉

헉.. 꼼꼼하지 못했네요 ㅠ ㅠ

Hue9010 · 2020-10-21T12:20:20Z

src/test/java/kitchenpos/ui/ProductRestControllerTest.java

-        product.setName("후라이드 치킨");
-        product.setPrice(BigDecimal.valueOf(16000));
+        final ProductRequest product = new ProductRequest("후라이드 치킨", BigDecimal.valueOf(16000));
+
        create("/api/products", product)


테스트가 잘 통과하고 있나요? 🤔

@RequestBody에 왜 기본 생성자는 필요하고, Setter는 필요 없을까?

2.2. @ConstructorProperties JDK 1.6부터 제공되었던 @java.beans.ConstructorProperties 은 생성자의 파라미터 이름을 지정하는 표준적인 방법입니다. 이를 활용하면 생성자의 파라미터 이름을 Reflection API를 통해서 알 수 있습니다. Jackson은 2.7.0버전부터 @ConstructorProperties 를 인지합니다. ( https://github.com/fasterxml/jackson-databind/issues/905 참조) 생성자에 @ConstructorProperties 으로 파라미터의 이름을 지정하면, Jackson에서는 동일한 이름의 JSON솔성값을 생성자로 넘겨줍니다.

Jackson 파싱 전략(불변 객체 활용)
이걸 이용해 DTO의 필드를 final로 처리했습니다.

그러나 궁금했던점이 있었는데요. 이걸 안붙혀도 가능한 경우가 있었습니다. 그 이유가 무엇일지 너무 궁금합니다..ㅠㅠㅠ (기본생성자가 없는데도 DTO 사용이 가능했습니다. 그 경우가 바로 ProductRequest였는데요....ㅎㅎ 찾아도... 잘 모르겠습니다.ㅠ ㅠ

이건 저도 조금 더 확인해볼게요. 🤔

Hue9010 · 2020-10-21T12:35:13Z

src/main/java/kitchenpos/application/TableGroupService.java

            if (!savedOrderTable.isEmpty() || Objects.nonNull(savedOrderTable.getTableGroupId())) {
-                throw new IllegalArgumentException();
+                throw new IllegalArgumentException("비어있지 않은 테이블은 그룹으로 지정할 수 없습니다.");
            }


OrderTable 내로 들어 갈 수 있을 것 같아 보이지 않나요. 🤔

Hue9010 · 2020-10-21T12:45:59Z

src/main/java/kitchenpos/application/TableGroupService.java

            savedOrderTable.setTableGroupId(tableGroupId);
-            savedOrderTable.setEmpty(false);


tableGroupId를 바꾸고, empty 상태로 만드는게 뭘 의미하나요?

지금처럼 단순위 값을 바꾸는 행위만 하면 이 행위가 뭘 의도하는 건지 추측 혹은 사전에 공유받아야 이해할 수 있어요.

OrderTable.setUpGroupTable(tableGroupId);

public void setUpGroupTable(Long tableGroupId) { this.tableGroupId = tableGroupId; this.empty = false; }

처럼 의도를 나타내는 메서드명으로 값을 세팅하면 어떨까요?(딱 이거다 하는 메서드명은 안떠오르네요. 😢 )

지금처럼 직접 set하면(비록 change로 set 이라는 이름은 안썼지만) 다른 곳에서의 재활용 성도 떨어지고 테이블을 그룹핑 할 때 empty를 true로 바꿔야 한다는걸 모든 작업자가 숙지를 하고 있어야해요. 지금처럼 단순히 두개의 값이라면 실수할 여지가 적지만 여러개의 복잡한 값이라면 매번 어떤 값을 어떻게 바꿔줘야하는지 알기 쉽지 않답니다.

Line 83에 있는

orderTable.setTableGroupId(null); orderTable.changeEmpty(false);

도 테이블을 치우다?? 등 행위가 무엇을 의도하는지 나타내줄 수 있겠죠?

오... 너무 좋네요 ㅠㅠ 이생각을 왜 못했을까요! ㅎㅎ
감사합니다!

이제보니 OrderService의 Line 56에도 같은 형태가 있네요. 입력 받은 값으로 바꾼다만 생각하고 작업하다 보면 이렇게 간단한 값들은 의식하지 않으면 이런 형태로 구현해버리는 경우가 많답니다. 😏

Hue9010 · 2020-10-21T20:13:57Z

우선 리뷰가 늦은점 다시한번 죄송합니다. 😭 😭
리뷰를 완전히 다 끝난 상태는 아니지만 리뷰 도중 대응해야 할 업무가 생겨 대응하다보니 마무리를 못지었네요. 리뷰를 전부 완료하고 피드백을 드리면 더 늦어질 것 같아 현재까지 리뷰한 내용 전달 드립니다. 🙏

우선 service 레이어에 전체적으로 비즈니스 로직이 많이 남아 있는걸로 보여요. 이 부분은 피드백 드린게 있는데 다른 service에서도 대체적으로 유사한 패턴을 보이고 있어요. 사실 이것만 개선되면 충분해 보여요.
그리고 간단한 퀴즈(?) 남겼는데 코멘트나 DM으로 답변 주시면 이건 리뷰와 별개로 빠른 피드백 드리도록 할게요. 😉

다시한번 늦은데다가 완벽하게 피드백 드리지 못한점 죄송합니다. 🙇 😭

ksy90101 · 2020-10-29T13:08:57Z

말씀드린 기간에 비해 늦었네요 ㅠㅠ
하면서 최대한 옮긴다고 옮겼는데 정상적으로 옮겼는지 잘 모르겠습니다 ㅠㅠ
일단 양방향 문제는 3단계 때 변경하려고 그대로 남겨뒀습니다.
항상 피드백 감사드립니다 👍

Hue9010

깔끔히 구현해주셨네요! 👍
이번 단계는 여기서 머지할게요. 고생하셨어요.

Hue9010 · 2020-10-31T19:01:28Z

src/main/java/kitchenpos/dto/TableChangeEmptyRequest.java

+import java.beans.ConstructorProperties;
+
+public class TableChangeEmptyRequest {
+    private final boolean empty;


넵 맞습니다. 그러면 요청측에서 empty를 비어 보내면 어떻게 될까요?
Boolean은 요청한 값이 없으니 null이, boolean은 false가 담깁니다. 후자의 상황으로 예상치 못한 버그가 생길 수도 있고, 디폴트 값으로 false를 가지게 의도하고 사용하기도 합니다.
개인적으로는 명시적이지 못해서 값이 없을 때의 디폴트 값이 false라도 Boolean 타입으로 만들고 디폴트 값을 코드상에 명시적으로 드러나게 구현하는 편입니다. 이것보다는 의도하지 않았는데 false가 담기는 상황을 조심해야 합니다. 마찬가지로 int, Integer도 0, null의 차이가 생겨서 아래 답변 주신 null이 안들어 갈 것같다는게 맞긴하지만 0이 입력된다는게 더 정확할 것 같네요.

Hue9010 · 2020-10-31T19:16:17Z

src/main/java/kitchenpos/dto/TableChangeEmptyRequest.java

+import java.beans.ConstructorProperties;
+
+public class TableChangeEmptyRequest {
+    private final boolean empty;


그러고보니 몇달 전에 회사분이랑 얘기하다가 0, 1도 Boolean에 true/false로 매핑해주는데(아마 boolean도 될 겁니다.) 이게 확실한가 찾아봤던 적이 있는데
Spring @RequestParam mapping Boolean based on 1 or 0 instead of true or false
위 답글에도 있지만 더 정확히는 StringToBooleanConverter를 보면 컨버터에서 어떻게 무슨 값들을 Boolean에다가 매핑해주는지 알 수 있답니다. 😉

Hue9010 · 2020-10-31T20:00:14Z

src/test/java/kitchenpos/ui/ProductRestControllerTest.java

-        product.setName("후라이드 치킨");
-        product.setPrice(BigDecimal.valueOf(16000));
+        final ProductRequest product = new ProductRequest("후라이드 치킨", BigDecimal.valueOf(16000));
+
        create("/api/products", product)


이건 저도 조금 더 확인해볼게요. 🤔

SeYun added 8 commits October 16, 2020 16:32

docs: 2단계 요구사항 작성

be1836b

refactor: product 도메인으로 비지니스 로직 이동 및 서비스 이동 dto로 변경

bb2ba68

refactor: menuGroup 서비스 로직 통신 dto로 변경

accdb6e

refactor: menu 비지니스 로직 도메인으로 이동 및 dto 생성

a0c021f

refactor: table 비지니스로직 도메인으로 이동 및 dto 생성

bc992dd

refactor: tableGroup 비지니스 로직 이동 및 dto 생성

bc24567

refactor: order 비지니스 로직 도메인으로 이동 dto 생성

7ce1945

refactor: order dto 통신으로 변경

3679c94

ksy90101 changed the base branch from master to ksy90101 October 19, 2020 11:16

Hue9010 suggested changes Oct 21, 2020

View reviewed changes

SeYun added 4 commits October 29, 2020 21:07

refactor: JDBC를 JPA로 변환

1e26841

refactor: OrderTable 값 변경 도메인으로 로직 이동

68e4a49

refactor: menuProduct의 Product 값 객체로 변경

592a63e

refactor: 에러 메시지 변경

8258cfb

Hue9010 approved these changes Oct 31, 2020

View reviewed changes

Hue9010 merged commit 75dce73 into woowacourse:ksy90101 Oct 31, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[럿고] 2단계 - 점진적인 리팩터링 미션 제출합니다. #19

[럿고] 2단계 - 점진적인 리팩터링 미션 제출합니다. #19

ksy90101 commented Oct 19, 2020

Hue9010 left a comment

Hue9010 Oct 21, 2020

Hue9010 Oct 21, 2020

ksy90101 Oct 29, 2020

Hue9010 Oct 21, 2020

ksy90101 Oct 29, 2020

Hue9010 Oct 31, 2020

Hue9010 Oct 21, 2020

ksy90101 Oct 29, 2020

Hue9010 Oct 31, 2020

Hue9010 Oct 31, 2020

Hue9010 Oct 21, 2020

Hue9010 Oct 21, 2020

ksy90101 Oct 29, 2020

Hue9010 Oct 21, 2020

ksy90101 Oct 29, 2020

Hue9010 Oct 21, 2020

ksy90101 Oct 29, 2020

Hue9010 Oct 31, 2020

Hue9010 Oct 21, 2020

Hue9010 Oct 21, 2020

ksy90101 Oct 29, 2020

Hue9010 Oct 31, 2020

Hue9010 commented Oct 21, 2020

ksy90101 commented Oct 29, 2020

Hue9010 left a comment

Hue9010 Oct 31, 2020

Hue9010 Oct 31, 2020

Hue9010 Oct 31, 2020

		savedOrderTable.setTableGroupId(tableGroupId);
		savedOrderTable.setEmpty(false);

[럿고] 2단계 - 점진적인 리팩터링 미션 제출합니다. #19

[럿고] 2단계 - 점진적인 리팩터링 미션 제출합니다. #19

Conversation

ksy90101 commented Oct 19, 2020

Hue9010 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hue9010 commented Oct 21, 2020

ksy90101 commented Oct 29, 2020

Hue9010 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment