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

[Yao Yuan] iP #310

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

[Yao Yuan] iP #310

wants to merge 60 commits into from

Conversation

ya0-yuan
Copy link

No description provided.

Copy link

@ZhangWanlin98 ZhangWanlin98 left a comment

Choose a reason for hiding this comment

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

Overall, your work is great, especially the structure.
You only need to take note of some requirements of the coding standard.

try {
ui.sayBye();
storage.writeToFile(list);
// to get out of loop and terminate the program

Choose a reason for hiding this comment

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

Is it better if an empty line is placed to separate the code above the comment and the comment?


public class ByeCommand extends Command {

ByeCommand(String str) {

Choose a reason for hiding this comment

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

Is it better if the access of the constructor is specified?

protected boolean isExit = false;

/**
* Creates a command.

Choose a reason for hiding this comment

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

Is it better if an empty line is placed separate the description and the param explanation?

this.str = str;
}

// different command class will have different execute implementation

Choose a reason for hiding this comment

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

Is it better to use a more formal java doc to explain the method?

private String time;

/**
* Creates a deadline task.

Choose a reason for hiding this comment

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

Is it better if we can have an empty line to separate the description and the @param?

super(description, isDone);
}

public String toString() {

Choose a reason for hiding this comment

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

Is it better if the @OverRide can be specified?

*/
public class EventCommand extends Command {

EventCommand(String str) {

Choose a reason for hiding this comment

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

Is it better if you specify the access?

import java.util.List;
import java.util.Scanner;

// deals with loading tasks from the file and saving tasks in the file

Choose a reason for hiding this comment

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

Is it better to have a formal java doc?

isDone = true;
}

public String toString() {

Choose a reason for hiding this comment

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

Is it better if the @OverRide can be specified?

super(description, isDone);
}

public String toString() {

Choose a reason for hiding this comment

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

Is it better if the @OverRide can be specified?

Copy link

@Sruthisarav Sruthisarav left a comment

Choose a reason for hiding this comment

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

Overall, good work! The code overall looks good, I like how you separated all the commands as well. Just remember to add spaces before '{' because other than that, I don't really see much coding violations.

@@ -1,5 +1,8 @@
import java.io.IOException;

/**
* Represents a command to be executed.
*/
public abstract class Command {
String str;

Choose a reason for hiding this comment

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

Could have better names for your variables. I have no idea what str does just from the name. Or maybe you could add a comment to explain what it does...?

/**
* Represents a delete command from a user
* which will lead to the deletion of a task in the task list.
*/
public class DeleteCommand extends Command{

Choose a reason for hiding this comment

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

needs a space before '{'

@@ -1,9 +1,19 @@
/**
* Represents a done command.
*/
public class DoneCommand extends Command{

Choose a reason for hiding this comment

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

Needs a space before '{'

@@ -1,3 +1,6 @@
/**
* Represents a event task.
*/
public class Event extends Task{

Choose a reason for hiding this comment

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

Needs a space before '{'

@@ -2,6 +2,9 @@
import java.time.format.DateTimeFormatter;
import java.time.format.DateTimeParseException;

/**
* Represents a event command from a user.
*/
public class EventCommand extends Command{

Choose a reason for hiding this comment

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

Needs another space here too


/**
* Represents a list command from a user.
*/
public class ListCommand extends Command{

Choose a reason for hiding this comment

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

Another space you forgot to add here.

// deals with interactions with the user
/**
* Deals with interactions with the user.
*/
public class Ui {
private Scanner sc;
private String line = "_________________________________________________________________";

Choose a reason for hiding this comment

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

This seems like a constant. Might wanna add 'final' to it as well.

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.

None yet

3 participants