did all required tsks#1254
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your implementation is missing several critical requirements from the task checklist. First, the log4j.xml configuration file is not provided — checklist item #1 explicitly requires configuring the logger with File and Console appenders in src/main/resources/. Second, the .gitignore file is completely missing, which is needed to exclude log files from GitHub. Third, log messages across files use string concatenation ("param=" + param) instead of the parameterized form ("param={}", param) as demonstrated in the checklist's good example. Please add the missing configuration files and update all logger calls to use {} placeholders.
✨ 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
| import mate.academy.service.AuthenticationServiceImpl; | ||
| import mate.academy.service.OrderService; | ||
| import mate.academy.service.OrderServiceImpl; | ||
| import org.apache.log4j.Logger; |
There was a problem hiding this comment.
Missing logger configuration file. According to checklist item #1, you must configure the logger with File and Console appenders. Create a log4j.properties or log4j.xml file in src/main/resources/ directory.
| import org.apache.log4j.Logger; | ||
|
|
||
| public class Main { | ||
| private static final Logger logger = Logger.getLogger(Main.class); |
There was a problem hiding this comment.
This violates checklist item: log messages should use parameterized form instead of string concatenation. Change to: logger.debug("Method login was called. Params: login={}", login);
| @@ -6,15 +6,18 @@ | |||
| import mate.academy.service.AuthenticationServiceImpl; | |||
| import mate.academy.service.OrderService; | |||
There was a problem hiding this comment.
Missing logger configuration file. According to checklist item #1, you must configure the logger with File and Console appenders.
| public class Main { | ||
| private static final Logger logger = Logger.getLogger(Main.class); | ||
|
|
||
| public static void main(String[] args) { |
There was a problem hiding this comment.
This violates checklist item: log messages should use parameterized form instead of string concatenation. Change to: logger.debug("Method completeOrder was called. Params: userId={}", userId);
| public class OrderServiceImpl implements OrderService { | ||
| private static final Logger logger = Logger.getLogger(OrderServiceImpl.class); | ||
|
|
||
| @Override |
There was a problem hiding this comment.
This uses string concatenation. According to the checklist's good example, use parameterized logging: logger.info("Method login was called. Params: login={}", login);
| @Override | ||
| public Order completeOrder(Long userId) { | ||
| // TODO: add log message about method completeOrder was called | ||
| logger.info("Method completeOrder was called. Params: userId=" + userId); |
There was a problem hiding this comment.
This uses string concatenation. Use parameterized logging: logger.info("Method completeOrder was called. Params: userId={}", 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 | ||
| logger.info("Data was successfully fetched from DataBase"); |
There was a problem hiding this comment.
Consider adding parameter context, e.g., what data was fetched or for which userId, to make the log more informative as per the checklist requirements.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
The log4j.xml configuration file is missing from src/main/resources/, which is required by [CHECKLIST ITEM #1]. You need to create this configuration file that defines both File and Console appenders to enable proper logging functionality. Without this configuration, the logging feature cannot work as specified in the 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
| @@ -1,3 +1,4 @@ | |||
| .idea/* | |||
There was a problem hiding this comment.
The log4j.xml configuration file in src/main/resources/ is missing. Checklist item #1 requires configuring the logger with File and Console appenders. You need to create this configuration file to enable logging functionality.
| import org.apache.logging.log4j.LogManager; | ||
| import org.apache.logging.log4j.Logger; | ||
|
|
||
| public class OrderServiceImpl implements OrderService { | ||
| private static final Logger logger = LogManager.getLogger(OrderServiceImpl.class); |
There was a problem hiding this comment.
Missing log4j.xml configuration file in src/main/resources/ directory. Checklist item #1 requires configuring the logger with File and Console appenders. Add a configuration file that defines both appenders.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I'm providing an educational approval to help you progress while learning. You've done great work implementing proper Log4j2 logging throughout all Java files, replacing System.out.println() and e.printStackTrace() with parameterized logger methods. The .gitignore correctly excludes the logs/ directory, which is excellent. However, the log4j.xml configuration file is still missing from src/main/resources/, which is a critical requirement from the task description that specifies 'Configure the logger in this project. Add appenders for File and Console'. This configuration file is essential to enable your logging functionality to work properly. Please create this file with both Console and File appenders configured, and you'll be all set!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| @@ -1,3 +1,4 @@ | |||
| .idea/* | |||
There was a problem hiding this comment.
The log4j.xml configuration file with File and Console appenders is missing from src/main/resources/. This is required by the task description: 'Configure the logger in this project. Add appenders for File and Console'. Create this configuration file to enable proper logging functionality.
2 participants
No description provided.