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

[AY1920S1-CS2113-T09-1] Hustler #91

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

Conversation

yzia2000
Copy link

Copy link

@seanieyap seanieyap 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 job. Just some coding quality issues like magic numbers and commenting issues.

Comment on lines 49 to 50
taskType = "deadline";
break;

Choose a reason for hiding this comment

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

Indentation problem here.

Comment on lines 52 to 53
taskType = "event";
break;

Choose a reason for hiding this comment

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

Indentation problem here.

Comment on lines 55 to 56
ui.show_message("/" + timeCommand + " is an invalid addition to /add");
return;

Choose a reason for hiding this comment

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

Indentation problem here.


Hustler.saveStorage();
} catch (CommandLineException | IOException e) {

Choose a reason for hiding this comment

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

??

Comment on lines 10 to 52
public class CommandLog {

private static ArrayList<String> commandlog;
private static boolean isRestoring = false;

public CommandLog() {
commandlog = new ArrayList<String>();
}

public static void recordCommand(String command) {
commandlog.add(command);
}

public static void restoreData(int numberOfCommandsToUndo) {
isRestoring = true;
int restoreDataUntil = commandlog.size() - numberOfCommandsToUndo;

if (restoreDataUntil >= 0) {
for (int i = 0; i < restoreDataUntil; i++) {
try {
CommandParser parser = new CommandParser();

Command command = parser.parse(commandlog.get(i));
command.execute();

Hustler.saveStorage();
} catch (CommandLineException | IOException e) {

}
}

while (commandlog.size() > restoreDataUntil) {
commandlog.remove(restoreDataUntil);
}
} else {
System.out.println("Error! You are attempting to undo more commands than is possible!");
}
isRestoring = false;
}

public static boolean isRestoring() {
return isRestoring;
}

Choose a reason for hiding this comment

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

Missing header comments. All non trivial methods should have javadoc format header comments. You have done so for the rest of the classes but not here.

Comment on lines 106 to 107
suffix = "st";
break;

Choose a reason for hiding this comment

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

Indentation problem here.

Comment on lines 15 to 34
/**
* Reminder list that contains tasks that are overdue.
*/
private static ArrayList<Task> overDueList = new ArrayList<>();
/**
* Reminder list that contain tasks with less than 30 minutes or less till deadline.
*/
private static ArrayList<Task> lastDayList = new ArrayList<>();
/**
* Reminder list htat contain tasks with less than 24 hours or less till deadline.
*/
private static ArrayList<Task> lastThirtyMinutesList = new ArrayList<>();
/**
* 24 hours converted to seconds.
*/
private static final int TWENTY_FOUR_HOURS = 86400;
/**
* 30 minutes converted to seconds.
*/
private static final int THIRTY_MINUTES = 1800;

Choose a reason for hiding this comment

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

New lines should seperate these since they have comments

Comment on lines 94 to 98
if (!done && checkDeadline || checkEvent || checkRange) {
if (checkOverdue(i)) {
overDueList.add(list.get(i));
}
}

Choose a reason for hiding this comment

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

These two if statements can be combined into one.

public static void displayReminders() {
if (!overDueList.isEmpty()) {
if (overDueList.size() == 1) {
System.out.println("\t_____________________________________");

Choose a reason for hiding this comment

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

These lines can be saved into some variable since they are used so many times. Also to avoid "magic strings"; if you need to change these in the future, it would be a hassle to do so to all of them.

Comment on lines 118 to 119
this.tag.toString() + " " +
this.getDescription();

Choose a reason for hiding this comment

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

Should be on the same line

@seanieyap
Copy link

Please code this PR after reading my comments.

EnriqueKhai and others added 30 commits November 11, 2019 16:51
Rename statusTypes.java to StatusTypes.java
Fixed small checkstyle error
Fixed checkstyle errors for JUNIT testing
[AY1920S1-CS2113T-T09-1]-jingkang97-Fixed checkstyle for reminders
Fixed Recommended Schedule bug for test cases
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants