[lcsroy] iP#68
Conversation
chydarren
left a comment
chydarren
left a comment
There was a problem hiding this comment.
Well-written and organized code! Just a few minor tweaks needed, keep up the good work! :)
| System.out.println("Hello! I'm Duke"); | ||
| System.out.println("What can i do for you?"); | ||
| String[] List = new String[100]; | ||
| int ListNo = 0; |
There was a problem hiding this comment.
Great that the purpose of the variable is described clearly, but do note that variable names must be in camelCase.
| line = in.nextLine(); | ||
| if (line.equals("bye")){ | ||
| break; | ||
| }else if(line.compareTo("list") != 0) { |
There was a problem hiding this comment.
For the if-else statement, try to maintain consistent spacing between the "}" brace and your "else if". Suppose your other cases you have a space between them, then try to maintain it throughout the code to neaten readability.
| } | ||
|
|
||
| public String getStatusIcon(){ | ||
| return (isDone ? "X" : " "); |
There was a problem hiding this comment.
Excellent use of an in-line conditional expression to make the code more intuitive!
| System.out.println("[" + t.getStatusIcon() + "] " + List[Integer.parseInt(line.substring(i))-1]); | ||
| } else { | ||
| System.out.println("added: " + line); | ||
| List[ListNo] = line; |
There was a problem hiding this comment.
Note that array variables need to be in camelCase too. Pascal cases should only be used for class and enum declarations.
| if (line.equals("bye")){ | ||
| break; | ||
| }else if(line.compareTo("list") != 0) { | ||
| if(line.contains("mark")){ |
There was a problem hiding this comment.
I would recommend using .startsWith() so that the use case can be more specific.
| System.out.println("Nice! I've marked this task as done:"); | ||
| System.out.println("[" + t.getStatusIcon() + "] " + List[Integer.parseInt(line.substring(i))-1]); | ||
| } else if(line.contains("unmark")) { | ||
| int i = line.indexOf(" ") + 1; |
There was a problem hiding this comment.
I'm not quite able to decipher why we are counting the index of the space, maybe might want to add a comment to describe this part.
okkhoy
left a comment
okkhoy
left a comment
There was a problem hiding this comment.
There are a few issues that I have noted.
- The PR title is incorrect. Please follow the given convention
- There are minimal updates since 19 days ago. I urge you to complete the iP tasks asap.
- If you are facing any issues in completing the iP tasks, please ping me for help.
| do{ | ||
| line = in.nextLine(); | ||
| if (line.equals("bye")){ | ||
| break; | ||
| }else if(line.compareTo("list") != 0) { |
There was a problem hiding this comment.
Spacing is an issue in this piece of code, not just for the if-else block as mentioned by chydarren
| break; | ||
| }else if(line.compareTo("list") != 0) { | ||
| if(line.contains("mark")){ | ||
| int i = line.indexOf(" ") + 1; |
There was a problem hiding this comment.
Try to get rid of the spaces and use some other demarcator to help identify the command/parameter boundaries
| public boolean markAsDone(){ | ||
| isDone = true; | ||
| return isDone; | ||
| } | ||
| public boolean unmarkAsNotDone(){ | ||
| isDone = false; | ||
| return isDone; | ||
| } | ||
| } |
There was a problem hiding this comment.
Two issues here:
- Spacing doesn't follow the coding standards
- please have some comments to all non-trivial classes and/or methods
3 participants
Level 3 still working in progress