Open
Conversation
brand0nnn
reviewed
Feb 6, 2025
src/main/java/WallE.java
Outdated
Comment on lines
+51
to
+72
| switch (params[0]) { | ||
| case "list": | ||
| printTaskList(tasks, size); | ||
| break; | ||
| case "mark": | ||
| taskIndex = Integer.parseInt(params[1]); | ||
| tasks[taskIndex - 1].markAsDone(); | ||
| printWithLineBreak("Nice! I've marked this task as done:\n" | ||
| + "\t [" + tasks[taskIndex - 1].getStatusIcon() + "] " | ||
| + tasks[taskIndex - 1].description); | ||
| break; | ||
| case "unmark": | ||
| taskIndex = Integer.parseInt(params[1]); | ||
| tasks[taskIndex - 1].unmarkAsDone(); | ||
| printWithLineBreak("OK, I've marked this task as not done yet:\n" | ||
| + "\t " + "[ ] " + tasks[taskIndex - 1].description); | ||
| break; | ||
| default: | ||
| tasks[size] = new Task(userInput); | ||
| size++; | ||
| printWithLineBreak("added: " + userInput); | ||
| } |
There was a problem hiding this comment.
Neat use of switch to handle the commands!
src/main/java/WallE.java
Outdated
Comment on lines
+4
to
+37
| public static void printLineBreak() { | ||
| String lineBreak = "\t_________________________________\n"; | ||
| System.out.print(lineBreak); | ||
| } | ||
|
|
||
| public static void printWithLineBreak(String response) { | ||
| printLineBreak(); | ||
| System.out.println("\t" + response); | ||
| printLineBreak(); | ||
| } | ||
|
|
||
| public static void printWithoutLineBreak(String response) { | ||
| System.out.println("\t" + response); | ||
| } | ||
|
|
||
| public static void printGreeting() { | ||
| String greeting = "Hello! I'm Wall-E!\n" + "\tWhat can I do for you?\n\n"; | ||
| printWithLineBreak(greeting); | ||
| } | ||
|
|
||
| public static void printExit() { | ||
| String exitMessage = "Bye. Hope to see you again soon!"; | ||
| printWithLineBreak(exitMessage); | ||
| } | ||
|
|
||
| public static void printTaskList(Task[] tasks, int size) { | ||
| printLineBreak(); | ||
| printWithoutLineBreak("Here are the tasks in your list: "); | ||
| for (int i = 0; i < size; i++) { | ||
| printWithoutLineBreak(Integer.toString(i + 1) + ". [" + tasks[i].getStatusIcon() + "] " | ||
| + tasks[i].description); | ||
| } | ||
| printLineBreak(); | ||
| } |
There was a problem hiding this comment.
Very neat code for the different functions!
src/main/java/WallE.java
Outdated
Comment on lines
+65
to
+66
| printWithLineBreak("OK, I've marked this task as not done yet:\n" | ||
| + "\t " + "[ ] " + tasks[taskIndex - 1].description); |
There was a problem hiding this comment.
Nice use of string concatenation for readability!
sevenseasofbri
left a comment
sevenseasofbri
left a comment
There was a problem hiding this comment.
Good job so far! Pay attention to the usage of magic literals in your code.
src/main/java/walle/FileParser.java
Outdated
Comment on lines
+12
to
+13
| File f = new File(filePath); | ||
| Scanner s = new Scanner(f); |
There was a problem hiding this comment.
Consider using more intuitive naming, f and s do not explain what the variables entail.
src/main/java/walle/FileParser.java
Outdated
Comment on lines
+15
to
+34
| while (s.hasNext()) { | ||
| String[] taskString = s.nextLine().split(","); | ||
| boolean isDone = taskString[0].equals("1") ? true : false; | ||
| String taskType = taskString[1]; | ||
| String taskDescription = taskString[2]; | ||
|
|
||
| if (taskType.equals("T")) { | ||
| tasks.add(new Todo(taskDescription)); | ||
| } else if (taskType.equals("D")) { | ||
| String dueDate = taskString[3]; | ||
| tasks.add(new Deadline(taskDescription, dueDate)); | ||
| } else if (taskType.equals("E")) { | ||
| String fromDate = taskString[3]; | ||
| String toDate = taskString[4]; | ||
| tasks.add(new Event(taskDescription, fromDate, toDate)); | ||
| } | ||
|
|
||
| if (isDone) { | ||
| tasks.get(listSize).markAsDone(); | ||
| } |
There was a problem hiding this comment.
There are many magic literals here, consider using constants instead or if not possible, then add some concise comments on what is happening here.
Comment on lines
+70
to
+86
| try { | ||
| FileWriter writer = new FileWriter(f); | ||
| for (int i = 0; i < listSize; i++) { | ||
| Task t = tasks.get(i); | ||
| String status = t.isDone() ? "1" : "0"; | ||
| writer.write(status + "," + t.getTypeIcon() + "," + t.getDescription()); | ||
| if (t.getTypeIcon() == "T") { | ||
| writer.write("\n"); | ||
| } else if (t.getTypeIcon() == "D") { | ||
| Deadline d = (Deadline) t; | ||
| writer.write("," + d.getDueDate() + "\n"); | ||
| } else if (t.getTypeIcon() == "E") { | ||
| Event e = (Event) t; | ||
| writer.write("," + e.getStartDate() + "," + e.getEndDate() + "\n"); | ||
| } | ||
| } | ||
| writer.close(); |
There was a problem hiding this comment.
This is around 3 levels of indentation, consider refactoring the internal levels into another method.
src/main/java/walle/WallE.java
Outdated
| import java.util.List; | ||
|
|
||
| public class WallE { | ||
| private static final int MAX_LIST_SIZE = 100; |
src/main/java/walle/WallE.java
Outdated
Comment on lines
+12
to
+15
| private static final String exitMessage = "Bye. Hope to see you again soon!"; | ||
| private static final String greeting = "Hello! I'm Wall-E!\n" + "\tWhat can I do for you?\n\n"; | ||
| private static int listSize = 0; | ||
| private static final String FILE_NAME = "data.txt"; |
There was a problem hiding this comment.
exitMessage and greeting could be considered constants since they technically shouldn't change in the course of the program
Constant names must be all uppercase using underscore to separate words (aka SCREAMING_SNAKE_CASE)
src/main/java/walle/WallE.java
Outdated
| Scanner reader = new Scanner(System.in); | ||
| String userInput = reader.nextLine(); | ||
|
|
||
| // Echoes the input, unless input == bye |
There was a problem hiding this comment.
Is this comment relevant to the program anymore? I suppose the chatbot does not simply echo the entered command anymore.
src/main/java/walle/WallE.java
Outdated
| userInput = reader.nextLine(); | ||
| } | ||
|
|
||
| //putIntoStoredTasks(tasks); |
There was a problem hiding this comment.
Avoid commenting out dead code, remove it instead.
src/main/java/walle/WallE.java
Outdated
Comment on lines
+136
to
+159
| for (int i = 1; i < params.length; i++) { | ||
| if (params[i].equals("/from")) { | ||
| stringType = 1; | ||
| } else if (params[i].equals("/to")) { | ||
| stringType = 2; | ||
| } else { | ||
| if (stringType == 0) { | ||
| if (eventName.isEmpty()) { | ||
| eventName.append(params[i]); | ||
| } else { | ||
| eventName.append(" ").append(params[i]); | ||
| } | ||
| } else if (stringType == 1) { | ||
| if (startDate.isEmpty()) { | ||
| startDate.append(params[i]); | ||
| } else { | ||
| startDate.append(" ").append(params[i]); | ||
| } | ||
| } else { | ||
| if (endDate.isEmpty()) { | ||
| endDate.append(params[i]); | ||
| } else { | ||
| endDate.append(" ").append(params[i]); | ||
| } |
There was a problem hiding this comment.
This is very deep nesting and can be considered arrowhead code. Try to refactor the internal layers into separate methods.
src/main/java/walle/WallE.java
Outdated
Comment on lines
+174
to
+189
| for (int i = 1; i < params.length; i++) { | ||
| if (params[i].equals("/by")) { | ||
| stringType = 2; | ||
| } else { | ||
| if (stringType == 0) { | ||
| if (!deadlineName.isEmpty()) { | ||
| deadlineName.append(" ").append(params[i]); | ||
| } else { | ||
| deadlineName.append(params[i]); | ||
| } | ||
| } else if (stringType == 2) { | ||
| if (!byDate.isEmpty()) { | ||
| byDate.append(" ").append(params[i]); | ||
| } else { | ||
| byDate.append(params[i]); | ||
| } |
There was a problem hiding this comment.
Same with this segment, this is very deep nesting .
src/main/java/walle/WallE.java
Outdated
Comment on lines
+209
to
+217
| printWithLineBreak("OK, I've marked this task as not done yet:\n" | ||
| + "\t" + tasks.get(taskIndex - 1).toString()); | ||
| } | ||
|
|
||
| private static void markTask(String[] params) { | ||
| int taskIndex; | ||
| taskIndex = Integer.parseInt(params[1]); | ||
| tasks.get(taskIndex - 1).markAsDone(); | ||
| printWithLineBreak("Nice! I've marked this task as done:\n" |
There was a problem hiding this comment.
Wrapped lines should be indented 8 spaces from their parent line. E.g.:
// from the java coding standards website
if (isReady) {
setText("Long line split"
+ "into two parts.");
}Add date time functionality to Deadline class
Add find functionality to WallE
Add JavaDoc comments for public methods and classes
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.