[Lyu Xingyu] iP#206
Conversation
| if(line.equals("bye")){ | ||
| // quit | ||
| System.out.println(exitPrompt); | ||
| break; | ||
| }else{ |
There was a problem hiding this comment.
Perhaps you can leave a space after the conditional clause and opening brace
| try { | ||
| index = Integer.parseInt(line.substring(funcIdx+1, line.length())) - 1; | ||
| } catch (Exception e) { | ||
| index = -1; | ||
| } |
There was a problem hiding this comment.
I like how you have added error handling
There was a problem hiding this comment.
New implementation I've noticed. I have not seen this before. Maybe something that I should google up. Well done~
| if(funcIdx == -1){ | ||
| funcIdx = line.length(); | ||
| } | ||
| String func = line.substring(0, funcIdx); |
There was a problem hiding this comment.
perhaps you could give this variable a better name, as it is stored as a String but is named func
There was a problem hiding this comment.
I agree with @lihka1202 . I'd suggest to give a variable name like commandIndex or inputIndex as it it a bit more clear for readers to understand what you are referring to.
There was a problem hiding this comment.
Thank you for your good suggestions! I have made corrections.
| } | ||
| todoList.markItem(index, false); | ||
| }else if(func.equals("todo")){ | ||
| Todo todo = Todo.toTodo(instruction); |
There was a problem hiding this comment.
Perhaps you could name object todo better, as it has the same name as the class and it might get confusing!
| // mark work as done | ||
| int index; | ||
| try { | ||
| index = Integer.parseInt(line.substring(funcIdx+1, line.length())) - 1; |
There was a problem hiding this comment.
Could you abstract this out to maintain SLAP
There was a problem hiding this comment.
Agreed. Thank you for your suggestion!
| if(funcIdx == -1){ | ||
| funcIdx = line.length(); | ||
| } | ||
| String func = line.substring(0, funcIdx); |
There was a problem hiding this comment.
I agree with @lihka1202 . I'd suggest to give a variable name like commandIndex or inputIndex as it it a bit more clear for readers to understand what you are referring to.
| try { | ||
| index = Integer.parseInt(line.substring(funcIdx+1, line.length())) - 1; | ||
| } catch (Exception e) { | ||
| index = -1; | ||
| } |
There was a problem hiding this comment.
New implementation I've noticed. I have not seen this before. Maybe something that I should google up. Well done~
| int contentIdx = instruction.indexOf("/from"); | ||
| int fromIdx = instruction.indexOf("/to"); |
There was a problem hiding this comment.
Perhaps u could use Index instead of Idx
| if(todo == null){ | ||
| printError("Wrong todo format"); | ||
| continue; | ||
| } | ||
| todoList.addItem(todo); | ||
| }else if(func.equals("deadline")){ | ||
| Deadline deadline = Deadline.toDeadline(instruction); | ||
| if(deadline == null){ | ||
| printError("Wrong deadline format"); | ||
| continue; | ||
| } | ||
| todoList.addItem(deadline); | ||
| }else if(func.equals("event")){ | ||
| Event event = Event.toEvent(instruction); | ||
| if(event == null){ | ||
| printError("Wrong event format"); | ||
| continue; | ||
| } | ||
| todoList.addItem(Event.toEvent(instruction)); |
There was a problem hiding this comment.
'switch' statement would be better since there are many cases you want to separate.
0nandon
left a comment
0nandon
left a comment
There was a problem hiding this comment.
Function is better to be in verb form.
| return new Deadline(deadlineContent, deadlineBy); | ||
| } | ||
|
|
||
| public Deadline(String description, String by) { |
There was a problem hiding this comment.
| public Deadline(String description, String by) { | |
| public getDeadline(String description, String by) { |
0nandon
left a comment
0nandon
left a comment
There was a problem hiding this comment.
It is better to avoid deep nesting as possible as you can.
| if(todo == null){ | ||
| printError("Wrong todo format"); | ||
| continue; | ||
| } | ||
| todoList.addItem(todo); | ||
| }else if(func.equals("deadline")){ | ||
| Deadline deadline = Deadline.toDeadline(instruction); | ||
| if(deadline == null){ | ||
| printError("Wrong deadline format"); | ||
| continue; | ||
| } | ||
| todoList.addItem(deadline); | ||
| }else if(func.equals("event")){ | ||
| Event event = Event.toEvent(instruction); | ||
| if(event == null){ | ||
| printError("Wrong event format"); | ||
| continue; | ||
| } | ||
| todoList.addItem(Event.toEvent(instruction)); |
There was a problem hiding this comment.
There is deep nesting in if-else statement while every if statements inside are conducting similar calculation. There would be other way to better construct this code like declaring special function for those calculation.
| System.out.println(SPLITTER); | ||
| System.out.println(" OK, I've marked this task as not done yet:"); | ||
| System.out.print(" [ ] "); | ||
| } | ||
| System.out.println(tasks[num].getDescription()); | ||
| System.out.println(SPLITTER); | ||
| System.out.println(); |
There was a problem hiding this comment.
So many printings, I think in if-else statement. How about just making additional function for those printing stacks?
Level-8: Dates and Times
Level-9: Find
4 participants
No description provided.