[Pranav Ganesh] iP by pranav-ganesh · Pull Request #477 · nus-cs2103-AY2122S1/ip

pranav-ganesh · 2021-08-26T14:05:35Z

🔥 DUKE 🔥

“Your mind is for having ideas, not holding them.” – David Allen source

Duke frees your mind of having to remember things you need to do. It's,

text-based
easy to learn
~~FAST~~ SUPER FAST to use

All you need to do is,

download it from here.
double-click it.
add your tasks
let it manage your tasks for you 😃 😊

And it is FREE!

Duke's features:

Managing features
Managing deadlines (coming soon)
Reminders (coming soon)

If you Java programmer, you can use it to practice Java too. Here's the main method:

public static void main(String[] args) {
    new Duke("./duke.txt").run();
}

Merge branch branch-Level-7 into Master

yongxiangng

There are many Javadocs missing, it would be good to write header comments for all public classes and methods see here

It might also be good to be consistent with the number of new lines at the end of your files rather than no or dangling new lines. I believe it is good to have a single new line at the end.

There are several places where System.out.println was directly used, possibly violating the abstraction from Ui.printMessage (I guess this might not really matter once you start implementing the GUI, you wouldn't be able to use System.out.println to directly print to your UI)

yongxiangng · 2021-08-30T01:28:00Z

@@ -0,0 +1,60 @@
+package duke;
+
+import duke.command.*;


You might want to avoid wild card imports even if you have to import everything
https://se-education.org/guides/conventions/java/intermediate.html#package-and-import-statements

For the parse method below

Perhaps a switch statement here would help to improve readability or you could modify the code by splitting the string by the empty character delimiter twice so you don't have to do another if inside the external if

Maybe your InvalidCommand can be an exception, and your doneCommand can throw the exception so you don't have to do all the checking within this method which could make the code easier to read

https://www.geeksforgeeks.org/split-string-java-examples/

https://nus-cs2103-ay2122s1.github.io/website/se-book-adapted/chapters/codeQuality.html#guideline-maximise-readability

yongxiangng · 2021-08-30T01:31:08Z

+        }
+    }
+
+    public void loadTaskListData(TaskList taskList) {


You may want to document public methods with javadocs
https://se-education.org/guides/conventions/java/intermediate.html#comments

The method here is also quite lengthy. It might be a good idea to abstract the other logic into various methods
https://nus-cs2103-ay2122s1.github.io/website/se-book-adapted/chapters/codeQuality.html#avoid-long-methods

yongxiangng · 2021-08-30T01:35:13Z

+                    System.out.println(str);
+                    String[] parts = str.split("\\|", 4);
+                    String subStr = parts[0].substring(3).trim();
+                    if (subStr.equals("duke.task.Todo") || subStr.equals("duke.task.Todo")) {


I might be wrong here but doesn't subStr.equals("duke.task.Todo") || subStr.equals("duke.task.Todo") do the same as subStr.equals("duke.task.Todo")

yongxiangng · 2021-08-30T01:39:42Z

+    public void loadTaskListData(TaskList taskList) {
+        try {
+            Scanner s = new Scanner(this.file); // create a Scanner using the File as the source
+            if (!s.hasNext()) {


The logic here is quite deeply nested, perhaps it might be good if you handled the unusual case with a guard clause
https://nus-cs2103-ay2122s1.github.io/website/se-book-adapted/chapters/codeQuality.html#make-the-happy-path-prominent

if (!s.hasNext()) { // code here... return; }

yongxiangng · 2021-08-30T01:44:50Z

+                            task.markAsDone();
+                        }
+                        taskList.addTask(task);
+                    } else {


You may want to use a switch statement for this long if else. It may be good because you're comparing strings and it can directly be used for your cases.

Additionally the else statement here captures all other cases of subStr, not necessarily when it is an Event. If new requirements come in and there are new type of Task, the developer might forget to update the logic here and the new type of Task will automatically fall into the else case and treated like an Event.

The default case in a switch statement could be used to catch invalid cases. I believe the default case is optional in Java
https://www.geeksforgeeks.org/switch-statement-in-java/

The style guide suggests that

Always include a default branch in case statements. Furthermore, use it for the intended default action and not just to execute the last option. If there is no default action, you can use the default branch to detect errors (i.e. if execution reached the default branch, raise a suitable error). This also applies to the final else of an if-else construct. That is, the final else should mean 'everything else', and not the final option. Do not use else when an if condition can be explicitly specified, unless there is absolutely no other possibility.

Linxcathyyy

Good job overall! Just one thing to take note of: try to use small and simple functions to handle your complex logic, don't put everything into one function. It makes readers confusing and it is also hard to do unit testing~

Linxcathyyy · 2021-08-30T09:09:58Z

+        this.command = command;
+    }
+
+    public Command parse() {


Perhaps try not to write deeply nested loops in your function? You might consider using 'switch' for this function?☺️

Linxcathyyy · 2021-08-30T09:18:18Z

+        }
+    }
+
+    public void loadTaskListData(TaskList taskList) {


You might want to break it down into smaller functions here? This function seems to be too deeply nested.

SKCheongBrian

Good job! Your code seems very well done.

SKCheongBrian · 2021-09-01T06:46:16Z

+        } else {
+            if (command.startsWith("todo")) {
+                return new todoCommand(command);
+            } else if (command.startsWith("deadline")) {
+                return new deadlineCommand(command);
+            } else if (command.startsWith("event")) {
+                return new eventCommand(command);
+            } else if (command.startsWith("delete")) {
+                return new deleteCommand(command);
+            } else {
+                return new InvalidCommand(command);    


You can consider using a switch statement here 😄

SKCheongBrian · 2021-09-01T06:53:24Z

+    public static void doneResponse(Task task) {
+        System.out.println("Nice! I've marked this task as done:");
+        System.out.println(task);
+        storage.writeToFile("./duke.txt", taskList);
+    }


Good idea to write to file everytime a task is edited. This way the state of the tasks are not lost incase the app crashes.

SKCheongBrian · 2021-09-01T06:57:27Z

+            Parser parser = new Parser(command);
+            if (parser.isExit()) {
+                break;
+            }
+            Command c = parser.parse();


You can consider making parse() a static method, that way you would not have to create a new parser object everytime a command is given.

Merge branch-A-JavaDoc into Master

Merge branch-Level-9 into Master

Merge add-gradle-support into Master

Smanmos

There were some violations of code (in particular class name capitalization). There was also a questionable design decision of writing to the file every time a new task was inserted or deleted, which can case performance issues.

But overall, it's a good program design.

Smanmos · 2021-09-05T13:37:16Z

+        } else if (command.startsWith("done") && Character.isDigit(command.charAt(command.length() - 1)) && command.length() <= 8 
+                && !Character.isAlphabetic(command.charAt(command.length() - 2))  && Character.isDigit(command.charAt(5))) {
+            return new doneCommand(command);


This if block's condition could be simplified, with only the command type being checked in the main condition, and the other checks could be further done inside the body.

Smanmos · 2021-09-05T13:37:55Z

+    /**
+     * Checks if the user types the command 'bye' or the user clicks enter without typing any command.
+     *
+     * @return True or False.


The return value could be further elaborated.

Smanmos · 2021-09-05T13:39:01Z

+ * Deals with making sense of the user command
+ */
+public class Parser {
+    private String command;


The string name command can be confusing, because a class called Command is in this project, but command is not a Command object.

Maybe change it to commandString?

Smanmos · 2021-09-05T13:49:28Z

+    private static TaskList taskList;
+    private static Storage storage;


Why are taskList and storage static?

Smanmos · 2021-09-05T13:51:04Z

+/**
+ * Represents the user command when the user enters a deadline.
+ */
+public class deadlineCommand extends Command {


Class name should be PascalCase: DeadlineCommand

Smanmos · 2021-09-05T13:56:12Z

+/**
+ * Represents the user command when the user enters an event.
+ */
+public class eventCommand extends Command {


Class name should be PascalCase: EventCommand

Smanmos · 2021-09-05T13:56:49Z

+
+import duke.Ui;
+
+public class findCommand extends Command {


Class name should be PascalCase: FindCommand

Smanmos · 2021-09-05T13:57:35Z

+        Ui.printMessage("Here are the matching tasks in your list:");
+        String[] parts = this.command.split(" ", 2);
+        String wordToFind = parts[1];
+        int count = 1;


count should start at 0 instead of 1, to make things more intuitive

Smanmos · 2021-09-05T13:57:56Z

+/**
+ * Represents the user command when the user wants to view the task list.
+ */
+public class listCommand extends Command {


Class name should be PascalCase: ListCommand

Smanmos · 2021-09-05T14:00:28Z

+ * An abstract class that represents the various types of commands that can be entered by the user.
+ */
+public abstract class Command {
+    private String command;


Perhaps consider making command protected instead of private? It would make the code much cleaner since subclasses make use of this variable a lot

Merge branch-Level-10 into Master

None of the existing classes have assertions to detect the source of logical flaws in the code. The addition of assertions will increase the probability of identifying bugs in the program that are caused by violation of certain assumptions in the program logic. Let's add assertions to the relevant parts of the code to define certain assumptions about the program state. Using assertions is greatly advantageous as assertions are easy to implement and help document and verify key assumptions that should hold at various points in the code.

Add assertions to document important assumptions in the code

There are several instances in the code where code quality can be further enhanced. Good quality of code makes sure that code is written in such a way that makes them highly readable by a human. Let's improve the code quality by critically examining the code and refactoring parts of it to make it more readable and make sure it complies with the code quality commonly practiced guidelines. Doing it this way will help eliminate common code quality problems such as weak SLAP, arrow-head style code, too-long methods and too-deep nesting.

damithc and others added 26 commits July 29, 2021 20:30

Add Gradle support

d839859

Finished Level-1

244ea63

Finished Level-2

05b4e5e

Finished Level-3

bad6e71

Finished Level-4

15cfb00

Finished A-TextUiTesting

6a1c9de

Finished Level-5

667a5bd

Finished Level-6

57c60d1

Comply with Java coding standard

e70d0aa

Finish Level-7

80299be

Finish Level-8

cfad495

Add line wrapping at appropriate places

10b01bb

Merge branch 'branch-Level-7'

2c44649

Merge branch branch-Level-7 into Master

Merge branch branch-Level-8 into Master

f7e6d0a

Add new files for individual classes in Duke.java

7ae663c

Add TaskList class

c6035cd

Add Storage class

a5f3c25

Add Command class and relevant subclasses

8026b89

Enhance the functionality of the Storage class

9605358

Add Ui class

4537188

Finish A-MoreOOP

068f340

Add A-Packages

493460f

Add JUnit Test for the Parser class

8479f34

Add JUnit Test for the DukeException class

02e7e75

Add JUnit Test for the TaskList class

8381e30

Finish A-JUnit

be2314a

yongxiangng reviewed Aug 30, 2021

View reviewed changes

Linxcathyyy reviewed Aug 30, 2021

View reviewed changes

Modify .gitignore file

ce9da73

SKCheongBrian reviewed Sep 1, 2021

View reviewed changes

e0661424 added 9 commits September 1, 2021 19:24

Finish A-CodingStandard

1e3a89f

Finish A-JavaDoc

92529e0

Finish Level-9

11a840b

Merge branch 'branch-A-JavaDoc'

15859ea

Merge branch-A-JavaDoc into Master

Merge branch-A-CodingStandard into Master

528b73e

Merge branch 'branch-Level-9'

8621cd4

Merge branch-Level-9 into Master

Finish Level-9 (Latest)

24169f9

Merge remote-tracking branch 'origin/add-gradle-support'

1e26b45

Merge add-gradle-support into Master

Modify project directory structure

013d465

Smanmos reviewed Sep 5, 2021

View reviewed changes

e0661424 and others added 15 commits September 10, 2021 00:35

Add branch-Level-10

f01545c

Merge branch 'branch-Level-10'

a84441a

Merge branch-Level-10 into Master

Finish Level-10

c2dfd54

Enhance exception handling

815d295

Merge branch-A-Assertions into master

956c0a1

Add assertions to document important assumptions in the code

Merge branch-A-CodeQuality into master

bf16d73

Add C-Update

c2cbb49

Add Ui.png and enhance GUI

8afc4ce

Add User Guide

2283f09

Add Javadoc to more files

b93000a

Add final touches to the code

5802939

Update User Guide

3848c56

Further Update User Guide

7668565

		private static TaskList taskList;
		private static Storage storage;

Uh oh!

Conversation

pranav-ganesh commented Aug 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔥 DUKE 🔥

Uh oh!

yongxiangng left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yongxiangng Aug 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Linxcathyyy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SKCheongBrian left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Smanmos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

pranav-ganesh commented Aug 26, 2021 •

edited

Loading

yongxiangng Aug 30, 2021 •

edited

Loading