Conversation
WalkthroughThis update introduces new static factory methods for enum lookup in Changes
Sequence Diagram(s)sequenceDiagram
participant Controller
participant SchoolRegistrationRequest
participant Area
participant Lunch
participant SchoolJpaEntity
Controller->>SchoolRegistrationRequest: Receive registration data
SchoolRegistrationRequest->>Area: Area.from(area)
SchoolRegistrationRequest->>Lunch: Lunch.from(lunch)
SchoolRegistrationRequest->>SchoolJpaEntity: Build entity with lunch, lunchPrice, deadlineTime, etc.
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🪧 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 @wlgns12370, 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 introduces new data fields for school registration, specifically for lunch details and application deadlines, and updates the corresponding API request DTO. It also enhances the robustness of enum conversions by adding utility methods and dedicated error handling. A notable aspect is the temporary omission of address field mapping, pending a business decision.
Highlights
- New School Fields: I've added new fields to the
SchoolJpaEntityto support lunch options, lunch price, and a deadline for school registration. This aligns with the requirement to capture more detailed information for school entries. - API Request Updates: The
SchoolRegistrationRequestDTO has been updated to include the newlunch,lunchPrice, anddeadlineTimefields. ThetoEntity()method now correctly maps these new fields to theSchoolJpaEntity. - Enum Utility Methods & Error Handling: I've introduced
from()static methods to bothLunchandAreaenums. These methods provide a safer way to convert string values to their corresponding enum types, throwing specificCustomRuntimeExceptionerrors (NOT_FOUND_LUNCH,NOT_FOUND_AREA) if an invalid string is provided. Corresponding error codes have been added toErrorCode.java. - Address Field Handling: The
SchoolRegistrationRequestno longer directly maps theschoolAddressfield to theSchoolJpaEntitybuilder. This change reflects the ongoing business decision regarding the handling of school address information, as noted in the PR description.
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 is currently in preview and 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 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 adds new fields for lunch options and deadlines to the school registration process. I've identified a few critical issues where the school's address, though provided in the request, is not being saved. I've also suggested some improvements for validation messages and code efficiency in enum lookups. Addressing these points will significantly improve the robustness and quality of the code.
| public SchoolJpaEntity(String schoolName, Area area, Lunch lunch, LocalDate examDate, | ||
| Long capacity, | ||
| LocalDateTime deadlineTime, Integer lunchPrice) { | ||
| this.schoolName = schoolName; | ||
| this.area = area; | ||
| this.address = address; | ||
| this.lunch = lunch; | ||
| this.examDate = examDate; | ||
| this.capacity = capacity; | ||
| this.deadlineTime = deadlineTime; | ||
| this.lunchPrice = lunchPrice; | ||
| } |
There was a problem hiding this comment.
The address field is declared in the entity but it's no longer being initialized in the constructor. This will result in the address information being lost when a new SchoolJpaEntity is created, even if it's provided in the request.
public SchoolJpaEntity(String schoolName, Area area, Lunch lunch, LocalDate examDate,
Long capacity,
LocalDateTime deadlineTime, Integer lunchPrice, AddressJpaVO address) {
this.schoolName = schoolName;
this.area = area;
this.lunch = lunch;
this.examDate = examDate;
this.capacity = capacity;
this.deadlineTime = deadlineTime;
this.lunchPrice = lunchPrice;
this.address = address;
}| return SchoolJpaEntity.builder() | ||
| .schoolName(schoolName) | ||
| .area(Area.valueOf(area)) | ||
| .address(address) | ||
| .area(Area.from(area)) | ||
| .examDate(examDate) | ||
| .schoolName(schoolName) | ||
| .lunch(Lunch.from(lunch)) | ||
| .lunchPrice(lunchPrice) | ||
| .deadlineTime(deadlineTime) | ||
| .capacity(capacity) | ||
| .build(); |
There was a problem hiding this comment.
The schoolAddress field is a required part of the request DTO, but it is not being used within the toEntity() method. This means the address information will be discarded and not persisted to the database.
You need to convert schoolAddress to AddressJpaVO and set it on the SchoolJpaEntity using the builder.
return SchoolJpaEntity.builder()
.area(Area.from(area))
.address(schoolAddress.toValueObject())
.examDate(examDate)
.schoolName(schoolName)
.lunch(Lunch.from(lunch))
.lunchPrice(lunchPrice)
.deadlineTime(deadlineTime)
.capacity(capacity)
.build()| public static Lunch from(String lunchName) { | ||
| return Arrays.stream(values()) | ||
| .filter(e -> e.getLunchName().equals(lunchName)) | ||
| .findFirst() | ||
| .orElseThrow( | ||
| () -> new CustomRuntimeException(ErrorCode.NOT_FOUND_LUNCH) | ||
| ); | ||
| } |
There was a problem hiding this comment.
For improved performance, use a static Map for looking up enum values from a string. The current implementation iterates through all enum values for every call, which has a time complexity of O(n). Using a pre-populated static Map provides a constant time complexity O(1) lookup.
private static final java.util.Map<String, Lunch> LUNCH_BY_NAME =
java.util.Arrays.stream(values()).collect(
java.util.stream.Collectors.toUnmodifiableMap(Lunch::getLunchName, e -> e));
public static Lunch from(String lunchName) {
Lunch lunch = LUNCH_BY_NAME.get(lunchName);
if (lunch == null) {
throw new CustomRuntimeException(ErrorCode.NOT_FOUND_LUNCH);
}
return lunch;
}| public static Area from(String areaName) { | ||
| return Arrays.stream(Area.values()) | ||
| .filter(area -> area.getAreaName().equals(areaName)) | ||
| .findFirst() | ||
| .orElseThrow(() -> new CustomRuntimeException(ErrorCode.NOT_FOUND_AREA)); | ||
| } |
There was a problem hiding this comment.
Using a static Map for lookups in the from method would be more efficient. While the performance impact is negligible with only three enum constants, it's a good practice for consistency and scalability.
private static final java.util.Map<String, Area> AREA_BY_NAME =
java.util.Arrays.stream(values()).collect(
java.util.stream.Collectors.toUnmodifiableMap(Area::getAreaName, e -> e));
public static Area from(String areaName) {
Area area = AREA_BY_NAME.get(areaName);
if (area == null) {
throw new CustomRuntimeException(ErrorCode.NOT_FOUND_AREA);
}
return area;
}| @Schema(description = "도시락 메뉴") | ||
| @NotNull | ||
| String lunch, | ||
|
|
||
| @Schema(description = "도시락 가격") | ||
| @NotNull | ||
| Integer lunchPrice, | ||
|
|
||
| @NotNull | ||
| @Schema(description = "신청 마감 일시", example = "2025-05-30T10:00:00") | ||
| LocalDateTime deadlineTime, |
There was a problem hiding this comment.
The new fields lunch, lunchPrice, and deadlineTime are annotated with @NotNull, but they are missing the message attribute. Providing a descriptive error message is important for client-side error handling and provides a better user experience.
Additionally, lunchPrice should probably be a positive number. Consider adding a @Positive annotation.
| @Schema(description = "도시락 메뉴") | |
| @NotNull | |
| String lunch, | |
| @Schema(description = "도시락 가격") | |
| @NotNull | |
| Integer lunchPrice, | |
| @NotNull | |
| @Schema(description = "신청 마감 일시", example = "2025-05-30T10:00:00") | |
| LocalDateTime deadlineTime, | |
| @NotNull(message = "도시락 메뉴는 필수입니다.") | |
| String lunch, | |
| @Schema(description = "도시락 가격") | |
| @NotNull(message = "도시락 가격은 필수입니다.") | |
| @Positive(message = "도시락 가격은 양수여야 합니다.") | |
| Integer lunchPrice, | |
| @NotNull(message = "신청 마감 일시는 필수입니다.") | |
| @Schema(description = "신청 마감 일시", example = "2025-05-30T10:00:00") | |
| LocalDateTime deadlineTime, |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/java/life/mosu/mosuserver/domain/school/Area.java (1)
15-17: Redundant explicit private constructor.Enum constructors are implicitly private, making the explicit
privatemodifier redundant. Consider removing it for cleaner code.- private Area(String areaName) { + Area(String areaName) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/main/java/life/mosu/mosuserver/domain/application/Lunch.java(2 hunks)src/main/java/life/mosu/mosuserver/domain/school/Area.java(1 hunks)src/main/java/life/mosu/mosuserver/domain/school/SchoolJpaEntity.java(3 hunks)src/main/java/life/mosu/mosuserver/global/exception/ErrorCode.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/school/dto/SchoolRegistrationRequest.java(1 hunks)
🔇 Additional comments (9)
src/main/java/life/mosu/mosuserver/domain/application/Lunch.java (1)
22-29: Well-implemented enum factory method.The static factory method follows good practices with proper error handling and clear exception messaging. The implementation is consistent with the Area enum's similar method.
src/main/java/life/mosu/mosuserver/global/exception/ErrorCode.java (1)
66-68: Appropriate error codes added.The new enum-related error codes are well-categorized and use appropriate HTTP status codes. The Korean error messages are consistent with the existing codebase style.
src/main/java/life/mosu/mosuserver/domain/school/Area.java (1)
19-24: Consistent enum factory method implementation.The static factory method implementation is consistent with the Lunch enum and follows good practices with proper error handling.
src/main/java/life/mosu/mosuserver/domain/school/SchoolJpaEntity.java (2)
41-48: New lunch and deadline fields properly implemented.The new fields are correctly annotated with appropriate JPA annotations (@Enumerated for the enum, @column for the others). The field types and naming are consistent with the domain model.
57-67: Constructor updated with new fields.The constructor correctly includes the new lunch, lunchPrice, and deadlineTime fields. The field assignments are properly implemented.
src/main/java/life/mosu/mosuserver/presentation/school/dto/SchoolRegistrationRequest.java (4)
34-44: New lunch and deadline fields properly validated.The new fields have appropriate validation annotations (@NotNull, @notblank) and clear schema descriptions. The field types are consistent with the entity model.
54-60: Proper use of enum factory methods.The
toEntity()method correctly uses the newArea.from()andLunch.from()methods, which provide better error handling than the previousvalueOf()approach.
16-49: Ignore field reordering – no explicit serialization order detectedI couldn’t find any
@JsonPropertyOrderannotations or Jackson configuration that enforces declaration-order serialization for SchoolRegistrationRequest. Jackson falls back to alphabetical ordering by default, so movingschoolNameafterexamDatewill not affect JSON output.Likely an incorrect or invalid review comment.
52-62: Confirm Address Mapping IntentThe
SchoolJpaEntitystill declares a privateAddressJpaVO address(line 36) but its constructor and Lombok-generated builder no longer accept or set this field—sotoEntity()can’t map the address and it’ll remainnull. Please verify whether this omission is intentional or if address assignment needs to be reintroduced (e.g., add a builder setter or handle it separately in the service layer).• Domain entity:
– File:src/main/java/life/mosu/mosuserver/domain/school/SchoolJpaEntity.java
– Line 36:private AddressJpaVO address;
– Constructor signature at lines 57–63 has noaddressparameter and noaddress(...)builder method.
• DTO mapping:
– File:src/main/java/life/mosu/mosuserver/presentation/school/dto/SchoolRegistrationRequest.java
– Lines 52–62:toEntity()omits anyaddressconversion.Please confirm and, if needed, restore address mapping or adjust the design accordingly.
| @Embedded | ||
| private AddressJpaVO address; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Address field not included in constructor.
The address field is still present in the entity but removed from the constructor parameters. This could lead to entities being created with null addresses. Verify if this is intentional or if the address should be included in the constructor.
🏁 Script executed:
#!/bin/bash
# Check if AddressJpaVO is used elsewhere in the codebase and how it's typically initialized
rg -A 5 "AddressJpaVO" --type javaLength of output: 10540
🏁 Script executed:
#!/bin/bash
# Display class declaration, annotations, constructor, and builder for SchoolJpaEntity
sed -n '1,200p' src/main/java/life/mosu/mosuserver/domain/school/SchoolJpaEntity.javaLength of output: 1865
Include address in the SchoolJpaEntity @builder constructor
The address field is declared but omitted from the builder constructor, which prevents calls like SchoolJpaEntity.builder().address(...) (used in DatabaseInitializer) and leaves address uninitialized.
• File: src/main/java/life/mosu/mosuserver/domain/school/SchoolJpaEntity.java
– The @Builder constructor (around line Thirty-nine) only takes (String schoolName, Area area, Lunch lunch, LocalDate examDate, Long capacity, LocalDateTime deadlineTime, Integer lunchPrice).
Please update the constructor to include AddressJpaVO address and assign it:
@Builder
-public SchoolJpaEntity(String schoolName,
- Area area,
- Lunch lunch,
- LocalDate examDate,
- Long capacity,
- LocalDateTime deadlineTime,
- Integer lunchPrice) {
- this.schoolName = schoolName;
- this.area = area;
- this.lunch = lunch;
- this.examDate = examDate;
- this.capacity = capacity;
- this.deadlineTime = deadlineTime;
- this.lunchPrice = lunchPrice;
-}
+public SchoolJpaEntity(String schoolName,
+ AddressJpaVO address,
+ Area area,
+ Lunch lunch,
+ LocalDate examDate,
+ Long capacity,
+ LocalDateTime deadlineTime,
+ Integer lunchPrice) {
+ this.schoolName = schoolName;
+ this.address = address;
+ this.area = area;
+ this.lunch = lunch;
+ this.examDate = examDate;
+ this.capacity = capacity;
+ this.deadlineTime = deadlineTime;
+ this.lunchPrice = lunchPrice;
+}🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/domain/school/SchoolJpaEntity.java around
lines 35 to 39, the address field is declared but missing from the @Builder
constructor parameters and assignment. Update the @Builder constructor to
include AddressJpaVO address as a parameter and assign it to this.address to
enable builder calls like SchoolJpaEntity.builder().address(...).
Docstrings generation was requested by @toothlessdev. * #95 (comment) The following files were modified: * `src/main/java/life/mosu/mosuserver/domain/application/Lunch.java` * `src/main/java/life/mosu/mosuserver/domain/school/Area.java` * `src/main/java/life/mosu/mosuserver/domain/school/SchoolJpaEntity.java` * `src/main/java/life/mosu/mosuserver/presentation/school/dto/SchoolRegistrationRequest.java`
|
Note Generated docstrings for this pull request at #100 |
1 participant
✨ 구현한 기능
📢 논의하고 싶은 내용
🎸 기타
Summary by CodeRabbit
New Features
Improvements