for check - #1261
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there are still critical checklist violations that are explicitly required by the task. The main blocking issue is that multiple // TODO comments remain even though the functionality has been implemented, specifically in AuthenticationServiceImpl.login and in OrderServiceImpl.completeOrder and OrderServiceImpl.getAllProductsFromShoppingCart [CHECKLIST ITEM #1]; per the task, all TODO comments must be removed before submission. Another required item is that every file must end with a trailing newline; please ensure .gitignore, Main.java, AuthenticationServiceImpl.java, and OrderServiceImpl.java all have a newline at the very end [CHECKLIST ITEM #2]. To fix this, simply delete those TODO comment lines (do not change the existing logging logic) and save each file with a final empty line in your editor.
Apart from these points, your use of Log4j, replacement of System.out.println/printStackTrace, and generally informative log messages are all solid and align well with the task requirements. Once you remove the remaining TODOs and add the trailing newlines, your solution will be ready to be accepted.
✨ 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
| import org.apache.logging.log4j.Logger; | ||
|
|
||
| public class Main { | ||
| private static final Logger logger = LogManager.getLogger(Main.class); |
There was a problem hiding this comment.
This violates checklist item #1: Let's remove all TODO comments before submitting solution. You’ve already implemented the logging, so this TODO comment should be deleted.
| public class Main { | ||
| private static final Logger logger = LogManager.getLogger(Main.class); | ||
|
|
||
| public static void main(String[] args) { |
There was a problem hiding this comment.
This violates checklist item #1: Let's remove all TODO comments before submitting solution. The log line is already present, so this TODO comment should be removed.
|
|
||
| @Override | ||
| public User login(String login, String password) throws AuthenticationException { | ||
| //TODO: add corresponding log message about method login was called |
There was a problem hiding this comment.
This violates checklist item: "Let's remove all TODO comments before submitting solution." The logging is already implemented, so this TODO comment should be deleted.
| public User login(String login, String password) throws AuthenticationException { | ||
| //TODO: add corresponding log message about method login was called | ||
| logger.info("Method login was called. Params: login {}", login); | ||
| User user = findByLogin(login); |
There was a problem hiding this comment.
This violates checklist item: "Let's remove all TODO comments before submitting solution." The logging is already implemented, so this TODO comment should be deleted.
| public class OrderServiceImpl implements OrderService { | ||
| private static final Logger logger = LogManager.getLogger(OrderServiceImpl.class); | ||
|
|
||
| @Override |
There was a problem hiding this comment.
This violates checklist item #1: Let's remove all TODO comments before submitting solution. You’ve already added the log message, so this TODO comment should be removed.
|
|
||
| @Override | ||
| public Order completeOrder(Long userId) { | ||
| // TODO: add log message about method completeOrder was called |
There was a problem hiding this comment.
This violates checklist item #1: Let's remove all TODO comments before submitting solution. The logging is implemented, so this TODO comment should be removed.
| @@ -25,6 +30,7 @@ private List<Product> getAllProductsFromShoppingCart(Long userId) { | |||
| Product xiaomi = new Product("Xiaomi 12", BigDecimal.valueOf(499)); | |||
| List<Product> products = List.of(iphone, macBook, xiaomi); | |||
| // TODO: add log message about successful fetched data from DB | |||
There was a problem hiding this comment.
This violates checklist item #1: Let's remove all TODO comments before submitting solution. The log message is already added, so this TODO comment should be removed.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your solution is approved because it fully meets the task requirements and checklist items without any critical blockers. You correctly configured Log4j2 with both console and file appenders, and all former System.out.println and e.printStackTrace usages have been replaced with appropriate logger calls. The log messages in Main, AuthenticationServiceImpl, and OrderServiceImpl are informative, including useful context such as method flow and parameters where relevant. All TODO comments have been removed [CHECKLIST ITEM #1], and each file ends with a trailing newline [CHECKLIST ITEM #2]. The .gitignore file properly ignores the logs directory, preventing log files from being committed. Overall, the implementation is clean, consistent, and aligned with the logging and configuration requirements for this task. Nicely done applying structured logging throughout the project.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
No description provided.