Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
.idea/*
*.iml
target/*
log/
7 changes: 7 additions & 0 deletions log/app.log
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
2026-07-07 19:10:23 [main] INFO mate.academy.service.AuthenticationServiceImpl - Method login was called for user: bob
2026-07-07 19:10:23 [main] INFO mate.academy.service.AuthenticationServiceImpl - User bob successfully logged in.
2026-07-07 19:10:23 [main] INFO mate.academy.service.OrderServiceImpl - Method completeOrder was called for user Id: 2
2026-07-07 19:12:49 [main] INFO mate.academy.service.AuthenticationServiceImpl - Method login was called for user: bob
2026-07-07 19:12:49 [main] INFO mate.academy.service.AuthenticationServiceImpl - User bob successfully logged in.
2026-07-07 19:12:49 [main] INFO mate.academy.service.OrderServiceImpl - Method completeOrder was called for user Id: 2
2026-07-07 19:12:49 [main] INFO mate.academy.service.OrderServiceImpl - Successfully fetched data from DB for user Id: 2. Retracted 3 products.
22 changes: 22 additions & 0 deletions pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,29 @@
<version>4.12</version>
<scope>test</scope>
</dependency>


<dependency>
<groupId>org.slf4j</groupId>
<artifactId>slf4j-api</artifactId>
<version>2.0.12</version>
</dependency>

<dependency>
<groupId>org.apache.logging.log4j</groupId>
<artifactId>log4j-core</artifactId>
<version>2.23.1</version>
</dependency>

<dependency>
<groupId>org.apache.logging.log4j</groupId>
<artifactId>log4j-slf4j2-impl</artifactId>
<version>2.23.1</version>
</dependency>

</dependencies>


<build>
<plugins>
<plugin>
Expand Down
11 changes: 9 additions & 2 deletions src/main/java/mate/academy/Main.java
Original file line number Diff line number Diff line change
Expand Up @@ -6,16 +6,23 @@
import mate.academy.service.AuthenticationServiceImpl;
import mate.academy.service.OrderService;
import mate.academy.service.OrderServiceImpl;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

public class Main {

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

public static void main(String[] args) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


AuthenticationService authenticationService = new AuthenticationServiceImpl();
User user;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);.

} catch (AuthenticationException e) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

e.printStackTrace();
return;
logger.error("Authentification failed for user bob");

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

throw new RuntimeException("Authentification failed for user bob");
}
OrderService orderService = new OrderServiceImpl();
orderService.completeOrder(user.getUserId());
Expand Down
29 changes: 22 additions & 7 deletions src/main/java/mate/academy/service/AuthenticationServiceImpl.java
Original file line number Diff line number Diff line change
@@ -1,23 +1,38 @@
package mate.academy.service;

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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."

import mate.academy.model.User;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

public class AuthenticationServiceImpl implements AuthenticationService {

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);
Comment thread
3xactCoder marked this conversation as resolved.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no space after the comma between {} and userId in this log call, which was part of the previous medium-level feedback about tidy and consistent log formatting. Adjusting to logger.info("Method completeOrder was called for user Id: {}", userId); will improve readability and parameterization.

if (!user.getPassword().equals(password)) {
throw new AuthenticationException("Username or password are incorrect");

Comment on lines +10 to +17

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (user == null || !Objects.equals(user.getPassword(), password)) {
logger.error("Failed login attempt for user: {}", login);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The failure log here does not include the exception object, which was requested in the previous review: Update the failure logging in AuthenticationServiceImpl to include the exception and use parameter placeholders ... e.g. logger.error("Authentication failed for user {}", login, e);. You can create the AuthenticationException instance and pass it as the third argument to logger.error so that the stack trace is logged.

throw new AuthenticationException("Failed login attempt for user");
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

logger.info("User {} successfully logged in.", login);
Comment on lines +18 to +23

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return user;
}
Comment on lines 24 to 25

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


private User findByLogin(String login) {
User user = new User(login, "1234");
// this user identifier should be set by DB. We will use dummy data for this example
user.setUserId(2L);
return user;
if ("bob".equals(login)) {
User user = new User(login, "1234");
user.setUserId(2L);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This log message has a trailing space before the closing quote: "... Id: {}. ". The previous review asked to clean up such minor issues so log output is tidy; removing the extra space (and keeping the period if you want) will make the message consistent.

return user;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return null;
}
Comment on lines +33 to 34

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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());.

}



15 changes: 9 additions & 6 deletions src/main/java/mate/academy/service/OrderServiceImpl.java
Original file line number Diff line number Diff line change
Expand Up @@ -4,27 +4,30 @@
import java.util.List;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The .gitignore correctly ignores the log/ directory for log files, but make sure that your actual log file path used by the logger configuration will reside under this log/ directory; otherwise the requirement "so that your file with logs is NOT pushed to GitHub" would not be fully satisfied.

import mate.academy.model.Order;
import mate.academy.model.Product;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

public class OrderServiceImpl implements OrderService {

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

@Override
public Order completeOrder(Long userId) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// TODO: add log message about method completeOrder was called
logger.info("Method completeOrder was called for user Id: {}",userId);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

List<Product> products = getAllProductsFromShoppingCart(userId);
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);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return order;
}

private List<Product> getAllProductsFromShoppingCart(Long userId) {
// NOTE: In production ready code this method should fetch data from DB
// For test purpose we simplify this method and return dummy data

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error log violates the logging checklist and the previous high‑priority requirement: failure logging should include the exception object and use parameter placeholders instead of hard‑coded values. Consider something like logger.error("Authentication failed for user {}", "bob", e); so that the stack trace is captured and the message is parameterized. This aligns with the requirement: logger.error("Authentication failed for user {}", login, e);.

Product iphone = new Product("iPhone X", BigDecimal.valueOf(1199));

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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("Successfully fetched data from DB for user with Id: {}. ", userId);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: {}.".

return products;
}
}
23 changes: 23 additions & 0 deletions src/main/resources/log4j2.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
<?xml version="1.0" encoding="UTF-8"?>
<Configuration status="WARN">
<Properties>
<Property name="basePath">logs</Property>
</Properties>

<Appenders>
<Console name="ConsoleAppender" target="SYSTEM_OUT">
<PatternLayout pattern="%d{yyyy-MM-dd HH:mm:ss} [%t] %-5level %logger{36} - %msg%n"/>
</Console>

<File name="FileAppender" fileName="log/app.log" append="true">
<PatternLayout pattern="%d{yyyy-MM-dd HH:mm:ss} [%t] %-5level %logger{36} - %msg%n"/>
</File>
</Appenders>

<Loggers>
<Root level="info">
<AppenderRef ref="ConsoleAppender"/>
<AppenderRef ref="FileAppender"/>
</Root>
</Loggers>
</Configuration>
Loading