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

[LWachtel1] iP #201

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

Conversation

LWachtel1
Copy link

No description provided.

Copy link

@yicheng-toh yicheng-toh left a comment

Choose a reason for hiding this comment

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

Great progress on iP. Do take note to follow the coding standards from java coding standard.

Comment on lines 10 to 17
/*
public String getBy() {
return this.by;
}

public String setBy() {
return this.by;
}*/

Choose a reason for hiding this comment

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

Suggested change
/*
public String getBy() {
return this.by;
}
public String setBy() {
return this.by;
}*/

Perhaps you might want to remove commented code that are not in use.

import java.util.Scanner;
import java.util.ArrayList;

// I am using the Google Java Style Guide https://google.github.io/styleguide/javaguide.html

Choose a reason for hiding this comment

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

For this module, we are following this java coding standard.

System.out.printf("%s%n"," ____________________________________________________________");
}

public static void replyAddedTask(){

Choose a reason for hiding this comment

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

Suggested change
public static void replyAddedTask(){
public static void replyAddedTask() {

Perhaps considering adding a space here to be consistent with the rest of the methods.

String taskType=userCommand.split(" ")[0].toUpperCase();

switch (taskType) {
default:

Choose a reason for hiding this comment

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

Perhaps you might want to put "default" statement at the end of the switch statements.

break;

case "DEADLINE":
String[] deadLine=userCommand.split("/");

Choose a reason for hiding this comment

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

Suggested change
String[] deadLine=userCommand.split("/");
String[] deadLine = userCommand.split("/");

You might want to consider adding spaces before and after the "=" sign for better readability.


switch (markChoice) {

case "MARK":

Choose a reason for hiding this comment

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

Suggested change
case "MARK":
case "MARK":

Perhaps you might want to put the case statements with same amount of indentation with the switch statement in accordance to java coding standard.

boolean shouldExitLoop=false;

while (!shouldExitLoop) {
if (enteredCommands.size()>=100) {

Choose a reason for hiding this comment

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

You might want to consider using a constant to replace the magic number of 100.

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.

2 participants