-
Notifications
You must be signed in to change notification settings - Fork 1.1k
feat: configure logging and replace println/stacktraces #1229
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,5 @@ | ||
| .idea/* | ||
| *.iml | ||
| target/* | ||
| logs/ | ||
| *.log |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,22 +2,26 @@ | |
|
|
||
| import mate.academy.exception.AuthenticationException; | ||
| import mate.academy.model.User; | ||
| import org.apache.log4j.Logger; | ||
|
|
||
| public class AuthenticationServiceImpl implements AuthenticationService { | ||
| private static final Logger logger = Logger.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("login() called for user: " + login); | ||
| User user = findByLogin(login); | ||
| if (!user.getPassword().equals(password)) { | ||
| logger.warn("Invalid credentials for user: " + login); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This warning message is good because it includes the username. To make it even more helpful, you could be more specific about why it failed. For example: |
||
| throw new AuthenticationException("Username or password are incorrect"); | ||
| } | ||
| logger.info("User authenticated: " + login); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a good log entry for a successful event. To improve consistency with other log messages, consider a more structured format like: |
||
| return user; | ||
| } | ||
|
|
||
| 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); | ||
| user.setUserId(2L); // dummy data | ||
| return user; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| log4j.rootLogger=INFO, console, file | ||
|
|
||
| # Console appender | ||
| log4j.appender.console=org.apache.log4j.ConsoleAppender | ||
| log4j.appender.console.layout=org.apache.log4j.PatternLayout | ||
| log4j.appender.console.layout.ConversionPattern=%d{ISO8601} %-5p [%t] %c - %m%n | ||
|
|
||
| # File appender | ||
| log4j.appender.file=org.apache.log4j.RollingFileAppender | ||
| log4j.appender.file.File=logs/app.log | ||
| log4j.appender.file.MaxFileSize=5MB | ||
| log4j.appender.file.MaxBackupIndex=3 | ||
| log4j.appender.file.layout=org.apache.log4j.PatternLayout | ||
| log4j.appender.file.layout.ConversionPattern=%d{ISO8601} %-5p [%t] %c - %m%n |
There was a problem hiding this comment.
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 it could be more structured to better align with the 'Good example' from the checklist. Consider including the parameter name in the message for clarity, for example:
"login() method called with login='" + login + "'".