Add logger#1242
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am asking for changes because a core requirement of the task is not yet met: the project does not contain a Log4j2 configuration file that defines both Console and File appenders [CHECKLIST ITEM #1]. You correctly use Logger in code (e.g., in src/main/java/mate/academy/Main.java, AuthenticationServiceImpl.java, and OrderServiceImpl.java), and .gitignore already ignores the logs/ directory, but without a log4j2.xml or log4j2.properties under src/main/resources, the required appenders are never actually configured. To fix this, add a Log4j2 configuration file (for example src/main/resources/log4j2.xml) that declares a Console appender and a File appender writing to something like logs/app.log, and configure the root logger (or your mate.academy package logger) to use both appenders. Once that is in place, your existing logger calls will output to both the console and the log file as the task requires. As a refinement, consider logging exceptions with the throwable itself rather than only e.getMessage() so that stack traces are captured, and clean up any minor inconsistencies in log message text (capitalization, trailing spaces). Your core code structure and logger usage are solid; after you add the proper Log4j2 configuration, the solution will align well with the assignment’s expectations.
✨ 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
|
|
||
| public class Main { | ||
| public static void main(String[] args) { | ||
| Logger logger = LogManager.getLogger(Main.class); |
There was a problem hiding this comment.
This violates checklist item #1: "Configure the logger in this project. Add appenders for File and Console" — I don't see any Log4j2 configuration file (e.g., src/main/resources/log4j2.xml or log4j2.properties) in the project. Add a logger configuration that defines a Console appender and a File appender (writing to the ignored logs/ folder), and configure a root logger or package logger to use them so logs are written both to console and to a file.
| 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. Login: {}", login); | ||
| User user = findByLogin(login); |
There was a problem hiding this comment.
This violates checklist item #1: "Configure the logger in this project and add appenders for File and Console". I don't see a Log4j2 configuration (e.g., log4j2.xml or log4j2.properties) in the project. Add a logger configuration file (for example src/main/resources/log4j2.xml) that defines a Console appender and a File appender so logs are written to both console and a file.
| private static final Logger logger = LogManager.getLogger(OrderServiceImpl.class); | ||
|
|
||
| @Override | ||
| public Order completeOrder(Long userId) { |
There was a problem hiding this comment.
This violates checklist item #1: "Configure the logger in this project and add appenders for File and Console." I do not see any Log4j2 configuration file (for example, src/main/resources/log4j2.xml or log4j2.properties) in the project, so Console and File appenders are not defined. Add a configuration file that declares both a Console and a File appender (writing to a file inside the ignored logs/ folder) so the logger behavior matches the task requirement.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Sorry, the pull request review was unsuccessful for the following reasons:
Warning
No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
[CODE: NCM]
If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.
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 a core logging requirement from the task is not yet satisfied and this directly violates the checklist. Specifically, your project is missing a Log4j2 configuration file that defines both a Console and a File appender and binds them to the root or mate.academy logger, so your LogManager.getLogger(...) calls will not actually write to a log file as required [CHECKLIST ITEM #1]. To fix this, add a configuration file such as src/main/resources/log4j2.xml that declares a Console appender, a File appender (e.g., writing to logs/app.log), and configures the root or mate.academy logger to use both appenders. In addition, in src/main/java/mate/academy/Main.java (and similarly in your service classes), you currently log exceptions as substitution parameters (e.g., logger.error("Authentication Exception. {}", e);), which means the stack trace will not be recorded; instead, pass the throwable as the last argument, for example logger.error("Authentication exception while logging in", e);. Aside from these points, you have done well to remove System.out.println, avoid printStackTrace, and use Log4j2 consistently across the codebase. Once the Log4j2 configuration is in place and exceptions are logged with the throwable, your solution will align well with the task requirements.
✨ 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
|
|
||
| public class Main { | ||
| public static void main(String[] args) { | ||
| Logger logger = LogManager.getLogger(Main.class); |
There was a problem hiding this comment.
This violates checklist item #1: "the project does not contain a Log4j2 configuration file that defines both Console and File appenders [CHECKLIST ITEM #1]". Add a Log4j2 configuration (for example src/main/resources/log4j2.xml) that declares a Console appender and a File appender (e.g., logs/app.log) and configure the root logger or the mate.academy package logger to use both appenders so LogManager.getLogger(...) actually writes to console and the log file.
There was a problem hiding this comment.
It's probably a mistake, because i already have example src/main/resources/log4j2.xml
| user = authenticationService.login("bob", "1234"); | ||
| } catch (AuthenticationException e) { | ||
| e.printStackTrace(); | ||
| logger.error("Authentication Exception. {}", e); |
There was a problem hiding this comment.
Log the exception as a throwable to capture the stack trace. Currently you pass the exception as a substitution parameter which will not include the stack trace. Consider using e.g. logger.error("Authentication exception while logging in", e); or include params and pass the throwable as the last argument.
| import org.apache.logging.log4j.Logger; | ||
|
|
||
| public class AuthenticationServiceImpl implements AuthenticationService { | ||
| private static final Logger logger = LogManager.getLogger(AuthenticationServiceImpl.class); |
There was a problem hiding this comment.
This project is missing the required Log4j2 configuration. Specifically: "the project does not contain a Log4j2 configuration file that defines both Console and File appenders [CHECKLIST ITEM #1]". Add a configuration file (e.g. src/main/resources/log4j2.xml) that declares a Console appender and a File appender (writing to something like logs/app.log) and configure the root logger or the mate.academy package logger to use both appenders so logs are output to console and file.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your solution as an educational override so you can continue progressing, but there is one important requirement that still needs attention. You correctly replaced System.out.println and e.printStackTrace() with Log4j2 logging in Main.java, AuthenticationServiceImpl.java, and OrderServiceImpl.java, and you already ignore the logs/ directory in .gitignore, which is great. However, the project is missing a Log4j2 configuration file (for example src/main/resources/log4j2.xml), so the loggers you create with LogManager.getLogger(...) in these classes currently won’t write anything to a log file, which violates the requirement to “Configure the logger in this project. Add appenders for File and Console” [CHECKLIST ITEM #1]. To fix this, add a log4j2.xml that defines both a Console appender and a File appender (e.g. targeting logs/app.log) and bind them to the root logger or the mate.academy logger, so all existing log calls in those classes automatically go to both console and file. As a minor improvement, double‑check that every source file ends with a trailing newline and that log messages use consistent formatting and capitalization. Overall, your logging usage is solid; once you add the proper Log4j2 configuration file, your solution will fully meet the logging checklist requirements.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| 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. Login: {}", login); | ||
| User user = findByLogin(login); |
There was a problem hiding this comment.
This violates the checklist item: "Configure the logger in this project. Add appenders for File and Console" — I cannot find a Log4j2 configuration file (for example src/main/resources/log4j2.xml) that declares a Console appender and a File appender and binds them to the root or mate.academy logger. Add such a config so LogManager.getLogger(...) actually writes to a log file (for example a File appender writing to logs/app.log).
| private static final Logger logger = LogManager.getLogger(OrderServiceImpl.class); | ||
|
|
||
| @Override | ||
| public Order completeOrder(Long userId) { |
There was a problem hiding this comment.
Missing Log4j2 configuration file. This violates the requirement: "A Log4j2 configuration file must be present that defines both a Console appender and a File appender and binds them to the root logger or the mate.academy logger, so that LogManager.getLogger(...) calls actually write to a log file [CHECKLIST ITEM #1]". Please add a config (for example, src/main/resources/log4j2.xml) declaring a Console appender and a File appender (e.g. writing to logs/app.log) and bind them to the root or mate.academy logger so logs are written to file.
|
|
||
| public class Main { | ||
| public static void main(String[] args) { | ||
| Logger logger = LogManager.getLogger(Main.class); |
There was a problem hiding this comment.
This violates the checklist: "Add a Log4j2 configuration file that declares both a Console appender and a File appender and configures the root logger or the mate.academy logger to use both appenders, so logging actually goes to the log file as required [CHECKLIST ITEM #1]." There's no src/main/resources/log4j2.xml (or other Log4j2 config) in the project, so LogManager.getLogger(...) calls won't write to a log file. Please add a configuration file (e.g. src/main/resources/log4j2.xml) that declares a Console and a File appender and binds them to the root or mate.academy logger (for example writing to logs/app.log), and ensure logs/ is in .gitignore.
2 participants
No description provided.