-
Notifications
You must be signed in to change notification settings - Fork 270
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
[Elumalai Oviya Dharshini] iP #275
base: master
Are you sure you want to change the base?
Conversation
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 see that you like Genshin Impact a lot 💯
Overall, I found your code easy to read and understand, except for a few places where the formatting could be improved. I noticed a few minor coding standard violations too. Other than that, LGTM. Keep up the good work! 👍
boolean isFirst = true; | ||
for (int i = 0; i < tasks.size(); i++) { | ||
String s = isFirst ? "" : "\n"; | ||
f.write(s + tasks.get(i).writeToFile()); |
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.
Perhaps a line break after the "." operators would enhance readability? I noticed the same issue in several other places too.
String s = ""; | ||
for (int i = 0; i < tasks.size(); i++) { | ||
if (!tasks.get(i).isEmpty()) { | ||
int index = i + 1; | ||
s += " " + index + ". " + tasks.get(i) + "\n"; | ||
} | ||
else { | ||
s += "You have " + i +" tasks on your list."; | ||
break; | ||
} | ||
} | ||
if (tasks.isEmpty()) { | ||
s = "You now have 0 tasks on your list."; | ||
} | ||
|
||
return s; |
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.
Maybe try adding line breaks in between logical units present in the code?
In addition, if-else statements should have the following form:
if (condition) {
statements;
} else {
statements;
}
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.
Thanks for highlighting this! I have now changed the code to follow the Java coding standards more closely
import org.junit.jupiter.api.Test; | ||
|
||
import static org.junit.jupiter.api.Assertions.assertEquals; | ||
import static org.junit.jupiter.api.Assertions.fail; |
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.
Minor nitpick, but import statements here should be reordered as per the Java coding standard. I noticed similar issues in other places too.
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 had a look at this and the guideline mentions that "the ordering of import statements must be consistent" but is otherwise generally vague. The consensus on a few StackOverflow posts appears to be that they ought to be in order of importance, then alphabetical order, with custom packages being imported after other packages. Is there another - perhaps more definitive - source you can point me to with regards to this?
assertEquals("", Parser.parse("hi")); | ||
assertEquals("", Parser.parse("todo")); | ||
assertEquals("", Parser.parse("todo ")); | ||
assertEquals("", Parser.parse("event ")); | ||
assertEquals("", Parser.parse("event birthday at")); | ||
assertEquals("", Parser.parse("event birthday at ")); | ||
assertEquals("event", Parser.parse("event birthday at 1500")); | ||
assertEquals("", Parser.parse("deadline cs2103t week 3 ip tasks by")); | ||
assertEquals("", Parser.parse("deadline cs2103t week 3 ip tasks by ")); | ||
assertEquals("deadline", Parser.parse("deadline cs2103t week 3 ip tasks by 2022/01/27 2359")); |
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.
Consider splitting the test into multiple JUnit test methods?
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.
The whole code looks very clear and I know what is every method doing, the structure of the project is also neat and clean! Keep up the good work!
* @author Elumalai Oviya Dharshini | ||
* @version 0.1 | ||
*/ | ||
public class TaskWithDateTime extends Task { |
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 like how you make another TaskWithDateTime class that differentiate between normal task and task with date and time!
|
||
public class DeadlineTest { | ||
@Test | ||
public void testStringConversion() { |
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.
Clear method name, it is obvious what is going to tested in this method
} | ||
|
||
@Test | ||
public void testWriteToFile() { |
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.
Clear method name, it is obvious what is going to tested in this method!
* @return formatted string of Todo, its completion status and a Todo marker | ||
*/ | ||
@Override | ||
public String writeToFile() { |
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.
One suggestion: Maybe you can rename writeToFile as taskToStringFormat ? Because write to file at the first sight, it is going to do the whole thing writing to file, but it is just producing String format of the task to be written to file.
But it is also good that you use writeToFile (no harm)
src/main/java/duke/Storage.java
Outdated
String[] tmp = line.split("\\|"); | ||
boolean isDone = tmp[1].trim().equals("D"); | ||
|
||
switch (tmp[0].trim()) { |
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.
You followed the coding standard for switch case! Great Job!
src/main/java/duke/Storage.java
Outdated
//Reused from https://stackoverflow.com/questions/27904329/warning-file-mkdir-is-ignored#answer-54341862 | ||
// with minor modifications | ||
@SuppressWarnings("unused") | ||
private static void IGNORE_RESULT(boolean result) { |
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 am not sure what does this means, it is not returning anything
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 utility function is defined for mkdirs
- which creates parent directories of a given location (if they don't exist) and returns True
or False
depending on whether directories were created. This suppresses the File.mkdirs() is ignored
warning that results from not handling the return value of mkdirs
.
I am not certain if this is the recommended way to handle it so I am leaving it as is for now.
Handbook
Are you behind on catching up with your last few commits these days? Does having to connect to the internet to log onto your favourite cloud-based task manager bother you? Are you tired of manually formatting your tasks on a new note on your smartphone
Or is the thought of slow...ly dragging your mouse across the screen a few dozen times to update your weekly list of things to be done putting you off updating your weekly list of things to be done?
If so, Handbook is the right app for you!
Handbook is a task manager that is free, lightweight, and easy to use.
It is a heavily text-reliant application that seeks to meet the needs of the general advanced user.
What Handbook does
Do your work for youWhy Handbook
Here is what some of our users have to say about Duke
Installing Handbook
So, you've decided that you want to see what the hype is all about. What now?
All you need to do is
If you are a Java programmer, you can customize Handbook to your liking as well; give it your own personality. Here's a brief snippet of the
Ui
class: