-
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
[kimberlybp] iP #245
base: master
Are you sure you want to change the base?
[kimberlybp] iP #245
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've noticed that your Ui class is not being used properly as some classes are still printing to console without calling the Ui class. Overall, your code looks good to me!
this.value = value; | ||
} | ||
|
||
public boolean equals(String input) { |
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 the methods should be called isEqual
?
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 isEqual
is a good idea!
src/main/java/duke/Parser.java
Outdated
import commands.*; | ||
import exceptions.DukeException; | ||
import tasks.*; |
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.
Shouldn't wildcard imports be avoided?
src/main/java/duke/Storage.java
Outdated
import tasks.*; | ||
|
||
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.
Shouldn't wildcard imports be avoided?
src/main/java/duke/Storage.java
Outdated
case "T": | ||
initList.add(new Todo(taskDesc, isCompleted)); | ||
break; | ||
case "D": | ||
initList.add(new Deadline(taskDesc, isCompleted, strArr[3])); | ||
break; | ||
case "E": | ||
initList.add(new Event(taskDesc, isCompleted, strArr[3], strArr[4])); | ||
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.
Shouldn't there be no indentation between switch and case?
src/main/java/duke/Duke.java
Outdated
import java.util.Scanner; | ||
|
||
public class Duke { | ||
private static final String PATH = "data/Duke.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.
Nice use of capitalization for constants
this.value = value; | ||
} | ||
|
||
public boolean equals(String input) { |
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 isEqual
is a good idea!
src/main/java/duke/Parser.java
Outdated
} | ||
|
||
public static Command parse(String input) throws DukeException { | ||
String[] inputArr = input.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.
Perhaps we can better understand this String array variable by naming it inputs
or inputTokens
?
src/main/java/duke/Parser.java
Outdated
throw new DukeException("Hey now.. The description of a deadline cannot be empty. >:("); | ||
} | ||
taskInput = inputArr[1]; | ||
String[] dArr = taskInput.split("/by", 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.
The naming of this array might not be immediately evident on what it is supposed to be, because of the short name. Perhaps we can use a more precise name?
src/main/java/duke/Storage.java
Outdated
} | ||
|
||
public List<Task> loadFile() throws Exception { | ||
List<Task> initList = new ArrayList<>(); |
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 we can rename this Collection variable to be tasks
as it is a collection of Tasks?
Previous build.gradle file had duke.Duke as mainClassName, however due to GUI addition to duke, it needs to be updated to duke.Launcher to launch GUI Update mainClassName to duke.Launcher For JAR files to build and launch the main method of the correct class
Code can be further improved by applying code quality guidelines Refactor and apply guidelines such as SLAP, KISS. Also rename certain variables for better readability
Add Assertions
Improve code quality
Sirius
Sirius is a CLI app that helps you manage your time. It's,
FASTSUPER FAST to useIn order to use it..
Features
Devs feel free and play around with the code
Here's a preview of the
main
method: