[Galen Cheung] iP by cheunggalen · Pull Request #238 · nus-cs2103-AY2021S2/ip

cheunggalen · 2021-01-28T15:56:47Z

No description provided.

# Conflicts: # src/main/java/Deadline.java # src/main/java/DeadlineException.java # src/main/java/DukeException.java # src/main/java/Event.java # src/main/java/EventException.java # src/main/java/Todo.java # src/main/java/TodoException.java # src/main/java/UnknownException.java

# Conflicts: # src/main/java/Parser.java

noelmathewisaac · 2021-02-04T04:35:08Z

src/main/java/Event.java

+        } else {
+            if (onesHour.equals(0)) {
+                end += "12:";
+            } else {
+                end += onesHour.toString() + ":";
+            }
+        }


Perhaps you can consider using else if and else statements here instead of a nested if-else to improve code readability? I noticed the same in other places too.

I will check and correct them, thanks.

noelmathewisaac · 2021-02-04T04:41:05Z

src/main/java/Duke.java

-                + "| |_| | |_| |   <  __/\n"
-                + "|____/ \\__,_|_|\\_\\___|\n";
-        System.out.println("Hello from\n" + logo);
+        new Duke("./data/duke.txt").run();


I like how you used a relative path here 👍

noelmathewisaac · 2021-02-04T04:52:00Z

src/main/java/Ui.java

+     *
+     * @param statement The new response that will replace the current one.
+     */
+    public void replace(String statement) {


Should this function be renamed to 'replaceResponse' or 'newResponse' to make the intent of the function clearer? I wasn't sure what 'replace; did when I encountered the function in the Parser.java file.

I will change it, thanks.

noelmathewisaac · 2021-02-04T05:16:09Z

src/main/java/Parser.java

+                String[] line = command.split(" ");
+                int length = line.length;
+                if (length == 1) {
+                    throw new IncompleteException("The description for Todo cannot be empty.");
+                }
+                ui.replace("Got it. I've added this task:");
+                String description = "";
+                for (int i = 1; i < length; i++) {
+                    if (i + 1 == length) {
+                        description += line[i];
+                    } else {
+                        description += line[i] + " ";
+                    }
+                }


Should the logic used extract out the description be simplified to make it more understandable? Perhaps instead of splitting on spaces and combing all the words after todo, you can simply remove the word 'todo' from the String? This will make require only a single line of code and is much more compact.

Something like:

String description = line.replace("todo", "").strip() //.strip() is to remove trailing spaces

The same can be applied in other parts of your code for descriptions of deadlines and events.

Ah yes, that will make it a lot easier, thanks!

noelmathewisaac · 2021-02-04T05:24:20Z

src/main/java/TaskList.java

+     * @param index Position of the to-be-replaced task in the list.
+     * @oaram task The new task that will replace the current one.
+     */
+    public void replaceTask(int index, Task task) {


I like the intuitive naming of all the functions in this class.

noelmathewisaac · 2021-02-04T05:36:23Z

src/main/java/Storage.java

+            BufferedReader br = new BufferedReader(new FileReader(this.filePath));
+            String input = br.readLine();
+            String type = "";
+            int length = 0;
+            int lastIndex = 0;
+            String timeWithBracket = "";
+            int twbLength = 0;
+            int twbLastIndex = 0;
+            String year = "";
+            String yearLine = "";
+            int descLastIndex = 0;
+            Task task = null;
+            String description = "";
+            String date = "";
+            String time = "";


Perhaps you can consider abstracting away the formatting/extraction of dates/times by creating a separate class for functions related to these? There are a lot of variables declared here and abstracting these details will make the code easier to understand.

Will take note, thanks.

noelmathewisaac · 2021-02-04T05:44:29Z

src/main/java/Task.java

+    public void formatTime() {
+        String copy = this.time;
+        char[] cArr = copy.toCharArray();
+        Integer tensHour = cArr[0] - '0';
+        Integer onesHour = cArr[1] - '0';
+        Integer tensMin = cArr[2] - '0';
+        Integer onesMin = cArr[3] - '0';
+        String formattedTime = "";
+        boolean isAfternoon = false;
+        if (!tensHour.equals(0)) {
+            Integer combinedHour = tensHour * 10 + onesHour;
+            if (combinedHour >= 12) {
+                isAfternoon = true;
+                if (combinedHour >= 13) {
+                    combinedHour -= 12;
+                }


Should you leverage the LocalDate or some other library to convert the date and time format? I feel that doing so will make the code less cluttered and save you the trouble of writing everything from scratch. This can also be applied to all the classes which inherit from Task.

Maybe consider something along the lines of this tutorial?

Noted, thanks for sharing the link!

noelmathewisaac

Overall I think that the code largely adheres to the coding standards. Making the changes mentioned by abstracting out more functions and using a library for date/time conversions/formatting will improve the readability and maintainability of the code.

gycc7253

haha nice reviewing your code, good work buddy!

gycc7253 · 2021-02-06T01:51:09Z

src/main/java/Parser.java

+                    Integer taskNum = i + 1;
+                    ui.replace(taskNum.toString() + "." + taskList.getTask(i).toString());
+                }
+            } else if (command.contains("todo")) {


If i have a command
'event todo a task /by tmr'
this string contains 'todo' but it is actually event...
i think testing prefix maybe safer?

Noted, will change it, thanks.

gycc7253 · 2021-02-06T01:53:21Z

src/main/java/Storage.java

+                        year += yearArr[i];
+                    }
+                    date = inputArr[lastIndex - 3] +
+                            " " + inputArr[lastIndex - 2] + " " + year;


i think over here, by coding standard, the + should be the start of the preceding line hh,
same mistake made bro.

Oh yes, thanks for pointing it out.

gycc7253 · 2021-02-06T01:54:10Z

src/test/java/TaskTest.java

@@ -0,0 +1,18 @@
+import org.junit.jupiter.api.Test;
+import static org.junit.jupiter.api.Assertions.assertEquals;


could you please double check if static import needs to be imported before non-static, i think so but i am not 100% sure haha

gycc7253 · 2021-02-06T01:55:01Z

src/main/java/Storage.java

+import java.io.FileWriter;
+import java.io.IOException;
+
+public class Storage {


public class and methods undocumented bro, small issue, maybe u will do it later haha.

gycc7253 · 2021-02-06T01:55:24Z

src/main/java/Event.java

+ * A type of Task.
+ */
+public class Event extends Task {
+    public Event(String description, String date, String time) {


i think public constructor needs a documentation also

Noted, thanks.

gycc7253 · 2021-02-06T01:56:26Z

src/main/java/Duke.java

+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.io.IOException;


maybe import of java.io should be in front of java.nio/util coz of lexicographic order

Will correct it, thanks.

Add branch-A-Assertions to master

* 'master' of https://github.com/cheunggalen/ip: Add A-Assertions

# Conflicts: # src/main/java/Parser.java

Add branch-A-CodeQuality to master

* 'master' of https://github.com/cheunggalen/ip: Add A-CodeQuality

Add C-DetectDuplicates

* 'master' of https://github.com/cheunggalen/ip: Add C-DetectDuplicates

* 'master' of https://github.com/cheunggalen/ip: Set theme jekyll-theme-midnight

* 'master' of https://github.com/cheunggalen/ip: Set theme jekyll-theme-cayman

cheunggalen added 15 commits January 21, 2021 23:38

Add level 1-6

6a34db9

Add test files

fd47a56

Add Level-7

673692a

Add Level-8

faefe9a

Merge branch 'branch-Level-8'

6c3d616

Add A-MoreOOP

ad9bbe1

Add JUnit Tests for Task and TaskList

da4bde2

Add A-JavaDoc

40a6901

Add A-CodingStandard

a523da4

Add Level-9

7e95fd7

Merge branch 'branch-Level-9'

5b68ef4

# Conflicts: # src/main/java/Parser.java

Add corrected Level-7

d250ecc

Add A-Gradle

5acb7f9

Add modified A-Gradle

ba449dd

noelmathewisaac reviewed Feb 4, 2021

View reviewed changes

gycc7253 reviewed Feb 6, 2021

View reviewed changes

cheunggalen and others added 6 commits February 6, 2021 19:08

Add Level-10

8c6c6ff

Add A-Assertions

f2f66f2

Add A-CodeQuality

5aa35bd

Merge pull request #2 from cheunggalen/branch-A-Assertions

9368796

Add branch-A-Assertions to master

Merge branch 'master' of https://github.com/cheunggalen/ip

5dda2d4

* 'master' of https://github.com/cheunggalen/ip: Add A-Assertions

Merge branch 'master' into branch-A-CodeQuality

7219bb1

# Conflicts: # src/main/java/Parser.java

cheunggalen and others added 14 commits February 18, 2021 09:35

Merge pull request #3 from cheunggalen/branch-A-CodeQuality

2d98d61

Add branch-A-CodeQuality to master

Merge branch 'master' of https://github.com/cheunggalen/ip

82abb8a

* 'master' of https://github.com/cheunggalen/ip: Add A-CodeQuality

Add C-DetectDuplicates

8191bc2

Merge pull request #4 from cheunggalen/branch-C-DetectDuplicates

652d7ea

Add C-DetectDuplicates

Merge branch 'master' of https://github.com/cheunggalen/ip

e0b227a

* 'master' of https://github.com/cheunggalen/ip: Add C-DetectDuplicates

Set theme jekyll-theme-midnight

ba97fbe

Add A-UserGuide

a536d24

Add screenshot of Duke, Ui.png into the user guide, README.md.

d93287f

Merge branch 'master' of https://github.com/cheunggalen/ip

89640ab

* 'master' of https://github.com/cheunggalen/ip: Set theme jekyll-theme-midnight

Add the modified user guide, README.md.

c8a89a6

Set theme jekyll-theme-cayman

31c4a9a

Merge branch 'master' of https://github.com/cheunggalen/ip

18fbc91

* 'master' of https://github.com/cheunggalen/ip: Set theme jekyll-theme-cayman

Add modified user guide, README.md.

ecf269f

Change shadow version

3629de4

		@@ -0,0 +1,18 @@
		import org.junit.jupiter.api.Test;
		import static org.junit.jupiter.api.Assertions.assertEquals;

Conversation

cheunggalen commented Jan 28, 2021

Uh oh!

noelmathewisaac Feb 4, 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!

Choose a reason for hiding this comment

Uh oh!

noelmathewisaac Feb 4, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

noelmathewisaac left a comment

Choose a reason for hiding this comment

Uh oh!

gycc7253 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!

3 participants

noelmathewisaac Feb 4, 2021 •

edited

Loading

noelmathewisaac Feb 4, 2021 •

edited

Loading