Skip to content
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

1차 스프린트 리팩토링을 진행한다. #154

Closed
wants to merge 7 commits into from

Conversation

JINU-CHANG
Copy link
Contributor

@JINU-CHANG JINU-CHANG commented Jul 28, 2024

❗ Issue

✨ 구현한 기능

  • Question이 cateogoryId를 가지고 있도록 변경했습니다.
  • 서비스내 readUserChecklistsPreview(), readChecklistsComparison() 메서드의 코드 가독성을 향상시켰습니다.
  • ChecklistScore 도메인을 생성해 점수 계산에 대한 책임을 부여했습니다.

📢 논의하고 싶은 내용

  • 체크리스트 생성시 사용자가 답변하지 않은 질문에 대해 null로 저장하는지/저장하지 않는지 논의 필요
  • ChecklistQuestion이 Question을 가지도록 구현할 수 있는지에 대한 논의 필요
  • 뱃지 부여 기준 8점으로 변경하는 것에 대한 논의 필요
  • ChecklistQuestion 양방향 설정

🎸 기타

  • 작업하지 않은 API에 대한 테스트코드 깨지는 것들은 주석처리해두었습니다.
  • DTO 내 타입 Grade인지 확인 필요
  • 스키마 변경 사항 secret으로 등록 필요
  • 테스트 코드 작성 방식 통일 필요

Comment on lines 52 to 53
public Badge provideBadge(Questionlist questionlist, List<ChecklistQuestion> questions) {
int categoryScore = calculateCategoryScore(this, questionlist, questions);
Copy link
Contributor

Choose a reason for hiding this comment

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

이부분은 static import를 안쓰는 게 더 좋은 것 같아 보여요!

Comment on lines 19 to 30
public static int calculateCategoryScore(Category category, Questionlist questionlist, List<ChecklistQuestion> questions) {
List<ChecklistQuestion> filteredQuestions = questionlist.filterQuestions(category, questions);

if (filteredQuestions.isEmpty()) {
return 0;
}

int maxScore = Grade.calculateMaxScore(filteredQuestions.size());
int totalScore = Grade.calculateTotalScore(filteredQuestions);

return totalScore * 10 / maxScore;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

점수 계산하는 두 메서드에서 중복을 제거할 수 있을 것 같아 보여요

@@ -109,8 +114,8 @@ private void createChecklistQuestions(ChecklistCreateRequest checklistCreateRequ
List<QuestionCreateRequest> questions = checklistCreateRequest.questions();
validateQuestion(questions);
for (QuestionCreateRequest questionCreateRequest : questions) {
Integer questionId = questionCreateRequest.questionId();
ChecklistQuestion checklistQuestion = new ChecklistQuestion(checklist, questionId,
Question question = questionList.findById(questionCreateRequest.questionId());
Copy link
Contributor

Choose a reason for hiding this comment

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

메서드 내에서 사용되는 부분이 없어 보입니다

Copy link
Contributor

@tkdgur0906 tkdgur0906 left a comment

Choose a reason for hiding this comment

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

리팩토링 하느라 고생하셨어요!
회의끝나고 마무리되면 approve 할게요~

import com.bang_ggood.category.domain.Category;
import java.util.List;

public class ChecklistScore {
Copy link
Contributor

Choose a reason for hiding this comment

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

점수에 대한 로직을 다른 도메인으로 분리하는 것은 좋은 것 같네요!

.mapToInt(CategoryScoreReadResponse::score)
.sum() / categoryScores.size();
private int getChecklistScore(Checklist checklist) {
return calculateTotalScore(checklist.getQuestions());
Copy link
Contributor

Choose a reason for hiding this comment

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

static import를 사용하지 않는 것이 더 명확할 것 같아요!

@shin-jisong shin-jisong reopened this Jul 30, 2024
@healim01 healim01 deleted the refactor/130-1-sprint branch August 12, 2024 06:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants