done by chupa-ilona · Pull Request #1234 · mate-academy/jv-logger

chupa-ilona · 2026-03-26T17:59:36Z

No description provided.

mateacademy-ai-mentor

Review Summary

Good job setting up the logger and configuring the .gitignore file. However, this requires changes before it can be approved. The main issues are that several explicit checklist requirements were missed, and the build is failing.

Here's what you need to fix:

Remove TODO comments: All TODO comments must be removed from AuthenticationServiceImpl.java and OrderServiceImpl.java before submission. [CHECKLIST ITEM #1]
Make log messages informative: The log messages should include method parameters to be useful for debugging. For example, in the login method, change the log to something like logger.info("Method login was called. Params: login={}", login);. [CHECKLIST ITEM #3]
Fix Checkstyle issues: The build is failing due to style violations. Please make the logger fields private in both service classes and add a blank line after their declarations.

Once these issues are addressed, your solution will be ready for another review.

✨ 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-03-26T18:01:10Z

+    Logger logger = getLogger(AuthenticationServiceImpl.class);
    @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 checklist item #1, all TODO comments must be removed before submitting the solution.

mateacademy-ai-mentor · 2026-03-26T18:01:10Z

    @Override
    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");


This log message is not informative enough. As per checklist item #3, it's better to include method parameters to provide more context. For example: logger.info("Method login was called. Params: login={}", login);.

mateacademy-ai-mentor · 2026-03-26T18:01:11Z

+    Logger logger = LogManager.getLogger();
    @Override
    public Order completeOrder(Long userId) {
        // TODO: add log message about method completeOrder was called


This TODO comment should be removed as per checklist item #1: 'Let's remove all TODO comments before submitting solution.'

mateacademy-ai-mentor · 2026-03-26T18:01:11Z

    @Override
    public Order completeOrder(Long userId) {
        // TODO: add log message about method completeOrder was called
+        logger.info("Method completeOrder was called");


This log message violates checklist item #3: 'Let's make our log messages informative'. It should include the method parameters to be more useful. For example: logger.info("Method completeOrder was called for userId={}", userId);

mateacademy-ai-mentor · 2026-03-26T18:01:11Z

@@ -25,6 +29,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


This TODO comment should be removed as required by checklist item #1.

mateacademy-ai-mentor · 2026-03-26T18:01:11Z

        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("Successfully fetched data from DB");


This log message is not informative enough, which violates checklist item #3. Please include the userId to provide more context about which user's data was fetched.

mateacademy-ai-mentor

Review Summary

Great job addressing most of the previous feedback, like making the loggers private and removing the TODOs! We're very close to the finish line, but a few changes are still needed to meet all the requirements.

The main issue is that the log messages still use string concatenation. Please update them to use parameterized placeholders ({}) as required by the checklist [CHECKLIST ITEM #3]. For example, logger.info("Method completeOrder was called with userId: " + userId) should be logger.info("Method completeOrder was called with userId: {}", userId);.

Additionally, please fix the code style issues: add a blank line after the logger field declaration in the service classes and ensure there's an empty line at the end of the .gitignore file [CHECKLIST ITEM #2]. The build is also failing due to an unused import in AuthenticationServiceImpl.java that needs to be removed.

Once these points are addressed, your solution will be ready!

✨ 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-03-27T09:08:07Z

+import org.apache.logging.log4j.Logger;

 public class OrderServiceImpl implements OrderService {
+    private Logger logger = LogManager.getLogger(OrderServiceImpl.class);


There should be a blank line after this field declaration to improve code readability. This was also mentioned in the previous review feedback.

mateacademy-ai-mentor · 2026-03-27T09:08:07Z

    @Override
    public Order completeOrder(Long userId) {
-        // TODO: add log message about method completeOrder was called
+        logger.info("Method completeOrder was called with userId: " + userId + "");


This violates checklist item #3: 'Let's make our log messages informative'. It's better to use parameterized logging for performance and readability, like this:

logger.info("Method completeOrder was called with userId: {}", userId);

mateacademy-ai-mentor · 2026-03-27T09:08:07Z

        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("Successfully fetched data from DB");


This violates checklist item #3: 'Let's make our log messages informative'. This log message would be more useful if it included the userId for which the products are being fetched. For example: logger.info("Successfully fetched data from DB for userId: {}", userId);

mateacademy-ai-mentor · 2026-03-27T09:08:07Z

-        //TODO: add corresponding log message about method login was called
+        logger.info("Method login was called with login: " + login + "");
        User user = findByLogin(login);
        if (!user.getPassword().equals(password)) {


This is an improvement, but it's still not quite right. According to checklist item #3, you should use parameterized logging for better performance and readability. Instead of string concatenation, use placeholders like this: logger.info("Method login was called. Params: login={}", login);

mateacademy-ai-mentor

Review Summary

Excellent work on this task! You've done a fantastic job addressing the previous feedback, especially with updating all log messages to use parameterized placeholders. I am approving your solution so you can continue progressing. There is just one minor style issue to keep in mind for future tasks: please ensure all files end with a single empty line. In this case, AuthenticationServiceImpl.java and OrderServiceImpl.java were missing it. This is a common convention that improves code consistency [CHECKLIST ITEM #2]. Keep up the great work!

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

Rate AI review example ↩

done

c98c302

mateacademy-ai-mentor suggested changes Mar 26, 2026

View reviewed changes

done

8152ab6

chupa-ilona requested a review from mateacademy-ai-mentor March 27, 2026 09:06

done

38c50a0

mateacademy-ai-mentor suggested changes Mar 27, 2026

View reviewed changes

done

8a1d438

chupa-ilona requested a review from mateacademy-ai-mentor March 27, 2026 09:11

mateacademy-ai-mentor approved these changes Mar 27, 2026

View reviewed changes

Conversation

chupa-ilona commented Mar 26, 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!

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!

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants