-
Notifications
You must be signed in to change notification settings - Fork 484
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
[vivienherq] iP #529
base: master
Are you sure you want to change the base?
[vivienherq] iP #529
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.
LGTM. Overall just some nits about coding standards!
src/main/java/Event.java
Outdated
this.from = from; | ||
this.to = to; | ||
} | ||
public String getType() { |
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 you can add a @Override
above your getType()
method to specify that you wish to override this method from Task
src/main/java/Dude.java
Outdated
|
||
// static Task[] taskList = new Task[100]; | ||
static ArrayList<Task> taskList = new ArrayList<Task>(); | ||
static int nTasks = 0; |
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.
Instead of having an nTasks
to keep track of the number of tasks in taskList
why not use taskList.size()
?
src/main/java/Dude.java
Outdated
public static void list() { | ||
for (int i = 0; i < nTasks; i++) { | ||
Task task = taskList.get(i); | ||
// Task task = taskList[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.
You are using comments to temporarily remove code, however comments should be used to describe the code to the reader. They should also be indented to the relative position to your code i.e. the //
needs to be on the same line as Task
on line 47
src/main/java/Dude.java
Outdated
|
||
public static void mark(int n) { | ||
// taskList[n-1].setDone(true); | ||
taskList.get(n-1).setDone(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.
For clarity, perhaps you can split this line up into
selectedTask = taskList.get(n-1);
selectedTask.setDone(true);
src/main/java/Dude.java
Outdated
String input = scanner.nextLine(); | ||
String[] words = input.split(" "); | ||
|
||
if (words[0].equals("list")) { |
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 you can use switch
statements for the control flow instead of if-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.
LGTM, just a few nits about coding standards. Overall, good job 😄
src/main/java/Dude.java
Outdated
@@ -0,0 +1,213 @@ | |||
import java.io.*; |
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.
Imported classes should be listed explicitly 😁
import java.io.*; | |
import java.io.BufferedReader; | |
import java.io.File; | |
import java.io.FileNotFoundException; | |
import java.io.FileReader; | |
import java.io.FileWriter; | |
import java.io.IOException; |
src/main/java/Task.java
Outdated
@@ -0,0 +1,35 @@ | |||
public class Task { | |||
private String description; | |||
private boolean done; |
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.
booleans variables should be named to sound like booleans
private boolean done; | |
private boolean isDone; |
src/main/java/Dude.java
Outdated
} | ||
|
||
public static void unmark(int n) throws IOException { | ||
taskList.get(n-1).setDone(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.
Operators should be surrounded by a space character.
taskList.get(n-1).setDone(false); | |
taskList.get(n - 1).setDone(false); |
src/main/java/Deadline.java
Outdated
import java.util.Locale; | ||
|
||
public class Deadline extends Task { | ||
LocalDateTime 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.
Variables should be non-public.
LocalDateTime by; | |
private LocalDateTime by; |
src/main/java/Dude.java
Outdated
static ArrayList<Task> taskList = new ArrayList<Task>(); | ||
static int nTasks = 0; | ||
|
||
private static final String FILE_PATH = "./data/dude.txt"; |
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 constant doesn't seem to be used. Did you forget to replace your instances of "./data/dude.txt"
with FILE_PATH
?
data/dude.txt
Outdated
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 file can be excluded from commits!
Add Gradle support
Add JUnit tests
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.
LGTM, minor fixes only.
src/main/java/dude/Ui.java
Outdated
|
||
import java.util.Scanner; | ||
public class Ui { | ||
Scanner sc; |
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.
Logical units within a block should be separated by one blank line.
Consider making the variable 'sc' private as well!
Add classes and methods to support Notes in Dude
Current structure is not very organised after adding new classes. New packages are added and classes are moved into packages. * dude.ui package added * dude.note package added
Null values are returned for Task and Note. Null values in loaded data can lead to unexpected behaviour and errors in the application.
Find command is case-sensitive. A case-insensitive find is more user-friendly because users cannot be expected to remember the exact case of the keywords. * Update search algorithm to use case-insensitive matching
Branch a better gui
Branch A-MoreErrorHandling
Branch A-Personality
Dude
Dude 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 are a Java programmer, you can use it to practice Java too. Here's the
main
method: