-
Notifications
You must be signed in to change notification settings - Fork 77
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
[Aaron Chan Zhi] iP #73
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.
Awesome job on finishing until Level 4! Great utilization of libraries!
src/main/java/ToDo.java
Outdated
@@ -0,0 +1,9 @@ | |||
public class ToDo extends Task { | |||
|
|||
public ToDo(String description){ |
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 adding spaces in between method name and inputs
src/main/java/Duke.java
Outdated
} | ||
} | ||
if (byPosition == 0){ | ||
System.out.println("Please state the deadline!"); |
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.
Good job on adding error handling!
src/main/java/Duke.java
Outdated
String cmd = input_list[0]; | ||
String rawInput = scanner.nextLine(); | ||
String[] inputList = rawInput.split(" "); | ||
String cmd = inputList[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.
Consider writing variable names in full to make it more readable
src/main/java/Duke.java
Outdated
System.out.println("---------------------------------------------------"); | ||
int atPosition = 0; | ||
String dateTime; | ||
for (int i=0; i<inputList.length; 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.
Consider adding spaces in between calculations
src/main/java/Duke.java
Outdated
|
||
while (isExit == false){ | ||
//Getting Input | ||
System.out.println("---------------------------------------------------"); |
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.
potentially add ---------... as a constant to increase readability of print statements.
src/main/java/Duke.java
Outdated
|
||
//Responding | ||
switch (cmd) { | ||
case ("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.
Avoid magic strings, so maybe for each of these cases, you can say for ex: LIST = "list" and reference the constant in the if statement
src/main/java/Duke.java
Outdated
System.out.println("Duke: Hello! What can I do for you today?"); | ||
|
||
|
||
while (isExit == 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.
Break this function into smaller helper functions :) to increase readability.
src/main/java/Event.java
Outdated
public Event(String description, String dateTime){ | ||
this.description = description; | ||
this.isDone = false; | ||
this.taskType = "E"; |
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.
make taskType a static final variable and instantiate it not in the constructor
src/main/java/Task.java
Outdated
} | ||
|
||
public void markAsNotDone(){ | ||
if (isDone==true) this.isDone = 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.
just do:
if (isDone) {
this.isDone = false;
}
src/main/java/TaskList.java
Outdated
public Task searchTask(String task_description){ | ||
for (int i=0; i<size; i++){ | ||
Task tempTask = this.getTask(i); | ||
if (tempTask.description.equals(task_description)) return tempTask; |
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.
make sure to do brackets after the if statement and a line break for "return tempTask"
src/main/java/TaskList.java
Outdated
for (int i=0; i<size; i++){ | ||
Task tempTask = this.getTask(i); | ||
String taskType = tempTask.getTaskType(); | ||
if (taskType == "T"){ |
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.
Magic String, maybe set "T" as a constant TASK = "T" so its easier for readers to know that T corresponds to 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.
Some repeated mistakes in the spacing and can make code more readable by putting some code in a new method.
String description; | ||
|
||
//Responding | ||
switch (cmd) { |
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 do the printing in another method to make the code look cleaner
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.
Indentation not needed for switch and case statements
src/main/java/Duke.java
Outdated
System.out.println("Duke: Hello! What can I do for you today?"); | ||
|
||
|
||
while (isExit == 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.
Add a space before opening the braces for better styling.
src/main/java/Duke.java
Outdated
System.out.print("You: "); | ||
String rawInput = scanner.nextLine(); | ||
String[] inputList = rawInput.split(" "); | ||
String cmd = inputList[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.
Name of variable can be improve, for example rather than cmd, it can be command
src/main/java/TaskList.java
Outdated
for (int i=0; i<size; i++){ | ||
Task tempTask = this.getTask(i); | ||
String taskType = tempTask.getTaskType(); | ||
if (taskType == "T"){ |
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.
All the printing can be done in a method to prevent repetition
src/main/java/Deadline.java
Outdated
public class Deadline extends Task { | ||
private String dueDate; | ||
|
||
public Deadline(String description, String dueDate){ |
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.
There should be spacing before braces (applies the whole file)
src/main/java/Task.java
Outdated
} | ||
|
||
public void markAsDone() { | ||
if (isDone==false) this.isDone = 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.
Add spaces before and after the logical operator so that it looks better
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.
There are no major coding standard violations. Good job!
src/main/java/Duke.java
Outdated
public class Duke { | ||
public static void main(String[] args) { | ||
boolean isExit = 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.
An alternative name can be used for the boolean isExit so that the purpose of the boolean is clearer
String description; | ||
|
||
//Responding | ||
switch (cmd) { |
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 switch statement should be aligned with the case statements.
src/main/java/Duke.java
Outdated
System.out.println("---------------------------------------------------"); | ||
int byPosition = 0; | ||
String dueDate; | ||
for (int i=0; i<inputList.length; 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 could consider adding spaces before and after comparison operators to improve readability
src/main/java/Duke.java
Outdated
System.out.print("You: "); | ||
String rawInput = scanner.nextLine(); | ||
String[] inputList = rawInput.split(" "); | ||
String cmd = inputList[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.
You could consider renaming the variable to "command" so that it is clearer to the reader
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 work on the iP, you can further improve by adding onto the consistency of aspects such as layout of your code generally!
src/main/java/Duke.java
Outdated
System.out.println("---------------------------------------------------"); | ||
} | ||
|
||
private static String getStringFromList(String[] inputList, int fromIndex, int toIndex) throws IllegalArgumentException{ |
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 maintain consistency in adding a space before the opening brace of function declarations.
src/main/java/FileHandler.java
Outdated
|
||
public static void initFiles(){ | ||
File dataDir = new File("./DataDir"); | ||
if (!dataDir.exists()) dataDir.mkdir(); |
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 enclosing the dataDir.mkdir()
in braces as stated in the coding standard to maintain consistency and readibility.
src/main/java/Duke.java
Outdated
case ("event"): | ||
printLine(); | ||
int atPosition = 0; | ||
String dateTime; | ||
for (int i=0; i<inputList.length; i++){ | ||
if (inputList[i].equals("at")){ | ||
atPosition = i; | ||
break; | ||
} | ||
} | ||
|
||
//Handle invalid input | ||
try{ | ||
description = getStringFromList(inputList, 1, atPosition); | ||
dateTime = getStringFromList(inputList, atPosition+1, inputList.length); | ||
} catch (IllegalArgumentException e){ | ||
System.out.println("Invalid input: task/date not given!"); | ||
break; | ||
} | ||
|
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.
In such situations where a particular feature may require extensive code to achieve, it may be better for readibility to abstract the relevant blocks of code to separate functions (SLAP)
src/main/java/FileHandler.java
Outdated
if (!dataDir.exists()) dataDir.mkdir(); | ||
|
||
try{ | ||
File dataFile = new File("./DataDir/Data.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.
It would be great to eliminate the use of magic strings, in this case, consider replacing this with a variable or static variable instead!
src/main/java/FileHandler.java
Outdated
if (taskType == "T"){ | ||
String line = "T | [ ] | " + task.description + "\n"; | ||
myWriter.write(line); | ||
Lines.add(line); | ||
} else if (taskType == "E"){ | ||
String line = "E | [ ] | " + task.description + " | " + task.getDateTime() + "\n"; | ||
myWriter.write(line); | ||
Lines.add(line); | ||
} else if (taskType == "D"){ | ||
String line = "D | [ ] | " + task.description + " | " + task.getDueDate() + "\n"; | ||
myWriter.write(line); | ||
Lines.add(line); | ||
} | ||
|
||
myWriter.close(); | ||
} catch (IOException e) { | ||
System.out.println("An error occurred."); | ||
return; | ||
} | ||
} |
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.
It would be good to ensure consistency by including a space after the if-else condition statements for each block, as you have done below.
src/main/java/Duke.java
Outdated
|
||
case ("delete"): | ||
printLine(); | ||
int tasknumber = Integer.parseInt(inputList[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.
You can consider naming the variable taskNumber
instead, in adherence to good naming practices of applying camel case for your variable names
src/main/java/TaskList.java
Outdated
for (int i=0; i<size; i++){ | ||
index = i; | ||
Task tempTask = this.getTask(i); | ||
if (tempTask.description.equals(task_description)) return index; |
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.
It would be great to enclose the if-else block in multiple lines, and add braces to return index;
in this case, to better adhere to coding standards
src/main/java/FileHandler.java
Outdated
public static void markAsDone(int index){ | ||
String oldLine = Lines.get(index); | ||
String newLine = oldLine.substring(0,5) | ||
+ "X" | ||
+ oldLine.substring(6, oldLine.length()); | ||
Lines.set(index, newLine); | ||
|
||
try{ | ||
//Erase Data | ||
File dataFile = new File("./DataDir/Data.txt"); | ||
FileWriter myWriter = new FileWriter(dataFile); | ||
myWriter.write(""); | ||
myWriter.close(); | ||
|
||
//Replace Data | ||
dataFile = new File("./DataDir/Data.txt"); | ||
myWriter = new FileWriter(dataFile, true); | ||
for (int i=0; i < Lines.size(); i++){ | ||
myWriter.write(Lines.get(i)); | ||
} | ||
myWriter.close(); | ||
} catch (IOException e){ | ||
System.out.println("An error occurred."); | ||
return; | ||
} | ||
} | ||
|
||
public static void markAsNotDone(int index){ | ||
String oldLine = Lines.get(index); | ||
String newLine = oldLine.substring(0,5) | ||
+ " " | ||
+ oldLine.substring(6, oldLine.length()); | ||
Lines.set(index, newLine); | ||
|
||
try{ | ||
//Erase Data | ||
File dataFile = new File("./DataDir/Data.txt"); | ||
FileWriter myWriter = new FileWriter(dataFile); | ||
if (!dataFile.exists()) dataFile.createNewFile(); |
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 there is a good amount of repeated logic within this two functions, perhaps you may want to consider further abstracting out some of the repeated functions with a helper function to improve code readibility?
Branch level 9
Branch-UG
Add: Feature List
Fix: Feature List new lines
Add: list
Added: all features
Fix: small errors in formatting
Add: intro paragraph
No description provided.