Develop by PatrykWski · Pull Request #1236 · mate-academy/jv-logger

PatrykWski · 2026-04-01T18:06:21Z

No description provided.

mateacademy-ai-mentor

Review Summary

You've made a great start by setting up the logger and configuring the .gitignore file correctly. However, there are a couple of critical requirements that need to be addressed before I can approve your solution.

First, several TODO comments remain in your code, particularly in AuthenticationServiceImpl.java and OrderServiceImpl.java. According to the task requirements, all TODOs must be completed and the comments removed [CHECKLIST ITEM #1].

Second, some log messages are not informative enough. For a log to be useful, it should include important context, like method parameters. For instance, in OrderServiceImpl, methods should log the userId they are operating on. Similarly, error logs should describe the specific error, not just state that a method was called [CHECKLIST ITEM #3].

Please address these points, and your solution will be ready for approval!

✨ Please rate this review in the Platform's notification center¹. 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 button² 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 tag³.
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.

mateacademy-ai-mentor · 2026-04-01T18:09:22Z

+
    @Override
    public Order completeOrder(Long userId) {
        // TODO: add log message about method completeOrder was called


This TODO has not been completed, and the comment has not been removed. A log message should be added here to indicate that the method has been called, including its parameters. This violates both the 'Complete all TODO statements' and 'Remove all TODO comments' requirements.

mateacademy-ai-mentor · 2026-04-01T18:09:22Z

        // NOTE: In production ready code this order identifier should be generated by DB
        // For test purpose we simplify this and return dummy data
        order.setOrderId(1L);
+        logger.info("Method completeOrder was called");


This violates checklist item #3: 'Let's make our log messages informative'. The log message should include the method parameters, in this case, the userId.

mateacademy-ai-mentor · 2026-04-01T18:09:22Z

        Product macBook = new Product("MacBook Air 2020", BigDecimal.valueOf(1399));
        Product xiaomi = new Product("Xiaomi 12", BigDecimal.valueOf(499));
        List<Product> products = List.of(iphone, macBook, xiaomi);
+        logger.info("Successful fetched data from DB");


This log message is not fully informative as required by checklist item #3. Please include the userId to specify for which user the products were fetched.

mateacademy-ai-mentor · 2026-04-01T18:09:22Z

        Product xiaomi = new Product("Xiaomi 12", BigDecimal.valueOf(499));
        List<Product> products = List.of(iphone, macBook, xiaomi);
+        logger.info("Successful fetched data from DB");
        // TODO: add log message about successful fetched data from DB


This TODO comment violates checklist item #1: 'Let's remove all TODO comments before submitting solution.' Since the log message is already present on the line above, this comment is redundant and should be removed.

mateacademy-ai-mentor · 2026-04-01T18:09:22Z

+
    @Override
    public User login(String login, String password) throws AuthenticationException {
        //TODO: add corresponding log message about method login was called


This TODO comment should be removed. According to the task requirements, you need to complete all TODO statements and then remove the comments. A log message should be added here at the beginning of the method to log that the method was called with specific parameters.

mateacademy-ai-mentor · 2026-04-01T18:09:22Z

        User user = findByLogin(login);
        if (!user.getPassword().equals(password)) {
-            throw new AuthenticationException("Username or password are incorrect");
+            logger.error("Login method was called. Params login ={}", login);


This log message is not very informative. It should describe the actual error that occurred, which is a password mismatch. The current message, "Login method was called", is more suitable for an informational log at the beginning of the method. Please make the message more descriptive of the failure.

mateacademy-ai-mentor · 2026-04-01T18:09:22Z

            user = authenticationService.login("bob", "1234");
        } catch (AuthenticationException e) {
-            e.printStackTrace();
+            logger.error("Method login was called, Params login={}", "bob", e);


This log message is intended for an error scenario, but its content, "Method login was called...", is more suitable for a DEBUG or INFO level log at the beginning of a method. An informative error message should clearly state what error occurred. For example: logger.error("Authentication failed for user '{}'", "bob", e);. This aligns with checklist item #3: 'Let's make our log messages informative'.

mateacademy-ai-mentor

Review Summary

Excellent work on improving the logging and cleaning up the TODO comments! You've addressed almost all the feedback from the previous review.

However, there is one critical issue in the login logic that needs to be fixed. In AuthenticationServiceImpl.java, when a user provides an incorrect password, you correctly log an error, but the method then proceeds to return the user, allowing the login to succeed. Instead, you should throw a new AuthenticationException in this case, as the method signature suggests.

This fix is crucial because it will ensure that the try-catch block in Main.java can properly handle failed login attempts.

Once you've made this change, your solution will be perfect. Keep up the great work!

✨ Please rate this review in the Platform's notification center¹. 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 button² 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 tag³.
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.

mateacademy-ai-mentor · 2026-04-01T18:23:39Z

        User user = findByLogin(login);
        if (!user.getPassword().equals(password)) {
-            throw new AuthenticationException("Username or password are incorrect");
+            logger.error("User or password are incorrect. Params login ={}", login);


When the password is incorrect, you log an error message, which is good. However, the method then continues and returns the user object as if the login were successful. The method is declared to throws AuthenticationException, so you should throw a new AuthenticationException here to indicate that the login failed.

mateacademy-ai-mentor · 2026-04-01T18:23:39Z

            user = authenticationService.login("bob", "1234");
        } catch (AuthenticationException e) {
-            e.printStackTrace();
+            logger.error("Authentication service failed. Params login={}", "bob", e);


This error logging correctly handles an AuthenticationException. However, the AuthenticationServiceImpl.login() method currently logs an error for an incorrect password but doesn't throw an exception. This means this catch block will not be executed for a password mismatch, which is a critical bug. Please ensure the login method throws an AuthenticationException as its signature suggests.

mateacademy-ai-mentor

Review Summary

Excellent work on this task! You've done a great job fixing the critical authentication issue from the previous review, and your logging implementation is very well done.

I am approving your solution as an educational override so you can continue progressing, but there are a couple of important points in Main.java to keep in mind for the future:

There is a leftover e.printStackTrace() call. Since you're already logging the exception, this line should be removed to fully meet the task requirements.
The application will crash with a NullPointerException if the login fails. This happens because the user object is null after an exception, but the code proceeds to use it. You can prevent this by adding a check to ensure user is not null before calling orderService.completeOrder().

Overall, you're very close to a perfect solution. Keep up the fantastic effort!

✨ Please rate this review in the Platform's notification center¹. I will be grateful for your feedback! ✨

Rate AI review example ↩

mateacademy-ai-mentor · 2026-04-01T18:39:15Z

        try {
            user = authenticationService.login("bob", "1234");
        } catch (AuthenticationException e) {
            e.printStackTrace();


As per the task requirements, all e.printStackTrace() calls should be replaced with logger methods. You are already logging the exception on the next line, so this line can be safely removed.

mateacademy-ai-mentor · 2026-04-01T18:39:15Z

        }
        OrderService orderService = new OrderServiceImpl();
+        assert user != null;
        orderService.completeOrder(user.getUserId());


There's a potential NullPointerException here. If authenticationService.login() throws an exception, the user object will be null. The code in the catch block will execute, but then the program will continue to this line and fail. You should add logic to prevent this, for example, by returning from the main method within the catch block.

PatrykWski added 3 commits April 1, 2026 19:39

adding lines to pom.xml and making xml folder in resources

7b1fd1c

Loggers were used

ac3dd2b

xd

d9fa9cc

mateacademy-ai-mentor suggested changes Apr 1, 2026

View reviewed changes

CHange some messages

3fc57c6

PatrykWski requested a review from mateacademy-ai-mentor April 1, 2026 18:20

mateacademy-ai-mentor suggested changes Apr 1, 2026

View reviewed changes

PatrykWski added 3 commits April 1, 2026 20:32

adding exceiption

231bd19

exception

e8b880e

xd

805ab39

PatrykWski requested a review from mateacademy-ai-mentor April 1, 2026 18:36

mateacademy-ai-mentor approved these changes Apr 1, 2026

View reviewed changes

Conversation

PatrykWski commented Apr 1, 2026

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants