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.

Sign up for GitHub

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

[Tay Jun Yang] iP #500

Open

dannytayjy wants to merge 18 commits into nus-cs2103-AY2122S1:master from dannytayjy:master

dannytayjy commented Aug 30, 2021

No description provided.

dannytayjy added 14 commits

August 19, 2021 19:39


Implement Level-1: Greet, Echo, Exit

7d4a3b6


Implement Level-2: Add, List

103b9a9


Implement Level-3: Mark task as Done

a48639a


Implement Level-4: ToDo, Event, Deadline and TaskType Enum

5ae82e5


Update Duke.java: Edit print output

5c3a321


Implement A-TextUiTesting: Update EXPECTED.TXT and input.txt

3f58e09


Implement Level-5: Handle errors via Exception Handling

b71fe73


Implement Level-6: Delete task

7c277bc


Implement Level-7: Save tasks to text file

73492fc


Implement Level-8: Teach Duke to understand dates and times

0a46d99


Merge branch 'branch-Level-8'

474f703


Implement A-MoreOOP: Make the code more OOP

0f100be


Update A-MoreOOP: Make the code more OOP

d89b0c1


Implement A-Packages and fix bugs

942e26c

leeroy999 reviewed

View reviewed changes

leeroy999 left a comment

Overall lgtm! Just note the few nits, and perhaps it could be better to reduce copy-pasting of certain code blocks, and abstract them or use as default from abstract class that it is inheriting from.

src/main/java/duke/Duke.java

+ new Duke("data", "tasks.txt").run();
+ }
+ public void run() {

leeroy999 Sep 1, 2021

Would be good to add headers comments for public methods.

src/main/java/duke/Duke.java


		import java.io.IOException;

		public class Duke {

leeroy999 Sep 1, 2021

Perhaps add header comment for the class.

Noticed that there is no JavaDocs added throughout. Perhaps it would be good practice to have them, as documentation to improve readability of your code.

src/main/java/duke/Parser.java

+ if ((commandType.equals("bye") || commandType.equals("list")) && !commandInfo.equals("")) {
+ throw new DukeException("INVALID COMMAND. Please try again!");
+ }
+ } else { // for "bye" and "list" commands

leeroy999 Sep 1, 2021

Comments should be indented relative to their position in the code.

src/main/java/duke/Parser.java

+ return command;
+ }
+ private int getTaskNumFromCommand(String commandInfo) throws DukeException {

leeroy999 Sep 1, 2021

Perhaps a shorter name would be better e.g. getTaskNum.

src/main/java/duke/Parser.java

+ }
+ }
+ private String getTaskDescriptionFromToDoCommand(String commandInfo) throws DukeException {

leeroy999 Sep 1, 2021

Same comment as above, method names could be shorter and more concise, and if they are specific to a command, perhaps it could be a better design to have them in the respective command classes.

This applies to other method names as well.

src/main/java/duke/Parser.java

Comment on lines +101 to +161

+ if (deadlineTaskDetails.length == 2) {
+ if (deadlineTaskDetails[0].trim().length() > 0) {
+ if (deadlineTaskDetails[1].trim().startsWith("by")) {
+ String beforeDateTime = deadlineTaskDetails[1].trim();
+ String[] beforeDateTimeParts = beforeDateTime.split("\\s+", 2);
+ if (beforeDateTimeParts.length == 2) {
+ return commandInfo;
+ } else {
+ throw new DukeException("INCOMPLETE COMMAND"
+ + System.lineSeparator() + "\t"
+ + "The date/time of a deadline cannot be empty!"
+ + System.lineSeparator() + "\t"
+ + "[Note: Enter /by before specifying the date/time]");
+ }
+ } else {
+ throw new DukeException("WRONG COMMAND"
+ + System.lineSeparator() + "\t"
+ + "Enter /by before specifying the date!");
+ }
+ } else {
+ if (deadlineTaskDetails[1].trim().startsWith("by")) {
+ String beforeDateTime = deadlineTaskDetails[1].trim();
+ String[] beforeDateTimeParts = beforeDateTime.split("\\s+", 2);
+ if (beforeDateTimeParts.length == 2) {
+ throw new DukeException("INCOMPLETE COMMAND"
+ + System.lineSeparator() + "\t"
+ + "The description of a deadline cannot be empty!");
+ } else {
+ throw new DukeException("INCOMPLETE COMMAND"
+ + System.lineSeparator() + "\t"
+ + "The description of a deadline cannot be empty!"
+ + System.lineSeparator() + "\t"
+ + "The date/time of a deadline cannot be empty!"
+ + System.lineSeparator() + "\t"
+ + "[Note: Enter /by before specifying the date/time]");
+ }
+ } else {
+ throw new DukeException("INCOMPLETE & WRONG COMMAND"
+ + System.lineSeparator() + "\t"
+ + "The description of a deadline cannot be empty!"
+ + System.lineSeparator() + "\t"
+ + "Enter /by before specifying the date/time!");
+ }
+ }
+ } else {
+ throw new DukeException("INCOMPLETE COMMAND"
+ + System.lineSeparator() + "\t"
+ + "The date/time of a deadline cannot be empty!"
+ + System.lineSeparator() + "\t"
+ + "[Note: Enter /by before specifying the date/time]");
+ }
+ } else {
+ throw new DukeException("INCOMPLETE COMMAND"
+ + System.lineSeparator() + "\t"
+ + "The description and date/time of a deadline cannot be empty!"
+ + System.lineSeparator() + "\t"
+ + "[Note: Enter /by before specifying the date/time]");
+ }
+ }

leeroy999 Sep 1, 2021

Avoid deep nesting, especially if you need more than 3 levels of indentation. In particular, avoid arrowhead style code.

src/main/java/duke/Parser.java

Comment on lines +164 to +226

+ if (commandInfo.trim().length() > 0) {
+ String[] eventTaskDetails = commandInfo.split("/", 2);
+ if (eventTaskDetails.length == 2) {
+ if (eventTaskDetails[0].trim().length() > 0) {
+ if (eventTaskDetails[1].trim().startsWith("at")) {
+ String startEndDateTime = eventTaskDetails[1].trim();
+ String[] startEndDateTimeParts = startEndDateTime.split("\\s+", 2);
+ if (startEndDateTimeParts.length == 2) {
+ return commandInfo;
+ } else {
+ throw new DukeException("INCOMPLETE COMMAND"
+ + System.lineSeparator() + "\t"
+ + "The date/time of an event cannot be empty!"
+ + System.lineSeparator() + "\t"
+ + "[Note: Enter /at before specifying the date/time]");
+ }
+ } else {
+ throw new DukeException("WRONG COMMAND"
+ + System.lineSeparator() + "\t"
+ + "Enter /at before specifying the date!");
+ }
+ } else {
+ if (eventTaskDetails[1].trim().startsWith("at")) {
+ String startEndDateTime = eventTaskDetails[1].trim();
+ String[] startEndDateTimeParts = startEndDateTime.split("\\s+", 2);
+ if (startEndDateTimeParts.length == 2) {
+ throw new DukeException("INCOMPLETE COMMAND"
+ + System.lineSeparator() + "\t"
+ + "The description of an event cannot be empty!");
+ } else {
+ throw new DukeException("INCOMPLETE COMMAND"
+ + System.lineSeparator() + "\t"
+ + "The description of an event cannot be empty!"
+ + System.lineSeparator() + "\t"
+ + "The date/time of an event cannot be empty!"
+ + System.lineSeparator() + "\t"
+ + "[Note: Enter /at before specifying the date/time]");
+ }
+ } else {
+ throw new DukeException("INCOMPLETE & WRONG COMMAND"
+ + System.lineSeparator() + "\t"
+ + "The description of an event cannot be empty!"
+ + System.lineSeparator() + "\t"
+ + "Enter /at before specifying the date/time!");
+ }
+ }
+ } else {
+ throw new DukeException("INCOMPLETE COMMAND"
+ + System.lineSeparator() + "\t"
+ + "The date/time of an event cannot be empty!"
+ + System.lineSeparator() + "\t"
+ + "[Note: Enter /at before specifying the date/time]");
+ }
+ } else {
+ throw new DukeException("INCOMPLETE COMMAND"
+ + System.lineSeparator() + "\t"
+ + "The description and date/time of an event cannot be empty!"
+ + System.lineSeparator() + "\t"
+ + "[Note: Enter /at before specifying the date/time]");
+ }

leeroy999 Sep 1, 2021

Same as comment above, avoid deep nesting.

src/main/java/duke/task/TaskWithDateTime.java

Comment on lines +52 to +90

+ if (!timeInput.equals("")) {
+ try {
+ int timeInputInInt = Integer.parseInt(timeInput);
+ if ((timeInputInInt < 0 || timeInputInInt > 2359)) {
+ isTimeInputProper = false;
+ time = null;
+ } else {
+ if (timeInput.length() != 4) {
+ isTimeInputProper = false;
+ time = null;
+ } else {
+ int hour = timeInputInInt / 100;
+ int min = timeInputInInt - (hour * 100);
+ String timeInputInTimeFormat = "";
+ if (hour < 10) {
+ timeInputInTimeFormat += "0" + hour;
+ } else {
+ timeInputInTimeFormat += hour;
+ }
+ if (min < 10) {
+ timeInputInTimeFormat += ":" + "0" + min;
+ } else {
+ timeInputInTimeFormat += ":" + min;
+ }
+ DateTimeFormatter timeFormatter = DateTimeFormatter.ISO_LOCAL_TIME;
+ time = LocalTime.parse(timeInputInTimeFormat, timeFormatter);
+ }
+ }
+ } catch (Exception e) {
+ time = null;
+ }
+ } else {
+ time = null;
+ }

leeroy999 Sep 1, 2021

Same as above, try to avoid deep nesting.

src/main/java/duke/task/TaskType.java

+ EVENT("event", "E");
+ private final String label;
+ private final String abbr;

leeroy999 Sep 1, 2021

Perhaps better name could be letter or spelled in full abbreviation

dannytayjy added 4 commits

September 9, 2021 19:41


Add JavaDoc comments

765d73c


Update Ui.java to amend String in displayWelcomeMessage

483a811


Add JUnit tests

4fa286f


Merge branch 'branch-A-JavaDoc'

377e1db

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