-
Notifications
You must be signed in to change notification settings - Fork 233
[CS2113-T11a-2] LogJob #31
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?
[CS2113-T11a-2] LogJob #31
Conversation
…nage-groups Feature manage groups
sevenseasofbri
left a comment
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.
Very good job team! I think the code is well structured and follows good code quality practices for the most part. I have briefly gone over and added some comments. Let me know if you have any questions.
| public boolean isRunning() { | ||
| return true; | ||
| } |
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 method always returns true, so how does it track state? If this is meant to represent what this command implies (i.e. if the command the current object contains means the application continues running) then i think you should name the function something else like shouldContinue or getNextRunState
| public boolean isRunning() { | ||
| return false; | ||
| } |
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.
Same with this, this method always returns false...
| } | ||
| } | ||
|
|
||
| private static void containsAllFlags(HashMap<Flag, List<String>> argMap, Flag... flags) |
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 function name reads more like a boolean variable name. It does not accurately describe what the code is doing - which is checking for missing flags right. So maybe a better name would be checkMissingFlags
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.
Same applies to containsNoDuplicateFlags
| } | ||
| } | ||
|
|
||
| private void requireNonNullFile() throws StorageException { |
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.
Not exactly an apt verb for this function name. Something like checkAndCreateNonNullFile or ensureFileExists would be better.
|
|
||
| } | ||
|
|
||
| public void introMessage() { |
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.
| public void introMessage() { | |
| public void printIntroMessage() { |
| System.out.println(Constants.LINE_BREAK); | ||
| } | ||
|
|
||
| public void exitMessage() { |
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.
| public void exitMessage() { | |
| public void printExitMessage() { |
76 tokenizer preamble
Add JUnit and Assertion for UI package
…ndexOutOfBoundsException
Fix LogJob logo in Readme.md
Refactor LogJob class into a Singleton to prevent erroneous instantiation
Fix storage bug when program exits unexpectedly
Extend JUnit tests to include more error paths for DeleteCommandTest
Add Sorting Function
Add Find Command
edit zexueteh PPP
Add PPP and UG changes
Update PPP and UG
Enable assertions
link bugfix
Minor bugfix
Minor change to PPP
fix hyperlinks to repo
remove PUML guide
… 223-Link-bugfix
fix Model links and find sequence diagram
fix startup image
LogJob
LogJob (LJ) is a desktop app for managing job applications, optimized for use via a Command Line Interface (CLI). If you can type fast, LJ can get your job application management tasks done faster than traditional GUI apps.