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

[Sunaga] iP #240

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

[Sunaga] iP #240

wants to merge 74 commits into from

Conversation

nowknowing
Copy link

No description provided.

Copy link

@w2vgd w2vgd left a comment

Choose a reason for hiding this comment

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

Code is easy to read overall 👍 I have noted a few coding standard violations.



private void processInput(String userInput) {
String keyword_UC = userInput.toUpperCase().split(" ", -1)[0];
Copy link

Choose a reason for hiding this comment

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

Perhaps use a camelCase variable name and a more descriptive name rather than UC here?

import duke.exceptions.DukeMissingDesException;
import duke.handler.Parser;
import duke.handler.Queries;
import duke.tasks.*;
Copy link

Choose a reason for hiding this comment

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

Should this import statement be listed explicitly instead?

import duke.tasks.*;
import duke.tasks.Event;

import java.awt.*;
Copy link

Choose a reason for hiding this comment

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

Same issue as above

import java.util.Scanner;

public class Storage {
private static String STORAGE_PATH;
Copy link

Choose a reason for hiding this comment

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

Perhaps this instance variable should be declared final instead? (as your variable name is already in all uppercase letters)

}

public String readCommand() {
assert hasCommand();
Copy link

Choose a reason for hiding this comment

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

Like the use of assertion here 😄

@@ -0,0 +1,14 @@
package duke.exceptions;

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.

Should there be a space between Exception and the opening bracket {?

@@ -0,0 +1,23 @@
package duke.exceptions;

public class DukeIDKException extends DukeException{
Copy link

Choose a reason for hiding this comment

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

Should there be a space between DukeException and the opening bracket {?
I noticed the same issue in several other places too.

}

public Task markDone() {
this.isDone = true;
Copy link

Choose a reason for hiding this comment

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

Perhaps you can remove the unnecessary this here?

this.type = type;
}

public Task markDone() {
Copy link

Choose a reason for hiding this comment

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

Should this method be named differently because markDone does not really tell us that a task will be returned?



public TaskList() {
this.tasks = new ArrayList<>();
Copy link

Choose a reason for hiding this comment

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

Perhaps remove the redundant this here?

Copy link

@cnlinh cnlinh left a comment

Choose a reason for hiding this comment

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

Overall clear and well-formatted code but some parts can be further abstracted for clarity.

import java.util.Scanner;

public class Duke {
private boolean exit = false;
Copy link

Choose a reason for hiding this comment

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

isExit

}

public static void main(String[] args) throws IOException {
new Duke("data/tasks.txt").run();
Copy link

Choose a reason for hiding this comment

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

You may want to use relative path

Copy link
Author

Choose a reason for hiding this comment

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

Hi my understanding was that this already is a relative path.
What do you mean by use a "relative path"? could you suggest how this is done? Thank you.

break;

case ADD:
Task toAdd;
Copy link

Choose a reason for hiding this comment

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

You may want to abstract each case into a separate method


public class Parser {
public static Task parseFromData(String dataInput) {
String[] splitInput = dataInput.split("\\|", -1);
Copy link

Choose a reason for hiding this comment

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

splitInputs?

/**
* Constructor for DukeException for incomprehensible request.
*/
public DukeIDKException() {
Copy link

Choose a reason for hiding this comment

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

DukeIdkException?

nowknowing and others added 30 commits February 12, 2021 17:48
Add TaskHandler absrtact class.
# Conflicts:
#	src/main/java/duke/handler/DeadlineHandler.java
#	src/main/java/duke/handler/EventHandler.java
#	src/main/java/duke/handler/TodoHandler.java
Format requires Schedule weekly (no.of times) (schedulable event).
Issue: listing does not work beyond 7 tasks when queried as after 1st query.
Add Ui.png screenshot
Change app  name to remove space
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.

3 participants