Added Logger#1238
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job setting up the logger and replacing the print statements! However, some changes are needed before this can be approved. There is a critical bug in the AuthenticationServiceImpl where the login method doesn't throw an AuthenticationException on a password mismatch. This allows users to log in with incorrect credentials and must be fixed.
Additionally, please address these points from the task requirements:
- Remove all
TODOcomments from your code. [CHECKLIST ITEM #1] - Make your log messages more informative by including relevant context, like a
userIdor the exception details. [CHECKLIST ITEM #3]
Once you've addressed these points, your solution will be ready for another review!
✨ 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
| User user; | ||
| try { | ||
| user = authenticationService.login("bob", "1234"); | ||
| } catch (AuthenticationException e) { |
There was a problem hiding this comment.
This log message isn't very informative. Per checklist item #3, it's better to include context, such as the username for which the login failed. Also, it's a good practice to pass the exception object e as a second argument to logger.error() to log the stack trace.
|
|
||
| @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.' Please remove this comment now that you've implemented the logging.
| @@ -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.' Please remove this comment now that you've implemented the logging.
| 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 | ||
| logger.info("Successful fetched data from DB"); |
There was a problem hiding this comment.
This violates checklist item #3: 'Let's make our log messages informative'. This log message is not very informative. It would be better to include the userId to specify for which user the data was fetched.
| if (!user.getPassword().equals(password)) { | ||
| throw new AuthenticationException("Username or password are incorrect"); | ||
| logger.warn("Username or password are incorrect for user: {}", login); | ||
| } |
There was a problem hiding this comment.
When the password doesn't match, you should throw an AuthenticationException as indicated by the method signature. Currently, the code only logs a warning and then proceeds to return the user, which would allow a successful login with an incorrect password.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on fixing the critical authentication bug and improving the logging throughout the application! You're very close to completing the task. However, one change is needed in src/main/java/mate/academy/Main.java.
The current error log message in the catch block, logger.error("Wrong password");, isn't informative enough as it hides the actual exception details. This goes against the requirement to make log messages informative [CHECKLIST ITEM #3].
To fix this, please include the exception object in your log call, like this: logger.error("Authentication failed", e);. This will ensure the full stack trace is logged, which is crucial for debugging. Once you've made that small adjustment, the solution will be perfect!
✨ 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
| user = authenticationService.login("bob", "1234"); | ||
| } catch (AuthenticationException e) { | ||
| e.printStackTrace(); | ||
| logger.error("Wrong password"); |
There was a problem hiding this comment.
This log message isn't very informative. It's a good practice to include the exception details in the log to make debugging easier. This violates checklist item #3: 'Let's make our log messages informative'.
You could pass the exception object e as the second argument to the logger method, like logger.error("Authentication failed", e); or include its message like logger.error("Authentication failed: {}", e.getMessage());.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work! I am approving your solution. You've done a fantastic job addressing the feedback from the previous review. Your logging is now implemented perfectly across the project, with informative messages that include relevant parameters. The error handling in Main.java is also correctly implemented to log the full exception stack trace. Keep up the great work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
No description provided.