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

[Jerry Ho] Duke Increments #451

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

Conversation

itsjerryho
Copy link

@itsjerryho itsjerryho commented Aug 27, 2020

Include Commits for Week 2

Edit runtest.txt to javac all java files and replace code with symbols in Task.java
- saveDataToList is able to add tasks (in String form) to ArrayList
- Modify Deadline.java to store dates as a LocalDate object and time as a String.
- Update Deadline handling in Duke.java
…on and fix infinite loop bug.

- Fix bug that is preventing bye command from exiting the program
Copy link

@lerxcl lerxcl left a comment

Choose a reason for hiding this comment

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

LGTM. Overall, the code logic flows well. However, you should do more OOP and correct some of the syntax, good effort!

}

public static void printMessage(String s) {
String output = String.format("____________________________________________________________\n%s\n" +
Copy link

Choose a reason for hiding this comment

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

The "+" should be in the next line instead.

public static void saveToDisk(ArrayList<Task> lst) {
String filePath = "./data/data.txt";
String info = "";
for (int i = 0; i < lst.size(); i ++) {
Copy link

Choose a reason for hiding this comment

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

There should not be a spacing between "i" and "++".

import java.util.Scanner;
import java.util.ArrayList;
import java.io.File;

public class Duke {
Copy link

Choose a reason for hiding this comment

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

I have noticed that your Duke class may have too many methods (a little cluttered), and seeing that you have not merged "branch-A-OOP" into master, I am not sure whether you have create new classes to adhere to OOP principles?

String greetings = "Hello! I'm Duke, your personal assistant.\nWhat can I do for you?";
printMessage(greetings);

ArrayList<Task> lst = new ArrayList<>();
Copy link

Choose a reason for hiding this comment

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

A better practice might be to split the instantiation of "lst".

After your import statements, it could be List lst;
Then in your Duke constructor: this.lst = new ArrayList<>();

}
public static void mainLogic(Scanner sc, ArrayList<Task> lst) {

while (true) {
Copy link

Choose a reason for hiding this comment

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

It might not be too good of practice to use "while (true)", as it is hard to tell what is your terminating condition (and there is a chance of an infinite loop)?

@@ -0,0 +1,5 @@
public class DukeException extends Exception{
Copy link

Choose a reason for hiding this comment

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

A spacing after "Exception" will look nicer!

@@ -0,0 +1,14 @@
public class ToDo extends Task{
Copy link

Choose a reason for hiding this comment

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

Same as mentioned in DukeException class.

}

public String toText(String type) {
int doneInt = this.isDone ? 1 : 0;
Copy link

Choose a reason for hiding this comment

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

It might be better to use Enum to represent 1 and 0, as it might look like "magic numbers".

Copy link

@hyngkng hyngkng left a comment

Choose a reason for hiding this comment

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

I like your overall naming of methods, just have to ensure all of them are consistent with the coding standard! :)

}
return newTask;
}
public static void mainLogic(Scanner sc, ArrayList<Task> lst) {
Copy link

Choose a reason for hiding this comment

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

Perhaps the method name could have been a verb instead? Eg. parseCommand.

Choose a reason for hiding this comment

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

Yes, I also found this in other classes i.e. method invalidInput() in Duke class perhaps can also be changed to a verb?

itsjerryho and others added 30 commits September 8, 2020 11:14
- Command Line Interfaace is not working at the moment
- Remove scanner from Ui.java
- Add a few Todos
- Program previously exit without saving task to data.txt
- Edit Deadline and Event to extend TimedTask
- Remove AdditionalInfo and Common classes
- Add Specific Command Classes (Add, Delete, Done, Exit, Find, Invalid, List, PrintTask)
- Shift execute command logic out of Duke.java into the Command class itself
- Remove additionalInfo from Command class and add storage, taskList and userInput to Command class
- Create TimedTask, which is a super class for both Deadline and Event classes
- Shift main parsing logic out of Parser into specific Command classes
- Edit Storage class to handle change in Parsing style
- Create enum for Task.java
- Update Ui and remove Scanner and CLI related commands from Ui
- Remove AdditionalInfo from ParserTest (still in progress)
# Conflicts:
#	src/main/java/duke/AdditionalInfo.java
- Find is now able to search using keyword that is not case-sensitive
- Add JUnit tests in ParserTest
- Add TaskListTest
- Add padding
- Change photo of Duke
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants