[레거시 코드 리팩토링 - 2단계] 케빈(박진홍) 미션 제출합니다. by xlffm3 · Pull Request #109 · woowacourse/jwp-refactoring

xlffm3 · 2021-10-24T21:55:24Z

안녕하세요 포모~

2단계 미션은 다음과 같이 진행했습니다.

Service Layer에 산재한 비즈니스 로직들을 Domain으로 이동
- 이 과정에서 JPA를 사용했는데요. 당장은 추가적으로 쿼리가 여러번 나가는 것을 크게 신경쓰지 않고 최대한 서비스 레이어의 가독성 및 객체와 객체들간 메시지를 주고받도록(?) 했습니다.
- 불필요한 setter 제거 및 생성자 사용 -> 지난 미션때는 일부러 setter를 사용해서 코드가 지저분해졌는데 많이 개선됬습니다.
- DAO 제거 및 Repository 추가
DTO 생성
- 예... 초기 코드가 컨트롤러, 서비스 모두다 도메인에 직접 의존하고 있는 구조인데요. 서비스 코드 리팩토링하면서 함께 진행하는게 테스트 작성하기 더 수월할것같아서 진행했는데.... 3단계 요구사항이었네요 ㅎㅎ...ㅎ.ㅎ.;;;
도메인 단위 테스트 추가
- 이름 입력에 대해 빈 문자열 등을 허용할 것인지에 대한 구체적인 요구사항이 없어서 그 부분은 생략했습니다..!

bosl95

안녕하세요 케빈!!!! 💪
꼼꼼한 코드 정말 잘 보았습니다!!!!!! 엔티티가 많고 복잡한 부분도 있었는데, 코드를 워낙 잘 작성해주셔서 읽는 데 크게 불편함을 못 느꼈네요 👍👍👍 많이 배워갑니다 !
크게 수정할 필요가 있다고 느껴진 부분은 없었구요!
작성해주신 코드에 대한 생각이 궁금해 몇자 코멘트 남겨보았어요! 보시고 다시 요청 보내주세요!!

bosl95 · 2021-11-01T14:28:56Z

src/main/java/kitchenpos/application/MenuGroupService.java

-        this.menuGroupDao = menuGroupDao;
+    private final MenuGroupRepository menuGroupRepository;
+
+    public MenuGroupService(MenuGroupRepository menuGroupRepository) {


코드들 보면 보통 인자에 final이 붙어있던데, 이 친구만 final이 없는 이유가 있을까요???

앗 사실 저는 인스턴스 필드를 제외한 파라미터 부분이나 지역변수에는 final 키워드 작성을 선호하지 않는데요. (너무 길어져서)
초기에 제공된 기본 코드는 final이 전부 붙어있었는데 리팩터링하면서 일부분을 지웠나봅니다~

bosl95 · 2021-11-01T23:28:36Z

src/main/java/kitchenpos/ui/MenuGroupRestController.java

+    private MenuGroupResponse convert(MenuGroupResponseDto menuGroupResponseDto) {
+        return new MenuGroupResponse(menuGroupResponseDto.getId(), menuGroupResponseDto.getName());


저는 컨트롤러에 이런 변환 로직이 많이 들어가는 걸 선호하지 않아서 menuGroupResponseDto.ToXXX()이런 식으로 DTO 내부에서 처리해주기도 해요! 케빈도 이미 아실 거 같긴 한데 어떠한 이유로 이렇게 작성해주셨을까요?!

메서드가 있었는데 깜빡하고 저걸 그대로 쓰고있었네요 ㅎㅎㅎ;;

bosl95 · 2021-11-01T23:40:36Z

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

+        List<OrderTable> orderTables = tableGroupRequestDto.getOrderTables()
+            .stream()
+            .map(orderTable -> convert(orderTable.getId(), tableGroup))
+            .collect(Collectors.toList());


이 부분도 DTO에게 List로 변환해서 반환해주도록 하면 서비스 레이어의 로직을 좀 더 작게 가져갈 수 있지 않을까 하는 생각입니다! 케빈은 어떻게 생각하시나요??

음 이부분은 convert를 보니

특정 그룹으로의 연관관계 맵핑 등의 로직이 있어서 서비스 레이어가 적합하다고 생각해서 두었습니다!

bosl95 · 2021-11-01T23:49:29Z

src/main/java/kitchenpos/ui/dto/response/tablegroup/TableGroupResponse.java

+
+    public static TableGroupResponse from(TableGroupResponseDto tableGroupResponseDto) {
+        if (Objects.isNull(tableGroupResponseDto)) {
+            return null;


궁금한 점이 있습니다!!

여기서 Null이 반환되면 이 이 TableGroupResponse의 null excpetion이 발생하는 곳은 어디일까요??? get()하기 전까지 이 Null을 계속 안고 가는 걸까요??

그리고 TableGroupResponse가 null인 이유는 TableGroupResponseDTO가 null이여서 일텐데, 만약 TableGroupResponse을 get으로 호출하다 null 예외가 발생할 경우 TableGroupResponseDTO가 null이기때문이라는 것을 파악하기 쉽지 않을 거 같아보였어요!

어떻게 생각하시나요!!

OrderTable과 TableGroup은 N:1 관계인데요. nullable = true입니다.

이같이 처음 OrderTable을 생성하는 경우 그 응답으로 OrderTableResponse(혹은 DTO)를 생성할 때 OrderTableResponse 내부에

이렇게 TableGroupResponse도 작성하는 로직이 있는데요.

tableGroup이 null일때 해당 코드가 없으면 npe가 발생합니다.

지금 제공된 API 스펙의 응답이 정해진게없어서 Response를 공통으로 쓰다보니 저런 분기가 조금 생긴것같은데요 ㅠㅠ
tradeoff라 생각됩니다..

bosl95 · 2021-11-01T23:52:32Z

src/main/java/kitchenpos/application/MenuGroupService.java


-import java.util.List;
-
+@Transactional(readOnly = true)


케빈 코드 볼때마다 느끼는 거지만 정말 꼼꼼하신 게 느껴집니다 👍 멋지네요!!

bosl95 · 2021-11-02T00:06:53Z

src/main/java/kitchenpos/ui/dto/response/menu/MenuGroupResponse.java

+    public static MenuGroupResponse from(MenuGroupResponseDto menuGroupResponseDto) {
+        return new MenuGroupResponse(menuGroupResponseDto.getId(), menuGroupResponseDto.getName());
+    }
+


오 ... 앞에서 제가 toXXX()로 변환에 대해 언급한 부분이 딱 이런 느낌일 거 같은데, 변환 로직을 dto에서 작성하시고 안하시는 기준이 있으신 걸까요?????/

으음 전달해야할 인자가 3개 이상 많다고 느껴지는 경우는 별도로 분리하고 그 밑은 그냥 두는 편인데요.
지금보니 하나로 통일해주는게 좋을것같네요 :)

지금보니 메서드를 만들어놨는데 제가 안쓰고 있었네요... ㅋㅋ ㅠㅠ

bosl95 · 2021-11-02T00:10:27Z

src/test/java/kitchenpos/domain/MenuTest.java

+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
+
+@DisplayName("Menu 엔티티 단위 테스트")


진짜 ... 꼼꼼 그 자체 ... 👍👍👍👍

bosl95 · 2021-11-02T00:11:54Z

src/test/java/kitchenpos/domain/OrderTableTest.java

+    @ParameterizedTest
+    @ValueSource(ints = {-2, -1})


Junit 배운 이후로 레벨 2 정도까지 쓰다가 잘 안 쓰게 되었는데, 케빈 코드 보면서 이런 것도 배웠었지 했습니다 ㅎㅎ 활용도 정말 잘하시네요 ㅎㅎ

bosl95

다음 단계 진행을 위해 머지하겠습니다! 수고하셨어요~
(그런데 수정된 부분이 없는 걸까요? 아니면 커밋 내역에 없는 건가요?? 이 부분 한번 확인부탁드려요!)

xlffm3 added 3 commits October 25, 2021 04:36

feat: jpa 적용 및 서비스 코드 리팩토링

df170db

refactor: 컨트롤러 dto 변경 및 테스트 수정

bee83f8

chore: repository 패키지 분리 및 코드 포맷팅

dd99745

xlffm3 changed the base branch from master to xlffm3 October 24, 2021 21:55

xlffm3 added 3 commits October 25, 2021 06:56

chore: stacktrace 제거

d0e9cca

test: 누락된 도메인 단위 테스트 추가

cbbce36

refactor: 컨트롤러 api 문서에 맞게 dto 수정 및 패키지 정리

1c83f06

bosl95 suggested changes Nov 2, 2021

View reviewed changes

bosl95 approved these changes Nov 25, 2021

View reviewed changes

bosl95 merged commit ccea391 into woowacourse:xlffm3 Nov 25, 2021

		private MenuGroupResponse convert(MenuGroupResponseDto menuGroupResponseDto) {
		return new MenuGroupResponse(menuGroupResponseDto.getId(), menuGroupResponseDto.getName());


		import java.util.List;

		@Transactional(readOnly = true)

Conversation

xlffm3 commented Oct 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bosl95 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!

xlffm3 Nov 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

bosl95 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

xlffm3 commented Oct 24, 2021 •

edited

Loading

xlffm3 Nov 20, 2021 •

edited

Loading

bosl95 left a comment •

edited

Loading