Add implementation#241
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job on the overall structure and use of CompletableFuture and ConcurrentHashMap! Your solution is very close, but it requires a couple of important fixes to meet the task requirements.
First, in AsyncRequestProcessor.java, the custom Executor provided to the constructor is not being used. Your asynchronous tasks are running on the default thread pool instead of the one specified in Main. You can fix this by passing the executor as the second argument to CompletableFuture.supplyAsync.
Second, the output for UserData doesn't match the example. It should contain user-specific details (e.g., "Details for user1") instead of the generic "Some details...". Please update the UserData creation to make the details string dynamic based on the userId.
Address these points, and your solution will be ready!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Good effort on the overall structure for caching and asynchronous processing. However, a couple of critical requirements are not yet met, so I am requesting changes.
- The custom
Executoris not being used. TheCompletableFuture.supplyAsync()method is being called without passing theexecutorinstance. This causes the task to run on the default thread pool, not the one you're supposed to use. Please pass theexecutoras the second argument tosupplyAsync. - User data is hardcoded. The
UserDataobject is created with a generic string. The output needs to be user-specific, like"Details for user1". You should generate this string dynamically using theuserIdparameter.
Please address these two points to complete the task.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| return CompletableFuture.supplyAsync(() -> { | ||
| try { | ||
| Thread.sleep(1000); | ||
| } catch (InterruptedException e) { | ||
| throw new RuntimeException(e); | ||
| } | ||
| UserData userData = new UserData(userId, "Some details..."); | ||
| cache.put(userId, userData); | ||
| return userData; | ||
| }); |
There was a problem hiding this comment.
The custom executor that's passed into the constructor isn't being used here. The supplyAsync method has an overload that accepts an Executor as a second argument. Using it will ensure the task runs on the intended thread pool.
| } catch (InterruptedException e) { | ||
| throw new RuntimeException(e); | ||
| } | ||
| UserData userData = new UserData(userId, "Some details..."); |
There was a problem hiding this comment.
The expected output requires user-specific details (e.g., "Details for user1"). This line currently uses a generic, hardcoded string. The details should be generated dynamically based on the userId.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on this task! You've successfully implemented the asynchronous processing with a custom executor and caching, which are the core concepts here.
I am approving your solution so you can continue learning, but please note one final adjustment is needed. In src/main/java/mate/academy/AsyncRequestProcessor.java, the details string you generate is "Some details for user " + userId. The requirement specifies the format "Details for " + userId. Please adjust this to match the expected output precisely.
You've done a great job addressing the previous feedback. Keep up the fantastic effort!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| } catch (InterruptedException e) { | ||
| throw new RuntimeException(e); | ||
| } | ||
| UserData userData = new UserData(userId, "Some details for user " + userId); |
There was a problem hiding this comment.
This is a good step toward making the details dynamic. However, the format of this string doesn't match the one specified in the task's expected output. The requirement shows details=Details for user1, but your current implementation will produce details=Some details for user user1. Please adjust the string to match the required format.
2 participants
No description provided.