first_try #1069

YarikNarozhnyi01 · 2025-01-05T22:43:12Z

No description provided.

mateacademy-ai-mentor

Thank you for your hard work on this project! There are a few critical issues that need to be addressed before we can approve the pull request. These include the presence of TODO comments, which should be removed, and the use of org.apache.logging.log4j.core.Logger, which should be replaced with org.apache.logging.log4j.Logger for better practice. Additionally, the log messages need to be more informative, and the logger configuration should be checked to ensure it meets the task requirements. Please make these changes, and feel free to reach out if you have any questions. Keep up the great work! 😊

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

🔄 Fix the issues, commit the changes, and then re-request my review using the Re-request review button in the Reviewers tab.

mateacademy-ai-mentor · 2025-01-05T22:44:42Z

.gitignore

@@ -1,3 +1,4 @@
 .idea/*
 *.iml
 target/*
+src/main/resources/log4j2.xml


The line src/main/resources/log4j2.xml is set to be ignored. If this file contains sensitive information or is environment-specific, it's appropriate to ignore it. However, if it's a crucial part of the application's configuration that should be versioned, consider removing this line.

mateacademy-ai-mentor · 2025-01-05T22:44:42Z

src/main/java/mate/academy/Main.java

@@ -6,15 +6,18 @@
 import mate.academy.service.AuthenticationServiceImpl;
 import mate.academy.service.OrderService;
 import mate.academy.service.OrderServiceImpl;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.core.Logger;


Consider using org.apache.logging.log4j.Logger instead of org.apache.logging.log4j.core.Logger. The core package is typically used for internal Log4j components, and using the standard Logger interface is recommended for application logging.

mateacademy-ai-mentor · 2025-01-05T22:44:42Z

src/main/java/mate/academy/Main.java

    public static void main(String[] args) {
        AuthenticationService authenticationService = new AuthenticationServiceImpl();
        User user;
        try {
            user = authenticationService.login("bob", "1234");
        } catch (AuthenticationException e) {
-            e.printStackTrace();
+            logger.trace("User logins");


The logging level trace might be too low for this message. Consider using error or warn to indicate an unsuccessful login attempt. Additionally, the message 'User logins' is not very informative. According to the checklist, log messages should be more descriptive, such as 'Failed login attempt for user: bob'.

mateacademy-ai-mentor · 2025-01-05T22:44:42Z

src/main/java/mate/academy/service/AuthenticationServiceImpl.java

@@ -2,11 +2,16 @@

 import mate.academy.exception.AuthenticationException;
 import mate.academy.model.User;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.core.Logger;


Consider using org.apache.logging.log4j.Logger instead of org.apache.logging.log4j.core.Logger. The core package is typically used for internal Log4j components, and using the standard Logger interface is recommended for application logging.

mateacademy-ai-mentor · 2025-01-05T22:44:43Z

src/main/java/mate/academy/service/AuthenticationServiceImpl.java

    @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 : {}", login);


The log message 'Method login was called : {}' is not very informative. According to the checklist, log messages should be more descriptive. Consider changing it to something like 'Method login was called. Params: login={}'.

mateacademy-ai-mentor · 2025-01-05T22:44:43Z

src/main/java/mate/academy/service/AuthenticationServiceImpl.java


 public class AuthenticationServiceImpl implements AuthenticationService {
+    private static final Logger logger = (Logger) LogManager.getLogger(AuthenticationServiceImpl.class);
+
    @Override
    public User login(String login, String password) throws AuthenticationException {
        //TODO: add corresponding log message about method login was called


Remember to remove all TODO comments before submitting your solution, as per the checklist requirements.

mateacademy-ai-mentor · 2025-01-05T22:44:43Z

src/main/java/mate/academy/service/OrderServiceImpl.java

@@ -4,11 +4,15 @@
 import java.util.List;
 import mate.academy.model.Order;
 import mate.academy.model.Product;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.core.Logger;


Consider using org.apache.logging.log4j.Logger instead of org.apache.logging.log4j.core.Logger. The core package is typically used for internal Log4j components, and using the standard Logger interface is recommended for application logging.

mateacademy-ai-mentor · 2025-01-05T22:44:43Z

src/main/java/mate/academy/service/OrderServiceImpl.java

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


The log message 'Method completeOrder was called : {}' is not very informative. According to the checklist, log messages should be more descriptive. Consider changing it to something like 'Method completeOrder was called. Params: userId={}'.

mateacademy-ai-mentor · 2025-01-05T22:44:43Z

src/main/java/mate/academy/service/OrderServiceImpl.java

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


Remember to remove all TODO comments before submitting your solution, as per the checklist requirements.

mateacademy-ai-mentor

Great job on your progress! 🎉 While there are some areas that need improvement, the decision to approve your pull request is to help you move forward with your learning journey. Here are some key points to address:

Logger Configuration: Ensure that the logger is configured with appenders for both File and Console. The absence of the log4j2.xml file is critical since it typically contains these configurations. Make sure to include it if needed for deployment or testing.
Logger Usage: Update the Main.java file to use org.apache.logging.log4j.Logger instead of org.apache.logging.log4j.core.Logger to align with best practices.
Informative Log Messages: Enhance your log messages by adding more context, such as timestamps or user information, to aid in debugging and monitoring.
Unnecessary Casting: Simplify your logger declaration by using private static final Logger logger = LogManager.getLogger(Main.class); without casting.
File Ignorance: Double-check that ignoring log4j2.xml is intentional, as it is crucial for logging configuration.

By addressing these issues, you'll improve the functionality and maintainability of your code. Keep up the good work, and continue learning from these experiences! 🚀

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

mateacademy-ai-mentor · 2025-01-05T22:57:16Z

.gitignore

@@ -1,3 +1,4 @@
 .idea/*
 *.iml
 target/*
+src/main/resources/log4j2.xml


The log4j2.xml file is ignored. Make sure this is intentional, as this file is often crucial for configuring logging in different environments. If this file is needed for deployment or testing, consider removing it from .gitignore.

mateacademy-ai-mentor · 2025-01-05T22:57:17Z

src/main/java/mate/academy/Main.java

@@ -6,15 +6,19 @@
 import mate.academy.service.AuthenticationServiceImpl;
 import mate.academy.service.OrderService;
 import mate.academy.service.OrderServiceImpl;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.core.Logger;


It is recommended to use org.apache.logging.log4j.Logger instead of org.apache.logging.log4j.core.Logger. The core package is intended for internal use and may not be stable for public use.

mateacademy-ai-mentor · 2025-01-05T22:57:17Z

src/main/java/mate/academy/Main.java


 public class Main {
+    private static final Logger logger = (Logger) LogManager.getLogger(Main.class);


Casting LogManager.getLogger(Main.class) to Logger is unnecessary if you use org.apache.logging.log4j.Logger. Consider changing the logger declaration to private static final Logger logger = LogManager.getLogger(Main.class);.

mateacademy-ai-mentor · 2025-01-05T22:57:17Z

src/main/java/mate/academy/Main.java

    public static void main(String[] args) {
        AuthenticationService authenticationService = new AuthenticationServiceImpl();
        User user;
        try {
            user = authenticationService.login("bob", "1234");
        } catch (AuthenticationException e) {
-            e.printStackTrace();
+            logger.error("Failed login attempt for user: bob");


The log message could be more informative by including details such as the reason for the failure or additional context. Consider adding more information to help with debugging and monitoring.

mateacademy-ai-mentor · 2025-01-05T22:57:17Z

src/main/java/mate/academy/service/AuthenticationServiceImpl.java

    @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 : {}", login);


The log message 'Method login was called' is informative, but consider adding more context, such as the timestamp or additional user information, to make it more useful for debugging.

mateacademy-ai-mentor · 2025-01-05T22:57:17Z

src/main/java/mate/academy/service/OrderServiceImpl.java

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


The log message 'Method completeOrder was called' is informative, but consider adding more context, such as the timestamp or additional user information, to make it more useful for debugging.

mateacademy-ai-mentor · 2025-01-05T22:57:17Z

src/main/java/mate/academy/service/OrderServiceImpl.java

@@ -24,7 +28,7 @@
        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);
-        // TODO: add log message about successful fetched data from DB
+        logger.info("Data was successfully fetched from DB");


The log message 'Data was successfully fetched from DB' is informative, but since this is a dummy implementation, consider clarifying in the log that this is simulated data for testing purposes.

first_try

2c28454

mateacademy-ai-mentor suggested changes Jan 5, 2025

View reviewed changes

syntax changes

b737f52

YarikNarozhnyi01 requested a review from mateacademy-ai-mentor January 5, 2025 22:55

mateacademy-ai-mentor approved these changes Jan 5, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

first_try #1069

first_try #1069

YarikNarozhnyi01 commented Jan 5, 2025

mateacademy-ai-mentor left a comment

mateacademy-ai-mentor Jan 5, 2025

mateacademy-ai-mentor Jan 5, 2025

mateacademy-ai-mentor Jan 5, 2025

mateacademy-ai-mentor Jan 5, 2025

mateacademy-ai-mentor Jan 5, 2025

mateacademy-ai-mentor Jan 5, 2025

mateacademy-ai-mentor Jan 5, 2025

mateacademy-ai-mentor Jan 5, 2025

mateacademy-ai-mentor Jan 5, 2025

mateacademy-ai-mentor left a comment

mateacademy-ai-mentor Jan 5, 2025

mateacademy-ai-mentor Jan 5, 2025

mateacademy-ai-mentor Jan 5, 2025

mateacademy-ai-mentor Jan 5, 2025

mateacademy-ai-mentor Jan 5, 2025

mateacademy-ai-mentor Jan 5, 2025

mateacademy-ai-mentor Jan 5, 2025


		public class Main {
		private static final Logger logger = (Logger) LogManager.getLogger(Main.class);

first_try #1069

Are you sure you want to change the base?

first_try #1069

Conversation

YarikNarozhnyi01 commented Jan 5, 2025

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment