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

[Ng Hong Liang] iP #277

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

[Ng Hong Liang] iP #277

wants to merge 37 commits into from

Conversation

jinnhl
Copy link

@jinnhl jinnhl commented Jan 27, 2022

Jeff the task manager

“My name is Jeff.” – Channing Tatum

Jeff allows you to have all your to-dos in one easy to access location. It's,

  • text-based 📝
  • easy to use 👌
  • fast SUPER FAST to use 🔥

How do I get Jeff? It's as easy as,

  1. download it from here.
  2. double-click it.
  3. add your task.

And voilà, Jeff would do everything for you!

Features:

  • Managing tasks
  • Managing deadlines
  • Reminders
  • Working GUI (coming soon)

Copy link

@DannyDakota DannyDakota left a comment

Choose a reason for hiding this comment

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

few suggestions to naming conventions and coding standards for switch cases. Otherwise, solid work handsome!

this.helper();
}

private LocalTime helper() {

Choose a reason for hiding this comment

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

maybe can use getTime here 😄

this.helper();
}

private LocalDate helper() {

Choose a reason for hiding this comment

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

maybe can use getDate here? 😄

* @param curr Holds the capacity for the stored list, used to check if request is out of bounds
* @throws DukeException
*/
public void checker(String[] arr, int curr) throws DukeException {

Choose a reason for hiding this comment

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

maybe can use a more descriptive name for the method 😄
e.g
commandChecker
stringChecker
inputTester

while (s.hasNext()) {
String[] inputLine = s.nextLine().split("\\| ", 4);
Task curr;
switch(inputLine[0]) {

Choose a reason for hiding this comment

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

switch cases shouldn't be indented according to the coding standard 😄
otherwise v nice use of switch to differentiate the various commands

Copy link

@pyk595 pyk595 left a comment

Choose a reason for hiding this comment

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

Overall, most of your code adheres to the coding standard, good job! Just a few minor nitpicks here and there

protected TimeParse time;

/**
* Constructor for Deadline class
Copy link

Choose a reason for hiding this comment

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

Might be better to put a line after the JavaDoc descriptor and the params?

@@ -1,10 +1,104 @@
import java.io.FileNotFoundException;
Copy link

Choose a reason for hiding this comment

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

Can consider putting your Duke.java in a package?

// If user inputs bye, terminate the program
if (input.equals("bye")) {
end = true;
}
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 in-line comments, they adhere to the coding standard's requirement as well

Choose a reason for hiding this comment

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

Agreed! I like how you use in-line comments to help in code readability and understanding the code :)

*/
public class ReadFile {

public static final String ADDRESS = "data/duke.txt";
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 full caps for constants!

fw.close();
}
//
// public static void main(String[] args) {
Copy link

Choose a reason for hiding this comment

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

Can consider removing these few lines of code if they are not used?

@@ -0,0 +1,31 @@
public abstract class Task {
protected String description;
protected boolean isDone;
Copy link

Choose a reason for hiding this comment

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

Good boolean variable naming!

case "T " :
curr = new Todo(inputLine[2]);
break;
case "D " :
Copy link

Choose a reason for hiding this comment

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

Same thing as mentioned by @DannyDakota, indentation should be removed


public static final String ADDRESS = "data/duke.txt";

public WriteFile(ArrayList<Task> store) throws IOException {
Copy link

Choose a reason for hiding this comment

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

Store might not be the best name for a task list, maybe naming it taskList might be a better choice?

Copy link

@Tiffanylin21 Tiffanylin21 left a comment

Choose a reason for hiding this comment

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

Overall, I found some minor issues in naming. Otherwise, I really like the in-line comments in the code :)

Comment on lines 1 to 4
import java.time.LocalDate;
import java.time.format.DateTimeParseException;
import java.util.ArrayList;
import java.time.format.DateTimeFormatter;

Choose a reason for hiding this comment

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

Perhaps consider separating similar ones into blocks?

this.dateInfo = this.date.toString() + " " + this.time.toString();
}

public String whatType() {

Choose a reason for hiding this comment

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

Perhaps consider naming the method with verbs such as 'getType()'?

System.out.println("Hello from\n" + logo);
Scanner scanner = new Scanner(System.in);
String input = "";
Boolean end = false;

Choose a reason for hiding this comment

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

Perhaps use a name that sounds more like a boolean? What about something like 'isEnd' or 'isExited'?

Scanner scanner = new Scanner(System.in);
String input = "";
Boolean end = false;
ArrayList<Task> store = new ArrayList(100);

Choose a reason for hiding this comment

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

Perhaps consider using a plural noun as the name of the ArrayList?

// While loop ends when user inputs bye
while(!end) {
input = scanner.nextLine();
String[] splitInput = input.split(" ", 2);

Choose a reason for hiding this comment

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

Perhaps consider using a plural noun as a variable name here?

// If user inputs bye, terminate the program
if (input.equals("bye")) {
end = true;
}

Choose a reason for hiding this comment

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

Agreed! I like how you use in-line comments to help in code readability and understanding the code :)

jinnhl and others added 10 commits February 18, 2022 02:35
Event and Deadline tasks does not check for assumptions that
string parsed as date and time are of correct size.

Event and Deadline tasks would cause Jeff to crash if certain
assumptions are not met.

Let's add assertions on those very variables have been added,
hence throwing an assertion failure instead of adding an
invalid entry into the list.

Assertions impact on performance is considered low compared
to its benefits.
Parser class have repeated conditional statements to check if the
user gave valid inputs.

To improve readability and code quality.

Let's refractor said conditional statements by
extract them into methods.

Easiest way to reuse code that have are duplicates of each other.
Allows easy navigation as they are on the same class file.
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