✨ Feature/#29 - 4.6 온기 공유하기 API 구현 #38
Conversation
WalkthroughThe changes introduce a new API endpoint for sharing Onjung information, specifically a PUT request to Changes
Assessment against linked issues
Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (10)
src/main/java/com/daon/onjung/onjung/application/usecase/CreateOrUpdateShareUseCase.java (2)
7-10: Add Javadoc documentation for better maintainability.The interface structure looks good, but adding documentation would improve maintainability and make the contract clearer for implementers.
Consider adding documentation like this:
@UseCase +/** + * Use case for creating or updating share counts for Onjung stores. + */ public interface CreateOrUpdateShareUseCase { + /** + * Creates or updates a share for a specific store by an account. + * + * @param accountId The UUID of the account sharing the store + * @param storeId The ID of the store being shared + * @return true if the share was successfully created/updated, false otherwise + */ Boolean execute(UUID accountId, Long storeId); }
9-9: Consider using a more detailed response type.Using a Boolean return type might not provide enough context about why an operation failed. This could make error handling and debugging more difficult.
Consider creating a dedicated response type that can include:
- Success/failure status
- Reason for failure
- Current share count (if successful)
Example:
public record ShareResult( boolean success, String errorMessage, Integer currentShareCount ) { public static ShareResult success(int count) { return new ShareResult(true, null, count); } public static ShareResult failure(String message) { return new ShareResult(false, message, null); } }Then update the interface:
-Boolean execute(UUID accountId, Long storeId); +ShareResult execute(UUID accountId, Long storeId);src/main/java/com/daon/onjung/onjung/domain/service/ShareService.java (1)
13-21: Consider translating comments to EnglishThe Korean comments might make it difficult for international team members to understand the code.
Consider:
- // 해당 가게에 대해 공유한 적이 없는 경우 + // Case: No previous share exists for this store if (share == null) { share = Share.builder() .user(user) .store(store) .build(); - } else { // 해당 가게에 대해 공유한 적이 있는 경우 + } else { // Case: Updating existing share counthttp/onjung/OnjungControllerHttpRequest.http (1)
41-44: LGTM! Consider enhancing the documentation.The endpoint follows RESTful conventions and maintains consistency with other endpoints. However, the documentation could be more comprehensive.
Consider adding:
### 4.6 온기 공유하기 // @no-log PUT {{host_url}}/api/v1/stores/{{onjung.API_4_6.id}}/shares Authorization: Bearer {{access_token}} + +### Example Response +```json +{ + // Add expected response format here +} +```src/main/java/com/daon/onjung/onjung/application/service/CreateOrUpdateShareService.java (3)
19-28: Add class-level documentation and organize dependencies.Consider the following improvements:
- Add Javadoc explaining the service's purpose and responsibilities
- Group related dependencies (repositories) together
Apply this diff:
@Service @RequiredArgsConstructor +/** + * Service responsible for managing the creation and update of share information + * between users and stores. This service ensures transactional consistency + * when handling share operations. + */ public class CreateOrUpdateShareService implements CreateOrUpdateShareUseCase { private final UserRepository userRepository; private final StoreRepository storeRepository; private final ShareRepository shareRepository; - private final ShareService shareService;
41-49: Consider potential race conditions in share creation/update.The current implementation might face race conditions when multiple users try to share simultaneously. Consider using optimistic locking or version control.
Apply this diff to add optimistic locking:
// 공유 정보 조회 Share share = shareRepository.findByUserAndStore(user, store) .orElse(null); Boolean isCreated = (share == null); + // Add retry logic for optimistic locking + int maxRetries = 3; + int attempts = 0; + while (attempts < maxRetries) { + try { // 공유 정보 생성 또는 Count 1 추가 share = shareService.createOrUpdateShare(share, user, store); shareRepository.save(share); + break; + } catch (org.springframework.orm.ObjectOptimisticLockingFailureException e) { + attempts++; + if (attempts == maxRetries) { + throw new CommonException(ErrorCode.CONCURRENT_MODIFICATION); + } + share = shareRepository.findByUserAndStore(user, store).orElse(null); + } + }
33-33: Translate comments to English for consistency.Consider translating the Korean comments to English for better maintainability and collaboration.
Apply this diff:
- // 유저 조회 + // Find user - // 가게 조회 + // Find store - // 공유 정보 조회 + // Find share information - // 공유 정보 생성 또는 Count 1 추가 + // Create new share or increment countAlso applies to: 37-37, 41-41, 46-46
src/main/java/com/daon/onjung/onjung/application/controller/command/OnjungCommandV1Controller.java (2)
47-49: Enhance method documentationThe current documentation only states the feature number and name. Consider adding:
- Expected behavior for new vs existing shares
- Success/failure conditions
- Return status code explanations
Example:
/** * 4.6 온기 공유하기 * * Creates or updates a share for the specified store. * Returns 201 for new shares and 200 for updates. * * @param accountId The ID of the account sharing * @param id The store ID to share * @return Empty response with appropriate status */
50-59: Add validation and API documentationThe endpoint implementation looks good, but consider these improvements:
- Add validation for the path variable
- Add API documentation annotations for OpenAPI/Swagger
Consider applying this enhancement:
+ @Operation(summary = "Share warmth for a store", description = "Creates or updates a share for the specified store") + @ApiResponses(value = { + @ApiResponse(responseCode = "201", description = "Share created"), + @ApiResponse(responseCode = "200", description = "Share updated"), + @ApiResponse(responseCode = "404", description = "Store not found") + }) @PutMapping("/api/v1/stores/{id}/shares") public ResponseDto<Void> shareOnjung( @AccountID UUID accountId, - @PathVariable Long id + @PathVariable @Positive(message = "Store ID must be positive") Long id ) {src/main/java/com/daon/onjung/onjung/domain/service/OnjungService.java (1)
Update calculateTotalOnjungCount to consider Share count field
The verification reveals that
calculateTotalOnjungCountneeds to be updated. WhilecalculateTotalOnjungAmountcorrectly usesshare.getCount()for calculations,calculateTotalOnjungCountsimply usesgetShares().size(), which doesn't account for the count field in Share objects.
calculateTotalOnjungCountshould be modified to sum up the count values from Share objects, similar to how it's done incalculateTotalOnjungAmount- Current implementation in
calculateTotalOnjungCount:onjung.getShares().size()- Suggested implementation: Add a stream operation to sum Share counts
🔗 Analysis chain
Line range hint
24-33: Verify share counting in calculateTotalOnjungCountThe
calculateTotalOnjungCountmethod currently counts each share as 1, but with the introduction of thecountfield inShare, should this method be updated to consider the count value as well?Let's check how Share count is used in other parts of the codebase:
Let me gather more context about the Share class and its usage in calculations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for Share class definition and usage ast-grep --pattern 'class Share { $$$ }' # Search for references to Share.getCount() rg -A 3 'share.*getCount\(\)'Length of output: 502
Script:
#!/bin/bash # Search for Share class definition ast-grep --pattern 'class Share { $$$ }' # Search for all methods in OnjungService that might use Share rg -A 10 'calculateTotal|calculate.*Count' src/main/java/com/daon/onjung/onjung/domain/service/OnjungService.java # Search for Share-related calculations rg -B 2 -A 5 'shares.*stream\(\)' src/main/java/com/daon/onjung/onjung/domain/service/OnjungService.javaLength of output: 1922
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
http/onjung/OnjungControllerHttpRequest.http(1 hunks)src/main/java/com/daon/onjung/onjung/application/controller/command/OnjungCommandV1Controller.java(3 hunks)src/main/java/com/daon/onjung/onjung/application/service/CreateOrUpdateShareService.java(1 hunks)src/main/java/com/daon/onjung/onjung/application/usecase/CreateOrUpdateShareUseCase.java(1 hunks)src/main/java/com/daon/onjung/onjung/domain/Share.java(2 hunks)src/main/java/com/daon/onjung/onjung/domain/service/OnjungService.java(1 hunks)src/main/java/com/daon/onjung/onjung/domain/service/ShareService.java(1 hunks)src/main/java/com/daon/onjung/onjung/repository/mysql/ShareRepository.java(1 hunks)src/main/java/com/daon/onjung/security/config/SecurityConfig.java(1 hunks)
🔇 Additional comments (10)
src/main/java/com/daon/onjung/onjung/repository/mysql/ShareRepository.java (1)
3-3: LGTM! Clean import additions
The new imports for Store and Optional are necessary and properly used by the new repository method.
Also applies to: 9-9
src/main/java/com/daon/onjung/onjung/domain/service/ShareService.java (2)
15-18: LGTM! Clean builder pattern usage
The builder pattern is appropriately used for creating the Share entity with required associations.
20-20: Verify share count increment logic
The addCount() method is called but its implementation isn't visible in the current context.
Let's verify the implementation:
✅ Verification successful
Share count increment implementation is correct and thread-safe
The addCount() method implementation is straightforward and correct:
- The count field is properly initialized to 1 in the constructor
- The increment operation is a simple increment of the count field
- The field is properly defined as a non-nullable Integer column
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the Share class implementation to verify addCount() method
ast-grep --pattern 'class Share {
$$$
addCount() {
$$$
}
$$$
}'
Length of output: 77
Script:
#!/bin/bash
# Let's try a broader search for Share class and its methods
rg -l "class Share"
Length of output: 146
Script:
#!/bin/bash
# Search for addCount method implementation
rg "addCount" -A 3
Length of output: 648
Script:
#!/bin/bash
# Try finding Share class files using fd
fd -e java Share
Length of output: 396
Script:
#!/bin/bash
# Let's check the Share class implementation to understand the count field
rg "class Share" -A 10 src/main/java/com/daon/onjung/onjung/domain/Share.java
Length of output: 516
Script:
#!/bin/bash
# Check if there are any tests for Share class
fd -e java test.*Share
Length of output: 26
Script:
#!/bin/bash
# Let's see the full Share class implementation including the count field
rg -U "class Share" -A 50 src/main/java/com/daon/onjung/onjung/domain/Share.java
Length of output: 1669
Script:
#!/bin/bash
# Check for any test files related to Share
fd -e java Share.*test
Length of output: 26
http/onjung/OnjungControllerHttpRequest.http (1)
43-43: Verify the endpoint behavior.
Let's verify if this endpoint is properly configured in the security settings and if it's accessible.
✅ Verification successful
Let me gather more information about the security configuration and implementation details.
Let me check the security configuration and authentication requirements for this endpoint.
Endpoint is properly secured and configured
The endpoint is properly configured with security measures:
- It requires authentication through JWT (JsonWebTokenAuthenticationFilter)
- Uses
@AccountIDannotation for user identification - Falls under protected URLs requiring USER/OWNER role
- Implements proper authorization checks through the security filter chain
- Returns appropriate response codes (201 for creation, 200 for update)
The HTTP request in the test file correctly includes the Authorization header with Bearer token, matching the security requirements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check security configuration and endpoint implementation
# Check security configuration for the endpoint
rg -A 5 "/api/v1/stores/.+/shares"
# Check controller implementation
ast-grep --pattern 'class $_ {
$$$
@$_("/api/v1/stores/{$_}/shares")
$$$
}'
Length of output: 1138
Script:
#!/bin/bash
# Check security configuration and implementation details
# Check for security annotations on the controller class and method
ast-grep --pattern 'class $_ {
$$$
@PutMapping("/api/v1/stores/{id}/shares")
public $_ shareOnjung($$$) {
$$$
}
}'
# Check for any security configuration related to this endpoint
rg -B 5 "@AccountID"
# Look for any security configuration classes
rg -l "SecurityConfig|WebSecurityConfig"
Length of output: 4594
Script:
#!/bin/bash
# Check security configuration details
# Check SecurityConfig implementation
cat src/main/java/com/daon/onjung/security/config/SecurityConfig.java
# Check AccountID annotation implementation
rg -g "*.java" -A 5 "public @interface AccountID"
# Check if there's any specific authorization check for this endpoint
rg -g "*.java" "createOrUpdateShareUseCase" -A 5
Length of output: 7265
src/main/java/com/daon/onjung/onjung/domain/Share.java (2)
29-30: LGTM! Well-structured entity with proper initialization.
The Share entity is well-designed with:
- Proper nullable constraints
- Clear field organization with descriptive comments
- Correct initialization in the constructor
- Appropriate use of the Builder pattern
Also applies to: 53-58
47-47: Consider keeping timestamp precision for audit purposes.
The change from LocalDateTime to LocalDate results in loss of time precision. This might impact audit trails and analytics that require exact timing of share events.
- private LocalDate createdAt;
+ private LocalDateTime createdAt;src/main/java/com/daon/onjung/onjung/application/controller/command/OnjungCommandV1Controller.java (2)
7-7: LGTM!
The new dependency injection and import changes are well-structured and follow the existing patterns in the codebase.
Also applies to: 12-12, 23-23
50-54: Verify security configuration
Ensure this endpoint is properly secured in the security configuration.
✅ Verification successful
Let me check the security configuration to verify how this endpoint is secured.
Let me check the Constants class to see the URL patterns that define the security rules.
Based on the security configuration and constants, I can now provide the final response:
Endpoint is properly secured
The /api/v1/stores/{id}/shares endpoint is adequately protected because:
- It falls under
/api/v1/**pattern which requires authentication with roles "USER" or "OWNER" as configured in SecurityConfig - JWT authentication is enforced via JsonWebTokenAuthenticationFilter
- The @accountid annotation ensures the user's identity is properly validated
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for security configuration related to this endpoint
rg -l "SecurityFilterChain|WebSecurityConfigurerAdapter|SecurityConfiguration" --type java
# Search for specific endpoint security rules
rg "/api/v1/stores/[^/]+/shares" --type java
Length of output: 350
Script:
#!/bin/bash
# Check the security configuration content
cat src/main/java/com/daon/onjung/security/config/SecurityConfig.java
# Also check for any custom annotations and their implementations
rg -g '*.java' -A 5 '@AccountID'
Length of output: 9756
Script:
#!/bin/bash
# Check the Constants class for URL patterns
cat src/main/java/com/daon/onjung/core/constant/Constants.java
# Also check for the @AccountID annotation implementation
rg -g '*.java' -B 5 'public @interface AccountID'
Length of output: 2012
src/main/java/com/daon/onjung/security/config/SecurityConfig.java (2)
77-77: LGTM! Verify client applications for the updated logout endpoint.
The change to include /api prefix in the logout endpoint maintains consistency with the login endpoint.
Let's verify the Constants class to ensure the security configuration URLs are updated:
#!/bin/bash
# Description: Check if Constants.java has been updated with new URL patterns
# Expected: NO_NEED_AUTH_URLS and USER_URLS should use /api prefix
# Search for URL constants
rg -t java 'NO_NEED_AUTH_URLS|USER_URLS' -A 1069-69: LGTM! Verify client applications for the updated login endpoint.
The change to include /api prefix in the login endpoint aligns with REST API best practices.
Let's verify if all auth-related endpoints follow this pattern:
✅ Verification successful
The previous search revealed that auth endpoints in Constants.java follow the /api/v1 pattern. Let's verify all other endpoints to ensure complete consistency across the codebase.
Let's try with proper quote handling to find any inconsistent endpoint patterns.
Based on the previous results, we found that:
- The Constants.java file shows all auth endpoints consistently using
/api/v1prefix - We found several REST controllers but need to verify their endpoint mappings
LGTM! All endpoints consistently use the /api/v1 prefix.
Based on the verification results:
- Auth endpoints in Constants.java follow
/api/v1pattern - AuthController uses
/api/v1/authprefix - OnjungQueryV1Controller uses
/api/v1/onjungsprefix - OnjungCommandV1Controller uses
/api/v1prefix - The login endpoint change aligns with the consistent pattern across the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any inconsistent auth endpoint patterns
# Expected: All auth endpoints should have /api prefix
# Search for auth-related endpoint mappings
rg -t java '@(Get|Post|Put|Delete|Request)Mapping.*["\'].*auth.*["\']'
# Search for potential auth-related configuration
rg -t java 'auth.*[Uu]rl.*=.*["\'].*["\']'
Length of output: 3796
Script:
#!/bin/bash
# Let's search for all endpoint mappings to verify consistency
rg -t java '@(Get|Post|Put|Delete|Request)Mapping.*["\'].*v1.*["\']' -A 1
# Also check for any controller classes that might define base paths
rg -t java '@RestController.*|@RequestMapping.*["\'].*v1.*["\']' -A 1
Length of output: 93
Script:
#!/bin/bash
# Search for endpoint mappings with fixed quotes
rg -t java "@(Get|Post|Put|Delete|Request)Mapping.*'/.*v1.*'" -A 1
# Search for controller base paths with fixed quotes
rg -t java "(@RestController|@RequestMapping.*'/.*v1.*')" -A 1
# Search for any remaining endpoints without /api prefix
rg -t java "@(Get|Post|Put|Delete|Request)Mapping.*'/v1.*'" -A 1
Length of output: 1409
Script:
#!/bin/bash
# Let's check the content of these controllers for their endpoint mappings
echo "=== AuthController ==="
rg -A 10 "class AuthController" src/main/java/com/daon/onjung/security/application/controller/AuthController.java
echo "=== OnjungQueryV1Controller ==="
rg -A 10 "class OnjungQueryV1Controller" src/main/java/com/daon/onjung/onjung/application/controller/query/OnjungQueryV1Controller.java
echo "=== OnjungCommandV1Controller ==="
rg -A 10 "class OnjungCommandV1Controller" src/main/java/com/daon/onjung/onjung/application/controller/command/OnjungCommandV1Controller.java
Length of output: 1865
|
|
||
| public interface ShareRepository extends JpaRepository <Share, Long> { | ||
|
|
||
| List<Share> findAllByUser(User user); | ||
|
|
||
| Optional<Share> findByUserAndStore(User user, Store store); |
There was a problem hiding this comment.
💡 Codebase verification
Optional handling needs improvement in CreateOrUpdateShareService
The method usage in CreateOrUpdateShareService could be improved:
- Current:
.orElse(null)defeats the purpose of using Optional by reverting to null checks - Recommended: Consider using
.orElseGet(Share::new)or handle empty case explicitly with.isPresent()
🔗 Analysis chain
LGTM! Well-designed repository method
The new findByUserAndStore method follows good practices:
- Uses Spring Data JPA naming conventions for automatic query generation
- Returns
Optional<Share>for proper null handling - Method signature aligns well with the warm sharing feature requirements
Let's verify the method usage pattern:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how the new method is being used in the service layer
# Expect: Usage in CreateOrUpdateShareService with proper Optional handling
# Search for method usage
rg -l "findByUserAndStore" --type java
# Check the implementation pattern
ast-grep --pattern 'findByUserAndStore($_, $_)'
Length of output: 252
Script:
#!/bin/bash
# Let's check how the method is used in CreateOrUpdateShareService
rg "findByUserAndStore" -B 2 -A 2 src/main/java/com/daon/onjung/onjung/application/service/CreateOrUpdateShareService.java
Length of output: 289
| public Share createOrUpdateShare(Share share, User user, Store store) { | ||
|
|
||
| // 해당 가게에 대해 공유한 적이 없는 경우 | ||
| if (share == null) { | ||
| share = Share.builder() | ||
| .user(user) | ||
| .store(store) | ||
| .build(); | ||
| } else { // 해당 가게에 대해 공유한 적이 있는 경우 | ||
| share.addCount(); | ||
| } | ||
| return share; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add @transactional annotation
The method modifies entity state but lacks transaction management.
Add the annotation:
+ @Transactional
public Share createOrUpdateShare(Share share, User user, Store store) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public Share createOrUpdateShare(Share share, User user, Store store) { | |
| // 해당 가게에 대해 공유한 적이 없는 경우 | |
| if (share == null) { | |
| share = Share.builder() | |
| .user(user) | |
| .store(store) | |
| .build(); | |
| } else { // 해당 가게에 대해 공유한 적이 있는 경우 | |
| share.addCount(); | |
| } | |
| return share; | |
| } | |
| @Transactional | |
| public Share createOrUpdateShare(Share share, User user, Store store) { | |
| // 해당 가게에 대해 공유한 적이 없는 경우 | |
| if (share == null) { | |
| share = Share.builder() | |
| .user(user) | |
| .store(store) | |
| .build(); | |
| } else { // 해당 가게에 대해 공유한 적이 있는 경우 | |
| share.addCount(); | |
| } | |
| return share; | |
| } |
🛠️ Refactor suggestion
Add method documentation and parameter validation
The method lacks JavaDoc documentation explaining its contract and parameter validation for user and store.
Consider adding:
+ /**
+ * Creates a new share or updates the count of an existing share.
+ *
+ * @param share The existing share record or null if no prior share exists
+ * @param user The user sharing the store (must not be null)
+ * @param store The store being shared (must not be null)
+ * @return The created or updated Share entity
+ * @throws IllegalArgumentException if user or store is null
+ */
public Share createOrUpdateShare(Share share, User user, Store store) {
+ if (user == null || store == null) {
+ throw new IllegalArgumentException("User and store must not be null");
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public Share createOrUpdateShare(Share share, User user, Store store) { | |
| // 해당 가게에 대해 공유한 적이 없는 경우 | |
| if (share == null) { | |
| share = Share.builder() | |
| .user(user) | |
| .store(store) | |
| .build(); | |
| } else { // 해당 가게에 대해 공유한 적이 있는 경우 | |
| share.addCount(); | |
| } | |
| return share; | |
| } | |
| /** | |
| * Creates a new share or updates the count of an existing share. | |
| * | |
| * @param share The existing share record or null if no prior share exists | |
| * @param user The user sharing the store (must not be null) | |
| * @param store The store being shared (must not be null) | |
| * @return The created or updated Share entity | |
| * @throws IllegalArgumentException if user or store is null | |
| */ | |
| public Share createOrUpdateShare(Share share, User user, Store store) { | |
| if (user == null || store == null) { | |
| throw new IllegalArgumentException("User and store must not be null"); | |
| } | |
| // 해당 가게에 대해 공유한 적이 없는 경우 | |
| if (share == null) { | |
| share = Share.builder() | |
| .user(user) | |
| .store(store) | |
| .build(); | |
| } else { // 해당 가게에 대해 공유한 적이 있는 경우 | |
| share.addCount(); | |
| } | |
| return share; | |
| } |
| public void addCount() { | ||
| this.count++; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add bounds checking and consider thread safety.
The addCount method has potential issues:
- No upper bound check could lead to integer overflow
- Non-atomic increment operation could cause race conditions in concurrent scenarios
Consider this safer implementation:
- public void addCount() {
- this.count++;
+ public synchronized void addCount() {
+ if (this.count == Integer.MAX_VALUE) {
+ throw new IllegalStateException("Share count has reached maximum value");
+ }
+ this.count++;
}Alternatively, consider using @Version for optimistic locking:
@Version
private Long version;| User user = userRepository.findById(accountId) | ||
| .orElseThrow(() -> new CommonException(ErrorCode.NOT_FOUND_RESOURCE)); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use specific error codes for different scenarios.
Currently using the same generic NOT_FOUND_RESOURCE error code for both user and store not found scenarios. This makes it harder to handle these cases differently in the client.
Apply this diff:
User user = userRepository.findById(accountId)
- .orElseThrow(() -> new CommonException(ErrorCode.NOT_FOUND_RESOURCE));
+ .orElseThrow(() -> new CommonException(ErrorCode.USER_NOT_FOUND));
Store store = storeRepository.findById(storeId)
- .orElseThrow(() -> new CommonException(ErrorCode.NOT_FOUND_RESOURCE));
+ .orElseThrow(() -> new CommonException(ErrorCode.STORE_NOT_FOUND));Also applies to: 38-39
| @Override | ||
| @Transactional | ||
| public Boolean execute(UUID accountId, Long storeId) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add parameter validation.
The method parameters should be validated to prevent null values or invalid inputs.
Apply this diff:
@Override
@Transactional
- public Boolean execute(UUID accountId, Long storeId) {
+ public Boolean execute(@jakarta.validation.constraints.NotNull UUID accountId,
+ @jakarta.validation.constraints.NotNull Long storeId) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Override | |
| @Transactional | |
| public Boolean execute(UUID accountId, Long storeId) { | |
| @Override | |
| @Transactional | |
| public Boolean execute(@jakarta.validation.constraints.NotNull UUID accountId, | |
| @jakarta.validation.constraints.NotNull Long storeId) { |
| onjung.getShares().stream() | ||
| .map(share -> { | ||
| return share.getCount() * 100; | ||
| }) | ||
| .reduce(0, Integer::sum); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance code maintainability and robustness
A few suggestions to improve the code:
- Extract the magic number
100as a constant to improve maintainability and document its purpose - Simplify the stream operation by removing unnecessary block and return
- Add null safety check for
getCount()
Here's the suggested implementation:
+ private static final int SHARE_AMOUNT_MULTIPLIER = 100; // Amount added per share count
+
public Integer calculateTotalOnjungAmount(Onjung onjung) {
return onjung.getDonations().stream()
.map(Donation::getDonationAmount)
.reduce(0, Integer::sum) +
onjung.getReceipts().stream()
.map(Receipt::getPaymentAmount)
.reduce(0, Integer::sum) +
onjung.getShares().stream()
- .map(share -> {
- return share.getCount() * 100;
- })
+ .map(share -> share.getCount() != null ? share.getCount() * SHARE_AMOUNT_MULTIPLIER : 0)
.reduce(0, Integer::sum);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| onjung.getShares().stream() | |
| .map(share -> { | |
| return share.getCount() * 100; | |
| }) | |
| .reduce(0, Integer::sum); | |
| private static final int SHARE_AMOUNT_MULTIPLIER = 100; // Amount added per share count | |
| onjung.getShares().stream() | |
| .map(share -> share.getCount() != null ? share.getCount() * SHARE_AMOUNT_MULTIPLIER : 0) | |
| .reduce(0, Integer::sum); |
Related issue 🛠
closed #29
어떤 변경사항이 있었나요?
CheckPoint ✅
PR이 다음 요구 사항을 충족하는지 확인하세요.
Work Description ✏️
Uncompleted Tasks 😅
N/A
To Reviewers 📢
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Refactor