Skip to content
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

[Michelle] Duke Increments #482

Open
wants to merge 47 commits into
base: master
Choose a base branch
from

Conversation

michelleykw
Copy link

No description provided.

@michelleykw michelleykw changed the title [Michelle Yong] Duke Increments [Michelle] Duke Increments Sep 13, 2019
String command = parser.getCommand(commandArr);
if (command.equals("todo")) {
try {
if (text.length() <= 4) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good guard clause!

String text = parser.nextCommand();
String[] commandArr = parser.breakDownCommand(text);
String command = parser.getCommand(commandArr);
if (command.equals("todo")) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you can try to abstract out the handler methods, and make use of switch?

} else if (command.equals("deadline")) {
try {
if (text.length() <= 8) {
throw new DukeException();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we consider specifying the error occurred here?

}
} else if (command.equals("deadline")) {
try {
if (text.length() <= 8) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe avoid using magic number here. Instead, you could use command.split(" ")[0].equals("deadline").

ui.printTaskAdded(deadline, taskList.getSize());
storage.appendTaskToFile(deadline);
} catch (DukeException e) {
ui.printDeadlineError();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try adding in the error message inside the DukeException class, and just call e.printMessage. Same for the rest of the code.

* @return The todo.
*/
public Todo getTodo(String line) {
String task = line.substring(5);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you have already split the command by breakDownCommand, maybe try making use of that abstraction?

* @return The task in a string format.
*/
private String convertTaskToFileFormat(Task task) {
StringBuffer textToAdd = new StringBuffer();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good use of StringBuffer to avoid extra overhead.

* @param list The list of tasks to be updated to the file.
* @throws IOException If an input or output exception occurred.
*/
public void updateTaskInFile(ArrayList<Task> list) throws IOException {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try using List instead of ArrayList, as programming to interface is a good practice.

* Contains the task list and updates tasks in the list.
*/
public class TaskList {
ArrayList<Task> list = new ArrayList<Task>();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be done in the class constructor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants