-
Notifications
You must be signed in to change notification settings - Fork 78
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
[azfarulmatin] IP #70
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall great job but some improvements to work on:)
src/main/java/Duke.java
Outdated
// Check for any inputs with '/' | ||
int checkSlash = userInput.indexOf("/"); | ||
if (checkSlash != -1) { | ||
description = userInput.substring(type.length() + 1, checkSlash).trim(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be refactored into a function? As we repeat it twice
src/main/java/Duke.java
Outdated
userInput = scanner.nextLine(); | ||
System.out.println(LINE); | ||
|
||
if (userInput.equals("bye")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could this be refactored into a switch statement for better readability?
src/main/java/Duke.java
Outdated
String by = bracketInfo.replace("/by", "").trim(); | ||
tasks[count] = new Deadline(description, by); | ||
} else if (type.equals("Event")) { | ||
String from = bracketInfo.split("/from")[1].split("/to")[0].trim(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code is relatively concise, but it might benefit from some additional comments or variable names to make it more self-explanatory. For example, you could define variables with more descriptive names for the split values.
src/main/java/Duke.java
Outdated
@Override | ||
public String toString() { | ||
return "[D]" | ||
+ super.toString() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code uses string concatenation to build the final string for the object's representation. While this approach works, it can be a bit less readable, especially when dealing with multiple string elements. Consider using String.format() for improved readability and performance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, GOOD JOB! There are some minor changes you can make for readability and organisation (using a switch statement sounds good!). You did well commenting each function and following the naming conventions!
src/main/java/Duke.java
Outdated
this.description = description; | ||
this.isDone = false; | ||
} | ||
public void markAsDone() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can consider spacing out (added new lines between) your functions for better readability
src/main/java/Duke.java
Outdated
} | ||
} | ||
|
||
class Deadline extends Task { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
having a new file for each class can keep your code more organised (abstraction)
src/main/java/Duke.java
Outdated
} | ||
public void markAsUndone() { | ||
isDone = false; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can consider combining this to become a setter method like "void setFound(boolean isFound);"
src/main/java/Duke.java
Outdated
|
||
class Task { | ||
protected String description; | ||
protected boolean isDone; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You did well in following the naming conventions (PascalCase for class and camelCase for variables)!!
src/main/java/Duke.java
Outdated
} | ||
} | ||
|
||
// Mark a task as done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job in explicitly commenting on the purpose of functions you added!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, your coding quality is very good, just small minor suggestions, not errors! Keep it up! 💯
src/main/java/Duke.java
Outdated
return "[X] " | ||
+ description; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason for separating it on different lines? It's a rather short line :)
src/main/java/Duke.java
Outdated
// Create a task of a specific type to the tasks array | ||
private static void addTask(String userInput, Task[] tasks, int count, String type) { | ||
String description; | ||
String bracketInfo; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps there might be a better more suggestive naming of the variable?
src/main/java/Duke.java
Outdated
String description; | ||
String bracketInfo; | ||
|
||
// Check for any inputs with '/' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like how you added comments for easier readability! 👍
src/main/java/Duke.java
Outdated
+ LINE); | ||
|
||
Scanner scanner = new Scanner(System.in); | ||
Task[] tasks = new Task[100]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job on always having proper naming of functions, classes and variables! 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall good job on your ip so far. Coding standard and code quality are alright. Try to structure your classes such that it only does one specific role. For example, your exceptions should be separated from your duke class. Keep up the good work!
src/main/java/duke/Duke.java
Outdated
public void run() { | ||
String LINE = "__________________________________________\n"; | ||
System.out.println(LINE | ||
+ "Hello I'm MatinBot\n" | ||
+ "What can I do for you?\n" | ||
+ LINE); | ||
|
||
Scanner scanner = new Scanner(System.in); | ||
Task[] tasks = new duke.Task[100]; | ||
int count = 0; | ||
String userInput; | ||
|
||
while (true) { | ||
// Get the user input first | ||
userInput = scanner.nextLine(); | ||
System.out.println(LINE); | ||
|
||
try { | ||
if (userInput.equals("bye")) { | ||
break; | ||
} else if (userInput.equals("list")) { | ||
printTasks(tasks, count); | ||
} else if (userInput.startsWith("mark")) { | ||
markTask(userInput, tasks, count); | ||
} else if (userInput.startsWith("unmark")) { | ||
unmarkTask(userInput, tasks, count); | ||
} else if (userInput.startsWith("todo")) { | ||
String description = userInput.replaceFirst("todo", "").trim(); | ||
if (description.isEmpty()) { | ||
throw new EmptyDescriptionException("todo"); | ||
} | ||
addTask("todo " + description, tasks, count, "todo"); | ||
count++; | ||
} else if (userInput.startsWith("deadline")) { | ||
String description = userInput.replaceFirst("deadline", "").trim(); | ||
if (description.isEmpty()) { | ||
throw new EmptyDescriptionException("deadline"); | ||
} | ||
// Check for /by in the description | ||
if (!description.contains("/by")) { | ||
System.out.println("☹ OOPS!!! The deadline task must include '/by' to specify the date."); | ||
} else { | ||
addTask("deadline " + description, tasks, count, "deadline"); | ||
count++; | ||
} | ||
} else if (userInput.startsWith("event")) { | ||
String description = userInput.replaceFirst("event", "").trim(); | ||
if (description.isEmpty()) { | ||
throw new EmptyDescriptionException("event"); | ||
} | ||
// Check for /from and /to in the description | ||
if (!description.contains("/from") || !description.contains("to")) { | ||
System.out.println("☹ OOPS!!! The event task must include '/from' and '/to' to specify the date range."); | ||
} else { | ||
addTask("event " + description, tasks, count, "event"); | ||
count++; | ||
} | ||
} else { | ||
throw new UnknownCommandException(); | ||
} | ||
} catch (EmptyDescriptionException e) { | ||
System.out.println(e.getMessage()); | ||
} catch (UnknownCommandException e) { | ||
System.out.println(e.getMessage()); | ||
} | ||
System.out.println(LINE); | ||
} | ||
|
||
System.out.println("Bye. Hope to see you again soon!\n" + LINE); | ||
scanner.close(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider abstracting away some of the heavy logic to reduce long methods. Generally more than 30 lines is too long.
src/main/java/duke/Duke.java
Outdated
|
||
Scanner scanner = new Scanner(System.in); | ||
Task[] tasks = new duke.Task[100]; | ||
int count = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using a more suitable variable name to keep track of your number of tasks.
src/main/java/duke/Duke.java
Outdated
switch (type) { | ||
case "todo": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Violation of coding standard here, there should not be an indentation for the case clause.
… to a string format to save
# Conflicts: # src/main/java/duke/Duke.java
No description provided.