코드리뷰 요청 PR

[kimminkyeu]

유원영 멘토님 안녕하세요!

코드 리뷰 상세 노션 페이지

https://www.notion.so/E2E-1247e19ca5ef4b0eb1cbea7845c3dff0?pvs=4

발표 장표 및 질문 내용은 위 노션 페이지에 정리했습니다.
감사합니다.

@kimminkyeu @Minju-Kimm @tigris24 @limbaba1120

[yoowonyoung]

리뷰 요청 1. 이미지 분석 기능

두 메소드에 대한 리팩토링 방식

• updateExistingColor
  • �메소드가 너무 길다고 생각이 드는 경우, 정말 이 메소드는 한가지 동작만 하고 있는가? 를 먼저 점검 해보면 좋습니다. 정말 한가지 동작만 하고 있는 것인가요? 두가지 이상의 동작을 하는건 아닌가요?
  • 두가지 이상의 동작으로 구분되는 경우, 다른곳에서 쓰이지 않는다면 클래스 내부의 private 메소드로 선언을 해서 사용하고, 추후 다른곳에서 필요하게 되는 경우 이것을 public으로 변경하는 방법이 있습니다
  • 이를 극단적으로 제한해서 메서드 하나가 5줄을 넘지 않게 하는 파이브 라인스 오브 코드 라는 책도 있습니다. 한번 읽어보면 도움이 될거에요
  • 또한, 코드 내부에서 클래스를 사용 하거나, 클래스의 생성자를 이용해서 해결할 수 있는 부분이 많아 보여요

PostPopularSearchCondition condition = new PostPopularSearchCondition();
				condition.setCreatedAt(post.getCreatedAt());
				condition.setLikeCount(post.getLikeCount());
				condition.setViewCount(post.getHits());

와 같은 코드는 PostPopularSearchCondition 에서 PostEntity를 파라미터로 받는 생성자를 만들고, 그 생성자에서 저 값들을 할당 해준다면 생성자 호출 단 한줄로 해결될 문제입니다.

• calcCloseColorsDist

  • 레디스 ZSet은 내부적으로 zip 리스트 혹은 skip 리스트를 사용 합니다(https://bugoverdose.github.io/posts/86/)
  • skip 리스트를 사용하게 되는 경우엔 메모리 사용량이 증가하게 되는데요, 이는 레디스를 클러스터로 구성해서 샤딩이 가능한 환경을 만들어두면 걱정은 줄어들것으로 보여요. 따라서, 레디스 서버의 용량은 얼마나 되며 얼마나 사용하게 될지를 미리 계산을 해보고 이에 맞춰 서버 환경을 구성 해둔다면 크게 문제는 없을수도 있습니다

• 실행시간 관련

  • 먼저 동작시간이 왜 오래걸리는지, 어디서 오래 걸리는지를 파악 해야 합니다. 따라서 각 연산 사이 사이 로깅 혹은 응답시간 측정등을 통해서 어느 부분에서 가장 오래걸리는지를 먼저 파악해야 어떻게 리팩토링을 할지를 고민 할수가 있죠. 이 부분을 먼저 한번 해보았으면 좋겟습니다
  • 단적인 예로, 코드 내부에서 반복문을 통해 계산을 하거나 객체를 만들어주는 등 다양한 동작을 하고 있는데요. 이부분에서 시간이 오래걸린다는게 확인이 되었다면 이 부분을 병렬로 수행할수있는지, 수행 할 수 있다면 병렬로 수행했을 경우 응답시간은 어떻게 변경되는지를 고민 해보는 과정을 거쳐야 합니다

비동기 호출 기능

• 비동기 메서드 체이닝
  • 가독성 관련해서는 일단 제가 보기엔 한가지 방식으로 통일되어 있어서 코드를 읽는데 어려움이 없었어요. 모든 개발이 그렇습니다. 어떤 방식이던지 좋은것 나쁜것이 아니라 통일만 되어있고 그 스타일이 유지가 된다면야 문제는 없어요.
  • 다만, 여기서는 제가 보기엔 onGoingTasks 를 이용해서 현재 진행중인 작업을 기록하고 하는 과정을 보았을때, 이 방법이 맞나? 라는 고민은 좀 드네요. 결국 각 Task가 끝나고 그 다음이 실행되어야 하는데, 그렇다면 이것을 각각의 이벤트를 발행하는 메시지큐가 필요 했던건 아닐지 한번 생각 해보았으면 좋겟어요.
  • signleThreadExecutor를 꼭 써야 하는게 아니라면 기본으로 제공되는 ForkJoinPool을 활용 하도록 생략하거나, 전역에서 공통적으로 쓰일 Executor가 필요하다면 Bean으로 등록해서 이를 DI를 통해 주입받아 쓰는것도 방법일것 같아요

Batch

• Spring Batch는 다양한 기능들이 제공되는데요, 기존 Spring의 다양한 기능들을 그대로 사용할 수 있어서 러닝커브가 높지 않기 때문에, 인터넷을 통해 쉽게 작성할 수 있는게 장점입니다. 하지만 대충 작성하면 안되죠. 꼭 따로 공부 해보길 추천 할게요
• deprecated 된 코드들은 가급적 지양하는게 당연합니다. 어떤 코드로 대체되었는지 확인 해보고 변경 하는게 좋습니다
• 어느 정도까지 Spring Batch를 이해햐느냐는 내가 Batch 로 어느 작업들을 처리 하느냐에 따라 다릅니다. 현재 예시 달아준것처럼 간단한 Save만 한다면 이정도로도 충분 하겠지만 복잡한 로직이 필요 하게 되었다면 공부를 당연히 더 해야겟죠. 지금 당장은 Spring Batch의 기본 구조와, 동작 원리, Spring Batch의 기본 테이블들에 대한 이해 정도만 있다면 문제 될건 없어보이네요

게시글 작성 기능

• 리팩토링에서 코드의 나쁜 냄새중 하나가 중복된 코드입니다. 3진아웃을 생각해서 2번 반복된다면 의심을, 3번 반복된다면 무조건 리팩토링을 하는것을 추천 하는데요, 중복되는 코드는 대부분 클래스를 선언하거나 메서드를 선언 하는것으로 해결 할 수 있습니다. PostDto.PostDtoResponse 하는 부분은 생성자에서 PostEntity 를 받아서 직접 할당하는것도 방법일것 같구요, ApiPostException.builder를 통해서 각종 Exception을 발생시키는것도 각 에러 카테고리에 해당하는 ApiUserException 의 하위클래스를 만들어서 에러메시지만 파라미터로 넘기는것도 방법이겟죠.
• 아예 중복되는 코드에 대해서는 IDE에서도 수정 추천을 해주기 때문에 이를 적극 활용해보는것도 도움이 됩니다
• 리팩토링의 고전이라고 할수 있는 마틴 파울러의 리팩토링, 혹은 켄트벡의 Tidy First 를 짬짬히 읽어보면 좋은 코드를 작성하는데 도움이 될꺼에요
• 다대다 관계에서의 queryDsl 작성에 어려움을 겪었다고 하셨는데, 구체적으로 어떤 부분이 어려웟는지를 같이 말씀 해주시면 더 좋을것 같아요. 다만, 현재 코드에서 어려우셨을걸로 보이는 부분은 left Join을 통해서 join연산을 하고 그 결과를 map을 이용해서 가공하는 저 부분이 어려우셨을것 같은데요. 이런 경우 querryDsl이 어려운게 아니라 쿼리 자체가 어려웠던것은 아닐지 확인 해보면 좋을것 같아요. 혹시 쿼리만으로는 추가 데이터 가공 없이 바로 데이터 추출이 가능 했던것일까요? 그것이 아니라면 DB 설계 자체를 점검해볼 필요가 있습니다. 너무 과도한 정규화는 이처럼 데이터의 사용이 어렵게 되어서, 어느정도 역정규화가 필요 할때도 오거든요