[team-03][BackEnd] todo-list 2주차 두번째 #228

nohriter · 2022-04-15T09:26:31Z

안녕하세요 3조의 짱민과 노리입니다.

벌써 마지막 PR이라니 아쉽습니다 ㅠㅠ 😭
2주 동안 부족한 코드 리뷰해주셔서 감사합니다!!

수요일 이후 리뷰내용 수정 하느라 추가 기능이 별로없네요!!
이번 진행상황은 아래와 같습니다.

2주차 1차 리뷰내용 반영

Repository에서 쿼리로 작성했던 카드이동 로직을 Service로 이동
테스트코드 DisplayName 개선
서비스에서 생성하던 CardResponse 로직을 생성자로 이동하였습니다.
- 내부로직은 Enum을 통해 개선할 수 있을 것 같은데 아직 개선하지 못했습니다.
Repository try-catch 일부 제거
기존 String 으로 하드코딩 되던 Location 정보를 Enum으로 개선

2주차 2차 개발 사항

CardRepository 테스트코드 작성

리뷰 감사합니다! 2주동안 고생 많으셨어요!!🙇

- CarsResponse 생성로직 생성자로 분리 - Enum 생성해서 기존 Location 하드코딩 개선

- insert, update, delete, findAll, updateLocation 테스트코드 작성

…to dev/backend

- Delete와 Move를 CardService에서 담당. - 추후 AOP를 통한 구현도 고려해볼만함.

…to dev/backend

ksundong

안녕하세요. 짱민 & 노리, 리뷰어 dion입니다.

첫번째 협업 프로젝트를 진행하느라 정말 수고 많으셨습니다~

CardRepository 테스트 열심히 진행해주셨네요. 몇가지 조언할 것이 있어 남겨두었으니 참고해주세요!
그리고 주석을 잘 작성해주는 것도 중요하긴 한데, 쓸모없는 주석을 남기지 않는 것이 더 중요합니다. 참고해주세요!
몇 가지 아쉬운 부분이 있어 변경요청을 드리니 확인하고 수정해주세요~ 고생하셨습니다.

ksundong · 2022-04-16T08:24:35Z

backend/todo-app/src/main/java/team03/todoapp/Service/CardService.java

        cardRepository.delete(cardId);
+
+        historyService.add(new History("remove", card.getTitle(), Optional.ofNullable(pastLocation), card.getCurrentLocation(),


왜 Optional을 인자로 넣어서 생성해주셨나요??

"remove"라는 문자열은 상수로 관리하거나 Enum을 쓰는게 낫지 않을까요?

ksundong · 2022-04-16T08:31:28Z

backend/todo-app/src/main/java/team03/todoapp/Service/CardService.java

    public void update(Long cardId, CardUpdateFormRequest request) {
        Card card = cardRepository.findById(cardId)
-            .orElseThrow(() -> new IllegalArgumentException("카드를 찾을 수 없습니다."));
+            .orElseThrow(() -> new NoSuchElementException("카드를 찾을 수 없습니다."));

        card.update(request.getTitle(), request.getContent());
        cardRepository.update(card);
    }

    public Card findOne(Long cardId) {
        Card card = cardRepository.findById(cardId)
-            .orElseThrow(() -> new IllegalArgumentException("카드를 찾을 수 없습니다."));
+            .orElseThrow(() -> new NoSuchElementException("카드를 찾을 수 없습니다."));
        return card;
    }

    public CardsResponse findAll() {
-        List<Card> cards = cardRepository.findAll();
-
-        return createCardsResponse(cards);
-    }
-
-    private CardsResponse createCardsResponse(List<Card> cards) {
-        CardsResponse cardsResponse = new CardsResponse();
-
-        List<CardResponse> todos = new ArrayList<>();
-        List<CardResponse> ings = new ArrayList<>();
-        List<CardResponse> dones = new ArrayList<>();
-
-        for (Card card : cards) {
-            if (card.getCurrentLocation().equals("todo")) {
-                todos.add(new CardResponse(card));
-            } else if (card.getCurrentLocation().equals("ing")) {
-                ings.add(new CardResponse(card));
-            } else if (card.getCurrentLocation().equals("done")) {
-                dones.add(new CardResponse(card));
-            }
-        }
-
-        cardsResponse.putCards("todo", todos);
-        cardsResponse.putCards("ing", ings);
-        cardsResponse.putCards("done", dones);
-
-        return cardsResponse;
+        return new CardsResponse(cardRepository.findAll());


여기에는 @Transactional 이 필요없나요?

ksundong · 2022-04-16T08:32:08Z

backend/todo-app/src/main/java/team03/todoapp/Service/HistoryService.java

    public void add(History history) {
+
        historyRepository.insert(history);
    }


여기서는 @Transactional 필요가 없나요~~?

ksundong · 2022-04-16T08:32:19Z

backend/todo-app/src/main/java/team03/todoapp/WebConfig.java

@@ -17,7 +17,7 @@ public void addInterceptors(InterceptorRegistry registry) {
        registry.addInterceptor(historyMakerInterCeptor)
            .order(1)
            .addPathPatterns("/card")
-            .addPathPatterns("/card/{cardId}")
+            //.addPathPatterns("/card/{cardId}");  // 서비스단으로 옮김


사용하지 않는 코드는 제거해주세요.

ksundong · 2022-04-16T08:32:50Z

backend/todo-app/src/main/java/team03/todoapp/controller/CardController.java

+    public void removeCard(@PathVariable("cardId") Long cardId) {
+        // putHistoryDataToRequest(request, "remove", null, cardService.findOne(cardId));

        cardService.remove(cardId);
    }

    @PatchMapping("/card/move/{cardId}")
    public void moveCard(@Validated @RequestBody CardMoveFormRequest cardMoveFormRequest,
-        BindingResult bindingResult, HttpServletResponse httpServletResponse,
-        @PathVariable("cardId") Long cardId, HttpServletRequest request) {
-        if (bindingResult.hasErrors()) {
-            log.error("bindingResult: {}", bindingResult);
-            httpServletResponse.setStatus(HttpServletResponse.SC_BAD_REQUEST);
-            return;
-        }
-        String pastLocation = cardService.findOne(cardId).getCurrentLocation();
+        BindingResult bindingResult, @PathVariable("cardId") Long cardId) {
+
+        //CardLocation pastLocation = cardService.findOne(cardId).getCurrentLocation();
+        //putHistoryDataToRequest(request, "move", pastLocation, cardService.findOne(cardId));


주석 처리한 코드가 너무 많이 등장하는 것 같아요. 필요 없으면 지워주세요.
아니면 주석의 사유를 적어주세요.

ksundong · 2022-04-16T08:44:12Z