-
Notifications
You must be signed in to change notification settings - Fork 364
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
[jeff] iP #256
base: master
Are you sure you want to change the base?
[jeff] iP #256
Conversation
0c30cb7
to
a9bbc0c
Compare
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.
Great personality 👍
LGTM! Great job in following the coding conventions as well!
src/main/java/Miki.java
Outdated
import command.Command; | ||
import shigure.Ui; | ||
import storage.Storage; | ||
import task.Parser; | ||
import task.TaskList; | ||
|
||
import java.io.IOException; |
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.
Great ordering of import statements!
src/main/java/Miki.java
Outdated
try { | ||
storage.load("autosave.txt", tasks); | ||
} catch (IOException | Storage.MikiLoadException ex) { | ||
|
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.
Would it be better if the user can see that the loading has error?
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.
True - no message is raised if there's no autosave file since that's 'normal' when we're running for the first time,
but now if the file exists but is corrupt there's a new message to alert the user.
src/main/java/Miki.java
Outdated
for (int i = 0; i < args.length; i++) { | ||
if (args[i].equals("--ascii-only")) { | ||
hasAsciiOnly = true; | ||
} | ||
if (args[i].equals("--no-autoload")) { | ||
hasNoAutoload = 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.
I really like the feature of no autoloading of task!
src/main/java/command/Find.java
Outdated
for (int i = 0; i < tasks.size(); i++) { | ||
if (tasks.get(i).hasMatchingObjective(regex)) { | ||
matches++; | ||
} | ||
} | ||
ui.print("here's your " + matches + (matches == 1 ? " match:" : " matches:")); | ||
for (int i = 0; i < tasks.size(); i++) { | ||
if (tasks.get(i).hasMatchingObjective(regex)) { | ||
ui.print(i + 1 + ". " + tasks.get(i)); |
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.
Can we run the linear search only once?
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.
Yep, probably a good idea to keep the expensive regexes to a minimum - adjusted accordingly!
src/main/java/storage/Storage.java
Outdated
} | ||
} | ||
} catch (NumberFormatException | TaskParseException ex) { | ||
throw new MikiLoadException("this file is corrupt..."); |
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.
Corrupted instead of corrupt?
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.
👀 Both are grammatical actually, but looks like corrupted is a lot more common when referring to files so switched to that
src/main/java/task/Deadline.java
Outdated
public static Deadline parseArgs(String[] args) throws TaskParseException { | ||
String objective = ""; | ||
String by = ""; | ||
boolean isInTokenBy = 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.
Perharps can explain what is the situation that this boolean is used for.
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.
Added a little comment to that line & similar lines elsewhere 👍
/** | ||
* {@inheritDoc} | ||
*/ |
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.
Great use of inheritDoc in here and other parts of the code!
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.
Looks good a lot of abstraction following single responsibility principle
The previous commit caused non-functional Java Archive files to be created as the main() method was placed in a class extending JavaFX's Application. JAR generation needs to be restored to allow for easier usage of the project. Let's rectify JAR generation by moving the main() method into a different class.
There are portions of code which expect arguments to be non-null, or call methods which are expected to have certain behaviours. If these code elements encounter incorrect arguments or method behaviour, they may throw difficult-to-diagnose Exceptions or fail silently. Let's add assertions to these portions of code to make it easier to diagnose such situations, if they arise due to bugs elsewhere in this project or in external Java modules. Using assertions allows a visible and clear error to be thrown when an unexpected situation arises, with the affected source line indicated and a relevant message of choice included.
There are sections of code which deviate from code quality guidelines. This makes the code harder to read and understand, and thus more difficult to debug. Let's, * improve code formatting, spacing and variable naming * add better comments to less-obvious code actions * perform method extraction on segments with deep nesting
Add Assertions
Refactor sources to code quality guidelines
Add natural language date support
Project Miki 🎀✨
Miki frees your mind from its physical locus.
Miki is
All you need to do is:
java -jar miki.jar
With Project Miki, you can:
soon(time is subjective))