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 Chenguang] iP #182

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

Conversation

classskipper351
Copy link

No description provided.

Comment on lines +45 to +47
System.out.print("---------------------------------------------\n");
System.out.print(command +"\n");
System.out.print("---------------------------------------------\n");
Copy link

Choose a reason for hiding this comment

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

I would suggest using System.lineSeparator() instead of "\n" for code compatibility with other systems.

Comment on lines 5 to 6
static String[] todolist = new String[MAX_LIST_LENGTH];
static boolean[] isDoneList = new boolean[MAX_LIST_LENGTH];
Copy link

Choose a reason for hiding this comment

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

Good use of a static final int, instead of specifying "100" directly in the array as you have avoided using magic numbers.

@@ -0,0 +1,62 @@
import javax.swing.*;
Copy link

Choose a reason for hiding this comment

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

Would it be better to not import all packages although according to google it is lightweight.

Comment on lines 14 to 17




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 reduce the number of line breaks.

if(s == null){
continue;
}
System.out.printf("%d."+GetIcon(i)+s+"\n",i);
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 add spaced before and after the "+", this is part of coding standards.

Copy link

@ChoonSiang ChoonSiang left a comment

Choose a reason for hiding this comment

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

Good attempt at abstraction, but need to work on formatting, especially on the usage of space to follow the coding standard and allow for easier readability. Also need to consider the logic of your program whereby the rest of the commands is only accessible after the user input list.

public class Todolist {

String Icon = "T";
private static final int MAX_LIST_LENGTH = 100;

Choose a reason for hiding this comment

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

Good use of constant for the max list length

static int ListCount= 0;
static Scanner in = new Scanner(System.in);

public static void AddToList(){

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 AddToList(){
public static void AddToList() {

Take note that for function declaration, there should be a space before {

while(!command.equals("quit")){
command = in.nextLine();
switch (command){
case"add":

Choose a reason for hiding this comment

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

Inconsistency in whether there is a space after case.

Suggested change
case"add":
case "add":


public static void PrintList(){
int i=1;
System.out.print("---------------------------------------------\n");

Choose a reason for hiding this comment

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

Consider making this a constant to prevent repetition of the same code and will be more consistent when modification is made

Comment on lines 24 to 37
switch (command){
case "Bye":
System.out.print(GOODBYE_GREETING);
break;
case "list":
Todolist.HandleList();
break;


default:
echo(command);
break;

}

Choose a reason for hiding this comment

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

Why is the other commands such as quit, help, mark, unmark etc. only accessible after the user input list?

System.out.print("supported commands: add , quit , help , mark,unmark ,clear, list\n");
break;
case "clear":
ClearList();

Choose a reason for hiding this comment

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

Good attempt at abstracting the functionality of each command

number= in.nextInt();
}

isDoneList[number-1]=true;

Choose a reason for hiding this comment

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

Suggested change
isDoneList[number-1]=true;
isDoneList[number-1] = true;

}
static void ClearList(){
int i;
for (i=0;i<MAX_LIST_LENGTH;i++){

Choose a reason for hiding this comment

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

Suggested change
for (i=0;i<MAX_LIST_LENGTH;i++){
for (i = 0; i < MAX_LIST_LENGTH; i++) {

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