-
Notifications
You must be signed in to change notification settings - Fork 116
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
[Qin Kai] iP #98
base: master
Are you sure you want to change the base?
[Qin Kai] iP #98
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 good. Can improve on coding standards like have better variable names, split into different methods, have different files etc.
src/main/java/bro.java
Outdated
public class bro { | ||
|
||
private static ArrayList<String> storer = new ArrayList<>(); |
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.
Can have a better variable name. Maybe specify what 'storer' stores.
src/main/java/bro.java
Outdated
public static void level0() { | ||
System.out.println("Hello! I'm bro"); | ||
System.out.println("What can I do for you?"); | ||
System.out.println("Bye. Hope to see you again soon!"); |
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.
This line should not be here? The chatbot prints Bye almost instantly
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.
Bye should not be in level 0
src/main/java/bro.java
Outdated
System.out.println("Nice! I've marked this task as done:"); | ||
System.out.println("[X] " + storer.get(task_num)); | ||
|
||
} else if (split_line[0].equals("unmark")) { |
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.
Should have made 'unmark' as another method. That way it is easier to debug/make changes.
src/main/java/bro.java
Outdated
System.out.println(line); | ||
} | ||
|
||
System.out.println("Bye. Hope to see you again soon!"); |
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.
This line is redundant. Needs to be displayed only when 'bye' is entered by user.
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. Can have better coding standards.
src/main/java/bro.java
Outdated
|
||
public class bro { | ||
|
||
private static final ArrayList<String> storer = new ArrayList<>(); |
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.
Have a better variable name. Can specify what 'storer' stores?
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 have no idea what's going on here. Code does not even do what is expected from the increments. Lots of improvements can be made. Also remove redundant functions.
src/main/java/bro.java
Outdated
System.out.println("Bye. Hope to see you again soon!"); | ||
} | ||
|
||
public static void echo() { |
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.
Did not remove redundant function that is no longer used.
src/main/java/bro.java
Outdated
private static final ArrayList<String> storer = new ArrayList<>(); | ||
private static final ArrayList<String> mark_tracker = new ArrayList<>(); | ||
|
||
public static void level0() { |
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.
Did bro hard code level 0 ? 😭 Did not remove redundant function again.
src/main/java/bro.java
Outdated
public class bro { | ||
|
||
private static final ArrayList<String> storer = new ArrayList<>(); | ||
private static final ArrayList<String> mark_tracker = new ArrayList<>(); |
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 have use a seperate class for Tasks to track the info and marking
src/main/java/bro.java
Outdated
String line; | ||
Scanner in = new Scanner(System.in); | ||
|
||
while (true) { |
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.
Wait if you have while(true) loop in your addList and you call your mark only after addList, wouldnt you be marking after the user inputs bye.
src/main/java/bro.java
Outdated
String line; | ||
Scanner in = new Scanner(System.in); | ||
|
||
while(true) { |
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.
Bro 😭, You can't add tasks once you are in marking mode ?
Agreed that I should put more time into it but the code does output what was needed for the first three levels. Just I haven't really cleaned the code yet but functionality is there. Also didn't upload part 4 yet; do you mind checking again once I updated my code. |
src/main/java/bro.java
Outdated
import java.util.ArrayList; | ||
import java.io.*; | ||
|
||
public class bro { |
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 class name bro should follow Java naming conventions. Use PascalCase for class names. Consider renaming it to something more descriptive, like TaskManager or TaskApp.
src/main/java/bro.java
Outdated
|
||
} | ||
|
||
public static void addList() { |
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.
Use descriptive names for your methods. For example, addList could be renamed to manageTasks, as it implies that this method is responsible for task management
src/main/java/bro.java
Outdated
while (true) { | ||
line = in.nextLine(); | ||
|
||
if (line.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.
Avoid using magic strings directly in your code (e.g., "Bye", "list", "todo"). Instead, consider defining constants at the beginning of your class to improve readability and maintainability.
src/main/java/bro.java
Outdated
saveTasks(); | ||
} | ||
|
||
} else if (line.startsWith("deadline")) { |
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 sections handling different task types (Todo, Deadline, Event) have similar patterns for adding and saving tasks. Can create a helper method to reduce duplication
src/main/java/bro.java
Outdated
storer.add(todo); | ||
break; | ||
case "D": | ||
Deadline ddl = new Deadline(parts[2], parts[3]); |
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.
Variables like ddl, taskInt, and task_num can be more descriptive. For instance, use deadlineTask instead of ddl, and taskIndex instead of taskInt. Consistency in naming conventions (camelCase) is also important
src/main/java/bro.java
Outdated
|
||
} | ||
|
||
public static void addList() { |
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 addList method is quite long. Break it down into smaller methods for each task type handling to improve readability and maintainability.
I am a person.