Added JUnit libary #17
Conversation
| <project xmlns="http://maven.apache.org/POM/4.0.0" | ||
| xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" | ||
| xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd"> | ||
| xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" | ||
| xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 | ||
| http://maven.apache.org/xsd/maven-4.0.0.xsd"> |
There was a problem hiding this comment.
Why was this changed? Apache may have their own specific standard, but that is not much different from the original header before this change. I believe the original header is the default for generating a POM in IntelliJ and should work fine for our project. Having a line for the XML stating the version and encoding is also important to ensure there are zero possible compatibility issues between any IDEs or editors.
| @@ -14,26 +15,26 @@ | |||
|
|
|||
| public class BotMain { | |||
|
|
|||
| private static final Logger logger = LoggerFactory.getLogger(BotMain.class); | |||
| private static final Logger LOG = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass()); | |||
There was a problem hiding this comment.
Why use MethodHandles? With a proper IDE refactoring function, this should not be necessary if the class name is ever changed. MethodHandles is more low-level compared to Reflection and should not be used in a repository aimed at allowing beginner programmers to commit and view code. Using a direct call to .class from the name of the class seems easier and better for performance and code readability. However, I do believe the logger name change is better suited to fit conventions.
| /** | ||
| * A command listener for JDA. It is responsible for listening to messages an invoking commands. | ||
| */ | ||
| public class CommandListener extends ListenerAdapter { | ||
|
|
||
| private static final Logger LOG = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass()); |
There was a problem hiding this comment.
See comment on MessageHandles in BotMain. I believe this class should have a logger but make sure to keep formatting the same, as you accidentally deleted the empty line put between the first variable and the class declaration line.
| @@ -54,6 +58,7 @@ public void onMessageReceived(@Nonnull MessageReceivedEvent event) { | |||
| ); | |||
| executor.execute(content, context); | |||
| } catch (CommandNotFoundException | NoSeparatorException ignored) { | |||
| LOG.trace("Command not foundor no separator in message {}", content); | |||
There was a problem hiding this comment.
Small little typing issue.
| LOG.trace("Command not foundor no separator in message {}", content); | |
| LOG.trace("Command not found or no separator in message {}", content); |
| private UserParsers() { | ||
| throw new IllegalStateException("Utility class"); | ||
| } |
There was a problem hiding this comment.
It might be important to decide if we should throw an exception like this to state that this is a utility class or just have a single line of private UserParsers() {} and expect future contributors of this class or any other util class to not instantiate this class? Since we are targeting this repository towards beginners, it might make sense. However, this may not be necessary.
| import java.util.Map; | ||
| import java.util.Optional; | ||
|
|
||
| /** | ||
| * A collection of messages, read from a file and maybe translated. | ||
| */ | ||
| public class Messages { | ||
|
|
||
| private static final Logger LOG = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass()); |
There was a problem hiding this comment.
See message on MethodHandles in BotMain.
| }); | ||
| } catch (IOException e) { | ||
| e.printStackTrace(); | ||
| LOG.error("An error occurred while reading messages ",e); |
There was a problem hiding this comment.
No space necessary. Maybe a typo when trying to put a space after the comma?
| LOG.error("An error occurred while reading messages ",e); | |
| LOG.error("An error occurred while reading messages", e); |
| import org.junit.jupiter.api.Test; | ||
|
|
||
| import java.time.Duration; | ||
| import static org.assertj.core.api.Assertions.*; |
There was a problem hiding this comment.
Space between imports and class declaration for standard.
| import static org.assertj.core.api.Assertions.*; | |
| import static org.assertj.core.api.Assertions.*; | |
| import static org.assertj.core.api.Assertions.*; | ||
| class DurationFormatterTest { | ||
|
|
||
| private DurationFormatter durationFormatterUnderTest; |
There was a problem hiding this comment.
This seems like a long variable name for no reason. A better name would be formatter or durationFormatter.
|
|
||
| private DurationFormatter durationFormatterUnderTest; | ||
|
|
||
| @BeforeEach |
There was a problem hiding this comment.
I see no point in creating a new DurationFormatter before each test, maybe use @BeforeAll? Also, we may want to convert DurationFormatter to a static util class; however this can be done at a later date.
| @BeforeEach | |
| @BeforeAll |
I have added JUnit jupiter and assertJ to the project to make it easier to write tests in the future