[kevinz420] iP #62
[kevinz420] iP #62
Conversation
src/main/java/Duke.java
Outdated
|
|
||
| Scanner in = new Scanner(System.in); | ||
| String line; | ||
| while (in.hasNextLine()) { |
There was a problem hiding this comment.
Overall, I really like your pull request.
You abided by the coding standard and structured your code in a readable and modular way.
Maybe consider adding Javadoc comments to your more complex methods: I know that it is somewhat annoying at first, but it can really help in the long term and makes it easier for other people to understand what is going on.
src/main/java/Duke.java
Outdated
| if (line.equals("bye")) { | ||
| break; | ||
| } | ||
|
|
||
| if (line.equals("list")) { | ||
| printTasks(); | ||
| } else { | ||
| handleCommand(line); | ||
| } |
There was a problem hiding this comment.
What do you think about refactoring this into an "if - else if - else" statement instead of having a separate "if" statement and another "if - else" statement?
src/main/java/Duke.java
Outdated
| private static HashMap<String, String> parseParameters(String line) { | ||
| HashMap<String, String> fieldToValue = new HashMap<>(); | ||
|
|
||
| int startDescription = line.indexOf(" "); | ||
| int endOfDescription = line.indexOf(" /"); | ||
| fieldToValue.put("description", line.substring(startDescription + 1, endOfDescription)); | ||
|
|
||
| String[] splitParams = line.split(" /"); | ||
| for (int i = 1; i < splitParams.length; i++) { | ||
| String rawParam = splitParams[i]; | ||
| int divider = rawParam.indexOf(" "); | ||
|
|
||
| String field = rawParam.substring(0, divider); | ||
| String value = rawParam.substring(divider + 1); | ||
| fieldToValue.put(field, value); | ||
| } | ||
|
|
||
| return fieldToValue; | ||
| } |
There was a problem hiding this comment.
I love that you factored this out into a separate method!
However, what do you think about adding a small Javadoc comment? It took me a bit to figure out how exactly the HashMap you return is structured, and I think that just documenting this in a small comment might make it easier for future contributors
src/main/java/Duke.java
Outdated
|
|
||
| tasks[numTasks] = task; | ||
| numTasks++; | ||
| System.out.println("Got it. I've added this task:\n" + task.getFormattedTask() + "\nNow you have " + numTasks + " tasks in the list."); |
There was a problem hiding this comment.
That line seems a bit long (although in the GitHub Web Interface I unfortunately can't see its exact length); what do you think about breaking it into two lines?
src/main/java/Duke.java
Outdated
| private static HashMap<String, String> parseParameters(String line) { | ||
| HashMap<String, String> fieldToValue = new HashMap<>(); | ||
|
|
||
| int startDescription = line.indexOf(" "); |
There was a problem hiding this comment.
for consistency, can rename to startOfDescription
src/main/java/Duke.java
Outdated
| if (line.contains("mark")) { | ||
| int idx = Integer.parseInt(line.substring(divider + 1)) - 1; | ||
| if (idx < 0 || tasks[idx] == null) { | ||
| System.out.println("Sorry! That's not a valid task"); | ||
| return; | ||
| } | ||
|
|
||
| markTask(idx, line.startsWith("mark")); | ||
| return; | ||
| } | ||
| if (line.startsWith("todo")){ | ||
| addTask(new Todo(line.substring(divider + 1))); | ||
| return; | ||
| } | ||
|
|
||
| HashMap<String, String> parameters = parseParameters(line); | ||
| String description = parameters.get("description"); | ||
| if (description == null) { | ||
| System.out.println("Sorry! Please provide a valid description"); | ||
| return; | ||
| } | ||
| if (line.startsWith("deadline")) { | ||
| String by = parameters.get("by"); | ||
| if (by == null) { |
There was a problem hiding this comment.
you should consider using if-else statements instead of all if statements. Another alternative if using switch-case statements
Add find functionality
Add JavaDoc comments
No description provided.