Conversation
# Conflicts: # src/main/java/life/mosu/mosuserver/application/exam/ExamService.java
…o refactor/mosu-130
|
Caution Review failedThe pull request is closed. WalkthroughThis update introduces several new records and processor classes to modularize and streamline application processing, validation, and data assembly in the application service layer. It refactors Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ApplicationService
participant ApplicationValidator
participant RegisterApplicationStepProcessor
participant SaveExamTicketStepProcessor
participant ApplicationContext
User->>ApplicationService: apply(userId, ApplicationRequest)
ApplicationService->>ApplicationValidator: RequestNoDuplicateExams, ExamIdsAndLunchSelection, NoDuplicateApplication
ApplicationService->>RegisterApplicationStepProcessor: process(RegisterApplicationCommand)
RegisterApplicationStepProcessor-->>ApplicationService: RegisterApplicationCommand
ApplicationService->>SaveExamTicketStepProcessor: process(ApplicationProcessingContext)
SaveExamTicketStepProcessor-->>ApplicationService: ApplicationProcessingContext
ApplicationService-->>User: CreateApplicationResponse
User->>ApplicationService: getApplications(userId)
ApplicationService->>ApplicationContext: new ApplicationContext(...)
ApplicationContext->>ApplicationContext: fetchExams(fetcher)
ApplicationContext->>ApplicationContext: fetchSubjects(fetcher)
ApplicationContext->>ApplicationContext: fetchPayments(fetcher)
ApplicationContext->>ApplicationService: assemble()
ApplicationService-->>User: List<ApplicationResponse>
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (22)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @chominju02, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request focuses on a significant refactoring of the application (신청) module to enhance code organization, readability, and maintainability. It achieves this by extracting responsibilities into specialized components for validation, multi-step processing, and data assembly, thereby simplifying the main application service logic.
Highlights
- Refactoring Application Service Logic: The core
ApplicationServicehas been significantly refactored. Complex validation logic has been extracted into a newApplicationValidatorcomponent, and multi-step processing (like registering exam applications and saving exam tickets) has been delegated to dedicatedStepProcessorimplementations (RegisterApplicationStepProcessor,SaveExamTicketStepProcessor). This greatly simplifies theapplymethod. - Streamlined Data Assembly with ApplicationContext: A new
ApplicationContextrecord has been introduced to centralize and simplify the retrieval and assembly of application-related data for responses. ThegetApplicationsmethod inApplicationServicenow leverages this context object to fetch exams, subjects, and payments, and then assemble the finalApplicationResponselist, replacing a large amount of previously inline and fragmented logic. - Generalization of StepProcessor Interface: The
StepProcessorinterface, previously located within theauth.processorpackage, has been moved to a more generalglobal.processorpackage. This indicates its intended use as a reusable pattern across different application domains. - Improved Subject Handling and Validation: The
Subjectenum now includes a staticgetSubjectmethod for robust conversion from string names, and a newEXAM_DUPLICATEDerror code has been added toErrorCodefor better validation of application requests.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request is a great refactoring of the application submission logic. It significantly improves the code's structure by separating concerns into a validator, step processors, and a context object, making the ApplicationService much cleaner. I've identified some areas for improvement, mainly concerning leftover code from the refactoring, a package name typo, and a minor inconsistency. Addressing these points will make this a very strong contribution.
| package life.mosu.mosuserver.presentation.a; | ||
|
|
||
| import java.util.ArrayList; | ||
| import java.util.List; | ||
| import java.util.stream.Stream; | ||
|
|
||
| class Item { | ||
|
|
||
| } | ||
|
|
||
| class Cart { | ||
|
|
||
| private final List<Item> items = new ArrayList<>(); | ||
| Stream | ||
| } | ||
|
|
||
| public class Test { | ||
|
|
||
| List<Item> items; | ||
| Cart cart; | ||
| } |
There was a problem hiding this comment.
This file appears to be temporary scratch code that was committed by mistake. It's located in the src/main/java source tree, is in a temporary package presentation.a, and contains non-compiling code. Please remove this file from the pull request.
| public class SaveApplicationStepProcessor implements StepProcessor<ApplicationProcessingContext> { | ||
|
|
||
| @Override | ||
| public ApplicationProcessingContext process(ApplicationProcessingContext request) { | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
This class appears to be either incomplete or dead code. The process method returns null, and the class is not used anywhere in the changes. It should be removed if it's not part of a feature being developed in this PR to avoid cluttering the codebase.
| @FunctionalInterface | ||
| public interface IdStream extends Function<ExamSubjectJpaEntity, Long> { | ||
|
|
||
| Long apply(ExamSubjectJpaEntity examSubject); | ||
|
|
||
| } |
There was a problem hiding this comment.
This functional interface IdStream is not used anywhere in the codebase. It appears to be dead code and should be removed to keep the codebase clean.
| @@ -0,0 +1,69 @@ | |||
| package life.mosu.mosuserver.application.application.vaildator; | |||
There was a problem hiding this comment.
The package name life.mosu.mosuserver.application.application.vaildator contains a typo. It should be validator.
This should be corrected by renaming the directory and updating all related import statements (e.g., in ApplicationService.java) to improve maintainability and prevent confusion.
| package life.mosu.mosuserver.application.application.vaildator; | |
| package life.mosu.mosuserver.application.application.validator; |
| } | ||
|
|
||
| private Map.Entry<Long, ExamApplicationResponse> createExamApplicationResponse( | ||
| ExamApplicationJpaEntity examApp) throws RuntimeException { |
There was a problem hiding this comment.
The throws RuntimeException clause is redundant here. RuntimeException and its subclasses are unchecked exceptions, so you don't need to declare them in the method signature. Removing it will make the code slightly cleaner.
private Map.Entry<Long, ExamApplicationResponse> createExamApplicationResponse(
ExamApplicationJpaEntity examApp) {| @@ -0,0 +1,43 @@ | |||
| package life.mosu.mosuserver.application.application; | |||
|
|
|||
| import jakarta.transaction.Transactional; | |||
There was a problem hiding this comment.
This class uses jakarta.transaction.Transactional, while other services like ApplicationService use org.springframework.transaction.annotation.Transactional. To ensure consistent transaction management behavior across the application, it's recommended to use Spring's annotation throughout. Please switch to org.springframework.transaction.annotation.Transactional.
| import jakarta.transaction.Transactional; | |
| import org.springframework.transaction.annotation.Transactional; |
| Long applicationId = context.applicationId(); | ||
| FileRequest fileReq = context.fileRequest(); | ||
|
|
||
| if (fileReq.fileName() != null && fileReq.s3Key() != null) { |
There was a problem hiding this comment.
The condition fileReq.fileName() != null && fileReq.s3Key() != null can be simplified by adding a helper method to the FileRequest record, for example isValid(). This would improve readability and encapsulate the logic within the FileRequest object itself.
| // Set<Subject> subjectSet; | ||
| // try { | ||
| // subjectSet = subjects.stream() | ||
| // .map(Subject::valueOf) | ||
| // .collect(Collectors.toSet()); | ||
| // } catch (IllegalArgumentException e) { | ||
| // throw new CustomRuntimeException(ErrorCode.WRONG_SUBJECT_TYPE); | ||
| // } | ||
| // | ||
| // if (subjectSet.size() != 2) { | ||
| // throw new CustomRuntimeException(ErrorCode.WRONG_SUBJECT_COUNT); | ||
| // } | ||
| // | ||
| // return subjectSet; | ||
| // } |
There was a problem hiding this comment.
This large block of commented-out code should be removed. Leaving commented-out code in the codebase can cause confusion and reduces maintainability. If this logic is needed for reference, it should be available in the version control history.
1 participant
✨ 구현한 기능
📢 논의하고 싶은 내용
🎸 기타
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Style
Documentation