-
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
[Nevin Lim] iP #270
base: master
Are you sure you want to change the base?
[Nevin Lim] iP #270
Conversation
This reverts commit ba73214.
src/main/java/Deadline.java
Outdated
@@ -0,0 +1,14 @@ | |||
public class Deadline extends Task { | |||
|
|||
protected String by; |
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 to use private modifier here? Or is there a particular reason to use protected?
src/main/java/Duke.java
Outdated
System.out.println("____________________________________________________________\n" + | ||
"Nice! I've marked this task as done:\n" + | ||
" " + tasks.get(num - 1).toString() + "\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.
I think you could make the line breaks private fields so that you don't need to type so many times
src/main/java/Duke.java
Outdated
public class Duke { | ||
private static final String greeting = "____________________________________________________________\n" + "Hello! I'm Duke\n" + "What can I do for you?\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.
I think for static final fields, the variable name should all be in uppercase:)
src/main/java/Duke.java
Outdated
public class Duke { | ||
private static final String greeting = "____________________________________________________________\n" + "Hello! I'm Duke\n" + "What can I do for you?\n" + "____________________________________________________________"; | ||
private static final String goodbye = "____________________________________________________________\n" + "Bye. Hope to see you again soon!\n" + "____________________________________________________________"; | ||
private static boolean active = 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.
perhaps rename to isActive to make the variable sound more boolean?
src/main/java/DukeException.java
Outdated
@@ -0,0 +1,7 @@ | |||
import java.util.*; |
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 specify the class being imported instead of just importing *
# Conflicts: # src/main/java/Duke.java
# Conflicts: # data/duke.txt # src/main/java/duke/main/Parser.java
|
||
import java.io.IOException; | ||
|
||
public class MarkCommand extends Command { |
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.
need javadoc
|
||
import java.io.IOException; | ||
|
||
public class MarkCommand extends Command { |
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 don't think it makes sense to have these as classes. methods make more sense
} | ||
|
||
public void execute(Tasklist tasklist, Ui ui, Storage storage) throws IOException { | ||
tasklist.markDone(this.taskNum - 1); |
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.
tasklist -> taskList
src/main/java/duke/main/Parser.java
Outdated
import duke.command.*; | ||
import duke.task.Deadline; | ||
import duke.task.Event; | ||
import duke.task.Todo; |
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.
should split the imports
src/main/java/duke/main/Parser.java
Outdated
* @throws DukeException when input is invalid | ||
*/ | ||
public static Command parseCommand(String fullCommand) throws DukeException { | ||
String[] splits = fullCommand.split(" ", 2 ); |
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 a very indicative name for this array
src/main/java/duke/main/Parser.java
Outdated
throw new DukeException("☹ OOPS!!! Please use format: event <description> /from <datetime> /to <datetime>"); | ||
} | ||
try { | ||
String[] secondsplits = splits[1].split("/from", 2); |
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.
variable naming standards
src/main/java/duke/main/Storage.java
Outdated
StringBuilder strings = new StringBuilder(); | ||
FileWriter fw = new FileWriter(this.file); | ||
|
||
for (Task curr : tasklist.getTasks()) { |
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 is a complicated expression. Can be simplified
src/main/java/duke/main/Storage.java
Outdated
.append(curr.getDescription()) | ||
.append("\n"); | ||
} else if (curr instanceof Deadline) { | ||
strings.append("D ") |
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 very readable
src/main/java/duke/main/Ui.java
Outdated
printLine(); | ||
} | ||
|
||
public void printFoundTasks(ArrayList<Task> foundTasks) throws DukeException { |
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.
missing javadoc
import duke.main.Ui; | ||
|
||
public class ExitCommand extends Command { | ||
public void execute(Tasklist taskList, Ui ui, Storage storage) { |
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.
nota very indicative name for the method
# Conflicts: # src/main/java/duke/command/AddCommand.java
Removed duplicated getTasksNum() method from Tasklist and did some cosmetic changes to several other classes.
Added the ability to recognize and deal with duplicate tasks from user.
Added *personality* to newly named Smudge program. Added new help command too
Duke bot
Duke frees your mind of having to remember things you need to do. It's:
All you need to do is,
And it is FREE❗ ❗ 😃
Features:
If you Java programmer, you can use it to practice Java too. Here's the
main
method: