added logger to project by 3xactCoder · Pull Request #1263 · mate-academy/jv-logger

3xactCoder · 2026-07-07T16:15:50Z

No description provided.

mateacademy-ai-mentor

Review Summary

I am requesting changes because some core requirements and explicit checklist items are still not satisfied, so the task is not yet complete. You still need to add a proper logging configuration with both File and Console appenders (e.g., via logback.xml or log4j2.xml), as required by the task description, so that logs go to both console and a file under an ignored directory like log/. There are remaining TODO comments that must be removed after implementation, specifically in Main.java (the TODO about adding logging), and in OrderServiceImpl where the log-related TODOs are implemented but the // TODO lines are still present, violating [CHECKLIST ITEM #1]. In AuthenticationServiceImpl, you also need to remove the two TODO comments that were used as placeholders for log messages now that those logs exist, and fix the malformed log message that concatenates strings and placeholders instead of using a single parameterized format string. Finally, Main currently catches an exception from authentication and immediately throws a RuntimeException without logging; you should add a logger to Main and log the authentication failure (including the exception) before rethrowing, to meet the project-wide logging requirement and improve observability. Once you address these critical issues, your logging solution will align with the task requirements and checklist, and the rest of your logging work (message content, .gitignore updates, and absence of System.out.println/printStackTrace) looks good and on the right track.

✨ 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-07-07T16:17:35Z

@@ -14,8 +14,7 @@ public static void main(String[] args) {
        try {
            user = authenticationService.login("bob", "1234");
        } catch (AuthenticationException e) {


This violates checklist item #1: Let's remove all TODO comments before submitting solution. You still have a TODO comment here; implement the logging as required and then remove the TODO line.

mateacademy-ai-mentor · 2026-07-07T16:17:35Z

+            User user = new User(login, "1234");
+            user.setUserId(2L);
+            return user;
+        }


This violates checklist item #1: "Let's remove all TODO comments before submitting solution." The comment // TODO: add log message about successful fetched data from DB should be removed now that the log statement is present.

mateacademy-ai-mentor · 2026-07-07T16:17:35Z

+        return null;
    }


The log message concatenates strings and parameters incorrectly: "Successfully fetched data from DB for user Id:" + " {}. Retracted {} products." with userId and products.size() passed as arguments. This will not substitute the placeholders correctly and also makes the message less clear. This conflicts with checklist item #3: "Let's make our log messages informative" (compare against the "Good example" which uses a single format string with placeholders). Consider using a single format string like logger.info("Successfully fetched data from DB for user Id: {}. Retrieved {} products.", userId, products.size());.

mateacademy-ai-mentor · 2026-07-07T16:17:35Z

+
+    private static final Logger logger = LoggerFactory.getLogger(AuthenticationServiceImpl.class);
+
    @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 for user: {}", login);
        User user = findByLogin(login);
-        if (!user.getPassword().equals(password)) {
+


The task requires: "Configure the logger in this project. Add appenders for File and Console" and "If you have System.out.println() then replace them all with corresponding logger methods" plus "Replace all e.printStackTrace(); with corresponding logger methods." While this file has no System.out.println or printStackTrace, it also does not use a logger at all, and in the catch block you immediately throw a RuntimeException without logging the failure. Consider adding a logger to Main and logging the authentication failure (including the exception) before handling or rethrowing to align with the project-wide logging requirement and make the error easier to trace.

mateacademy-ai-mentor · 2026-07-07T16:17:35Z

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


This violates checklist item: "Let's remove all TODO comments before submitting solution." You’ve added the log call, but the TODO comment itself still needs to be removed.

mateacademy-ai-mentor · 2026-07-07T16:17:35Z

@@ -25,6 +31,8 @@ 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 violates checklist item: "Let's remove all TODO comments before submitting solution." You added a log message, but the TODO comment is still present and should be deleted.

mateacademy-ai-mentor

Review Summary

I am requesting changes because there is still a critical functional issue in the authentication flow and some logging requirements are not fully met. In AuthenticationServiceImpl.login, the method currently logs a successful login even when credentials are invalid or the user is null, and it does not throw AuthenticationException on failure; this breaks the core authentication behavior and must be fixed so that failed logins are logged (with the exception) and then result in an exception rather than a “success” log. Additionally, the failure log in this method should include the exception object and use a parameterized message, for example logger.error("Authentication failed for user {}", login, e);, to improve observability and align with the logging checklist.

To get this approved, please:

Adjust AuthenticationServiceImpl.login so that invalid credentials lead to an AuthenticationException, and only log a success message after you have confirmed the user and password are correct.
Update the failure logging in AuthenticationServiceImpl to include the exception and use parameter placeholders instead of hard-coded values or concatenation.
Clean up the minor log message issues noted (trailing space in the message ending with {} and improve parameterization/spacing in the “method completeOrder was called” style logs) so the output is tidy and consistent.
Overall, your logging setup and use of SLF4J look solid, and once you correct the authentication control flow and refine these log messages, the solution will fully meet the task requirements.

✨ 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-07-07T16:53:35Z

 package mate.academy.service;

+import java.util.Objects;
 import mate.academy.exception.AuthenticationException;


The task requires a proper logger configuration with both File and Console appenders so logs go to console and a file under an ignored directory like log/. There is no such configuration file (e.g., logback.xml or log4j2.xml) in the repo, which violates the core requirement: "Configure the logger in this project. Add appenders for File and Console."

mateacademy-ai-mentor · 2026-07-07T16:53:35Z

        return user;
    }


When logging the authentication failure, you are not including the exception e in the log call, which makes debugging harder. Also, the message is slightly misspelled and not parameterized. To improve observability and align with the project-wide logging requirement, log the failure with the exception (e.g., logger.error("Authentication failed for user {}", "bob", e);) before throwing, instead of a plain message without the cause.

mateacademy-ai-mentor · 2026-07-07T16:53:35Z

+        if (user == null || !Objects.equals(user.getPassword(), password)) {
+            logger.warn("Failed login attempt for user: {}", login);
        }
+
+        logger.info("User {} successfully logged in.", login);


This method currently never throws AuthenticationException when login fails and will log "User ... successfully logged in" even when user is null or the password is wrong. That breaks the intended authentication behavior: on invalid credentials, it should log the failed attempt and then throw AuthenticationException instead of proceeding to success logging and returning null. Adjust the control flow so that failed login attempts lead to an exception and skip the success log.

mateacademy-ai-mentor · 2026-07-07T16:53:35Z

    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 for user: {}", login);
        User user = findByLogin(login);


This log message is informative but could better follow the checklist guidance about parameterized messages. Also, you might want a small space after the comma for readability: logger.info("Method completeOrder was called for user Id: {}", userId);. More importantly, consider whether you want to include additional context (e.g., number of products) to make the log more useful.

mateacademy-ai-mentor · 2026-07-07T16:53:35Z

+        if ("bob".equals(login)) {
+            User user = new User(login, "1234");
+            user.setUserId(2L);
+            return user;


This message has a trailing space inside the format string ("... {}. "), which is slightly inconsistent and unnecessary. While minor, cleaning it to "Successfully fetched data from DB for user with Id: {}." will keep logs tidy and more professional.

mateacademy-ai-mentor

Review Summary

I am approving your solution as an educational override so you can move forward while still learning from the remaining points below.

You did a solid job configuring SLF4J with console and file appenders, replacing System.out.println/printStackTrace, and fixing the authentication flow so AuthenticationException is thrown correctly and success is logged only after validation. The main improvements to focus on next are around logging: in AuthenticationServiceImpl.login and in Main.main, the failure logs should include the exception object and use parameter placeholders, e.g. logger.error("Authentication failed for user {}", login, e); so the stack trace and user are clearly visible [CHECKLIST ITEM #3]. In Main, your info/error logs currently hard-code bob; instead, use a login variable with placeholders like logger.info("User {} tries to login", login); to keep logs informative and reusable. In OrderServiceImpl, there are a few minor formatting issues (missing space after a comma in "{}",userId and a trailing space inside one message) and it’s good practice to tidy those up for clean, consistent logs.

Overall, you’re very close—your logging structure and authentication logic are correct, and with these small refinements you’ll fully align with the checklist and build strong logging habits.

✨ 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-07-07T17:05:19Z

    public static void main(String[] args) {
+
        AuthenticationService authenticationService = new AuthenticationServiceImpl();
        User user;


The failure log in login currently logs only a message and the login parameter but not the exception; this violates the requirement to "Update the failure logging in AuthenticationServiceImpl to include the exception and use parameter placeholders" (see example logger.error("Authentication failed for user {}", login, e);). Consider creating the AuthenticationException in a variable and passing it as the third argument to logger.error.

mateacademy-ai-mentor · 2026-07-07T17:05:19Z

        User user;
        try {
            user = authenticationService.login("bob", "1234");
+            logger.info("User bob tries to login");


This log message hard-codes bob instead of using a parameterized placeholder and a login variable, which goes against checklist item #3 about making log messages informative and parameterized; consider logging something like logger.info("User {} tries to login", login);.

mateacademy-ai-mentor · 2026-07-07T17:05:19Z

        } catch (AuthenticationException e) {
-            e.printStackTrace();
-            return;
+            logger.error("Authentification failed for user bob");


Similarly to the info log above, this error message hard-codes bob and does not leverage parameterization as suggested in checklist item #3; using placeholders and a login variable would make this log more informative and reusable.

mateacademy-ai-mentor · 2026-07-07T17:05:19Z

+
+    private static final Logger logger = LoggerFactory.getLogger(Main.class);
+
    public static void main(String[] args) {


The log message has no space after the comma in {},userId, and previous review feedback asked to tidy up such minor formatting issues; adding the space ({}, userId) will make the output cleaner and consistent.

mateacademy-ai-mentor · 2026-07-07T17:05:19Z

+            logger.error("Failed login attempt for user: {}", login);
+            throw new AuthenticationException("Failed login attempt for user");
        }
+


This log message does not follow checklist item Let's make our log messages informative. It hard-codes the username and doesn’t include any parameters; consider using a parameterized message with the login value and logging it before or along with the login attempt (e.g., logger.info("User {} tries to login", login) or similar) rather than a fixed string.

mateacademy-ai-mentor · 2026-07-07T17:05:19Z

-        // NOTE: In production ready code this method should fetch data from DB
-        // For test purpose we simplify this method and return dummy data
+
        Product iphone = new Product("iPhone X", BigDecimal.valueOf(1199));


Similarly, throwing a RuntimeException with a hard‑coded message but without including the original AuthenticationException loses context. While the task doesn’t mandate wrapping exceptions, the previous requirement focuses on logging the exception object; you should at least ensure the log above includes the AuthenticationException as a parameter so its stack trace is preserved.

mateacademy-ai-mentor · 2026-07-07T17:05:20Z

+    private static final Logger logger = LoggerFactory.getLogger(OrderServiceImpl.class);
+
    @Override
    public Order completeOrder(Long userId) {


The info log for the login method is informative and parameterized, which satisfies the checklist, but you can improve readability by adding a space after the colon: "Method login was called for user: {}". The earlier review explicitly mentioned tidying spacing/parameterization of these "method ... was called" logs.

mateacademy-ai-mentor · 2026-07-07T17:05:20Z

        Order order = new Order(products, userId);
-        // 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);


This failure log currently does not include the exception object as requested in the previous review (e.g. logger.error("Authentication failed for user {}", login, e);). To meet that requirement, update the AuthenticationException you throw so you can pass it to the logger, or log with an appropriate AuthenticationException instance. This violates the requirement: Update the failure logging in AuthenticationServiceImpl to include the exception and use parameter placeholders instead of hard-coded values or concatenation.

mateacademy-ai-mentor · 2026-07-07T17:05:20Z

        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 for user with Id: {}. ", userId);


The info log is parameterized and informative, which is good, but the spacing around the placeholder is slightly inconsistent ("... Id: {}" vs "... Id: {}. "). To address the earlier medium‑priority comment about tidy log messages, consider removing the trailing space in the message and keeping consistent spacing: e.g. "Successfully fetched data from DB for user with Id: {}.".

mateacademy-ai-mentor · 2026-07-07T17:05:20Z

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


Similarly, here you have no space after the comma between the literal and the placeholder ("... Id: {}",userId). For cleaner, consistent logging per the checklist and prior comments, add a space: "... Id: {}", userId.

added logger to project

bac7e6d

mateacademy-ai-mentor suggested changes Jul 7, 2026

View reviewed changes

fixed some issues on logger

1cf62af

3xactCoder requested a review from mateacademy-ai-mentor July 7, 2026 16:52

mateacademy-ai-mentor suggested changes Jul 7, 2026

View reviewed changes

fixed some issues on logger2

7887caf

3xactCoder requested a review from mateacademy-ai-mentor July 7, 2026 17:04

mateacademy-ai-mentor approved these changes Jul 7, 2026

View reviewed changes


		private static final Logger logger = LoggerFactory.getLogger(Main.class);

		public static void main(String[] args) {

Uh oh!

Conversation

3xactCoder commented Jul 7, 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!

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!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants