-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[FEAT] 가까워지기 API 구현 #137
[FEAT] 가까워지기 API 구현 #137
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
복잡한 구현도 뚝딱뚝딱 빠르게 해내시는군요!! 역시. 수고하셨슴다 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
네이밍 좋군요!
default Optional<CloserQuestion> findRandomExceptIds(List<Long> ids) { | ||
Random random = new Random(); | ||
List<CloserQuestion> allQuestions = findAll(); | ||
|
||
if (allQuestions.isEmpty()) { | ||
throw new CustomException(ErrorType.NOT_FOUND_CLOSER_QUESTION); | ||
} | ||
if (ids.isEmpty()) { | ||
int randomIndex = random.nextInt(allQuestions.size()); | ||
return Optional.ofNullable(allQuestions.get(randomIndex)); | ||
} | ||
|
||
List<CloserQuestion> filteredQuestions = allQuestions.stream() | ||
.filter(question -> !ids.contains(question.getId())) | ||
.collect(Collectors.toList()); | ||
|
||
if (filteredQuestions.isEmpty()) { | ||
return Optional.empty(); | ||
} | ||
|
||
int randomIndex = random.nextInt(filteredQuestions.size()); | ||
return Optional.of(filteredQuestions.get(randomIndex)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Service Layer에서 처리하는 대신 Repository Layer의 default 메서드로 정의하신 이유가 따로 있을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
기존의 Question을 랜덤으로 뽑아오는 메서드가 Repository 계층에 존재했었기 때문에 이렇게 구현했던 것인데요,
고민해보니, 로직 자체는 Service 계층에 더 어울리는 것 같습니다!
그런데 해당 메서드는 QnAService와 CloserService에서 모두 가져다 쓰고 있습니다
(처음 온보딩을 마칠 때 QnAService에서 호출하며, 다음 가까워지기로 넘어갈때 CloserService에서 호출)
따라서 Service 계층에서 Service 계층을 주입받아 사용하는 것보다는, 해당 메서드를 Repository 계층으로 가져가는 방식을 택했습니다.
혹시 이부분에 대해서 어떻게 생각하시나요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
흠 단순히 테이블에서 가져온 데이터를 필터링하는 로직이고, 여러 Service에서 사용되니 코드의 중복을 막기 위해서는 Repository에 정의하는 게 좋은 방법인 것 같네요!!
일단 지금은 다른 부분과의 통일성을 위해 이렇게 해두고, 다음에 계층에 대한 리팩토링을 진행할 때 Service-Repository 레이어의 명확한 분리와 의존관계를 위해 서비스 구현체로 빼고 JpaRepository와는 분리하는 것도 좋을 것 같습니다!
// 가까워지기 QnA 리스트도 초기화 | ||
parentchild.initCloserQna(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Parentchild의 closerQnAList 필드를 final로 선언하여 바로 ArrayList 객체를 생성해주는 대신, 여기서 CloserQnA 리스트를 초기화하신 이유가 궁금합니당
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 부분도 역시 기존의 qnAList 구현과 통일성 있게 가져가려고 하다보니 이렇게 구현하였습니다!
말씀해주신 방식이 더욱 좋은 방식이라고 합니다! 이유는 아래와 같습니다.
- 초기화: 필드를 선언하는 동시에 초기화하므로 initCloserQna() 메서드를 호출할 필요가 없습니다.
- 불변성: final 키워드를 사용하여 필드를 선언하면 해당 필드는 변경될 수 없습니다. 이는 코드의 안정성과 유지보수성을 높여줍니다.
- 코드의 간결성: 필드를 선언하는 동시에 초기화하는 것이 코드를 더 간결하게 만듭니다.
따라서 기존의 qnaList와 closerQnaList 모두 final 키워드를 사용하도록 변경했습니다 😎
📌 관련 이슈
closed #136
✨ 어떤 이유로 변경된 내용인지
GET
오늘의 가까워지기 화면 불러오기PATCH
가까워지기 질문에 답변하기PATCH
결과창에서 다음으로 버튼 누르기🙏 검토 혹은 리뷰어에게 남기고 싶은 말