Skip to content

Conversation

@forkyy-dev
Copy link

어느덧 마지막 pr을 보내는 날이 왔네요😿
시그리드 덕분에 많은 성장을 할 수 있었던것 같습니다! 항상 정성이 담긴 피드백을 주셔서 감사합니다~

이번 pr은 수요일 피드백을 적용하는것에 초점을 맞췄습니다!

리뷰 적용 사항

1. cardController의 add 메서드 return 값이 애매하다는 리뷰.

  • EventService를 CardService 내부로 이동.

  • 변경 전 코드

  • 변경 후 코드

2. responsCardDto에게 section 값을 비교하는 책임을 위임하기 & else 제거하기

  • Card 도메인 객체에게 section의 변경 여부를 확인하는 책임 위임.
  • if문 내부에서 값을 리턴하는 방식을 사용해 else 제거. -> return 문이 중복된다는 단점이 있지만 다른 방식을 찾지 못했습니다.

3. put, delete 메서드에 최소한 요청 성공 여부를 알려주는 반환값 정도는 주도록 개선하기

   {
     "code" : "200",
     "message" : "성공",
     "content" : "{...}"
   }
  • Response 형태의 Dto를 만들고 내부에 content 필드의 타입을 제네릭으로 설정하는 방식으로 변경하면 좋겠다고 생각했습니다.
  • 모든 응답을 위의 형식으로 보내주는게 맞을것같다고 전부터 생각했고 추후 반영할 사항으로 미뤄두다가 프로젝트가 끝났습니다.. 😵

4. Action enum 내부에서 Event가 스스로 dto의값들을 사용하도록 개선하기

  • Event에게 자율성을 보장해주기 위해 기존 dto의 값을 꺼내 주던 형식에서 dto 객체를 넘겨주는 형식으로 변경

5. ResponseCardsDto에서 Map의 키값을 enum 타입을 넣어서 표현하도록 개선

  • Section Enum 클래스를 작성하고 카드를 분류하는 로직을 Enum 클래스 내부로 옮김

앞으로 적용해야할 사항

  • eventJdbcRepository의 rowMapper에서 컬럼명을 동적으로 관리할 방법 찾아보기
  • findAny는 id가 unique하지 않을 경우 동일성이 보장이 안됨 → 개선 해보기
  • custom한 Exception 만들어보기
  • action을 인터셉터로 구현해보기

@forkyy-dev forkyy-dev added the review-BE Improvements or additions to documentation label Apr 15, 2022
@forkyy-dev forkyy-dev requested a review from sigridjineth April 15, 2022 04:12
Copy link

@sigridjineth sigridjineth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요, 리뷰어 Sigrid Jin입니다.
이번 프로젝트 정말 수고 많으셨구요. 클린코드의 입장에서 정말 장족의 발전이 있었다고 평해볼 수 있겠습니다.
많은 것을 가져가실 수 있던 시간이 되길 바라며, 다음에 다시 찾아뵙도록 하겠습니다. 궁금한 점 있으시면 언제든 디엠 부탁하구요. 감사합니다.

ResponseCardDto responseCardDto = cardService.addCard(requestCardDto);
eventService.addEvent(new RequestEventDto(responseCardDto), Action.ADD);
return responseCardDto;
return cardService.addCard(requestCardDto);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

서비스 객체로 책임과 역할을 분리시켜, 컨트롤러는 거들 뿐이라는 사실을 상기했습니다. 👍

} else {
eventService.addEvent(new RequestEventDto(prevSection, responseCardDto), Action.MOVE);
}
cardService.updateCard(id, requestCardDto);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

와 눈물이 나는 변화로군요 💯


@GetMapping("/events")
public List<ResponseEventDto> getEvents() {
public List<Event> getEvents() {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이번에는 도메인 객체를 직접 넘겼지만, 결국 DTO 세트를 담아서 넘겨야 하는 시간이 올 것입니다.
누가 왜 그렇게 구현했나요, 무슨 차이가 있나요 라고 면접에서 물어본다면 대답하실 수 있기를 바랍니다.

return result;
}

private static Section findSection(String sectionTitle) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

enum에 직접 비즈니스 로직을 넣어준 점 아주 좋습니다 👍

List<Card> responseSection = result.get(findSection(section).sectionTitle);
responseSection.add(card);
}
return result;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

result 도 별도의 object로 wrapping하는 것에 대해서는 어떻게 생각하시나요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jypthemiracle 해당 메서드는 요청에 대한 응답값으로 넘겨줄 Map객체를 반환하는 메서드입니다. 하지만 서비스에서 해당 맵객체를 ResponseCardsDto로_ Wrapping하는데 굳이 또 감쌀 필요가 있는지 잘 모르겠습니다!
혹시 제가 놓친 이유가 있다면 알려주시면 감사하겠습니다!

ADD((dto, action) -> new Event(dto, action)),
MOVE((dto, action) -> new Event(dto, action)),
UPDATE((dto, action) -> new Event(dto, action)),
REMOVE((dto, action) -> new Event(dto, action));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

훨씬 깔끔하죠? 💯

public class ResponseCardsDto {

private Map<String, List<ResponseCardDto>> cards;
private Map<String, List<Card>> cards;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Card를 mapping하는 object를 별도로 wrapping해서 만들 수는 없을까요?

return card.toResponseCardDto();
}

@Transactional

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이번 프로젝트의 마지막 숙제입니다.
@Transactional 의 쓰임과 작동 방식을 조사해서 자신만의 언어로 정리해주세요.

@sigridjineth
Copy link

이만 머지하겠습니다. 커멘트 추가로 달아보시고 궁금한 점 있으시면 저에게 디엠 주세요. 즐거운 학습과정이 되시기 바랍니다.

@sigridjineth sigridjineth merged commit f552d22 into codesquad-members-2022:team-08 Apr 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review-BE Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants