Skip to content

[Bryan] iP - #89

Open
Bryan-BC wants to merge 15 commits into
nus-cs2113-AY2122S2:masterfrom
Bryan-BC:master
Open

Bryan-BC wants to merge 15 commits into
nus-cs2113-AY2122S2:masterfrom
Bryan-BC:master

Conversation

@Bryan-BC

@Bryan-BC Bryan-BC commented Feb 4, 2022

Copy link
Copy Markdown

No description provided.

@YitHien YitHien left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great code overall! LGTM

Comment thread src/main/java/Duke.java Outdated
Comment on lines +65 to +68
ind = Integer.parseInt(commands[1]) - 1;
printMark(ind);
} else if (commands[0].equals("unmark")) {
ind = Integer.parseInt(commands[1]) - 1;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps a more descriptive variable name would be better instead of 'ind'?

Comment thread src/main/java/Duke.java Outdated
}

public static void printList() {
System.out.println("Hemre are the tasks im your list:");

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was initially confused if you made a typo; realized it was the bot's personality! Nice!

Comment thread src/main/java/Duke.java Outdated
public static void printList() {
System.out.println("Hemre are the tasks im your list:");
for (int i = 0; i < index; i++) {
String number = String.valueOf(i + 1) + ". ";

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think 'number' could be better named to describe the numbering of the list items!

Comment thread src/main/java/Duke.java Outdated
@@ -1,10 +1,85 @@
import java.util.Scanner;

public class Duke {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps you can consider adding some comments to describe each method? The format can be found here under 'Comments': https://se-education.org/guides/conventions/java/basic.html

Comment thread src/main/java/Duke.java Outdated
String command = in.nextLine();
String[] commands = command.split(" ");
System.out.println(separator);
do {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good bracing positioning! You may consider using a switch-case if you like!

@FTang21 FTang21 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall well done!

Comment thread src/main/java/Duke.java Outdated
command = in.nextLine();
commands = command.split(" ");
System.out.println(separator);
} while (!command.equals("bye"));

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe consider taking the string literals and turning them in Magic constants or enums?

Comment thread src/main/java/Duke.java Outdated

public class Duke {
private static int index = 0;
private static String separator = "-------------------------------------------";

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Separator here acts like a magic constant. Maybe consider declaring it final and making it capitalized?

Comment thread src/main/java/Duke.java Outdated
}

public static void farewell() {
String farewell = "Goodbye. See you next time frem!\n" + "-------------------------------------------";

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can probably use the separator variable here.

@Mick609 Mick609 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code clean and readable. Good job.

Comment thread src/main/java/Duke.java Outdated
private static final String SEPARATOR = "-------------------------------------------";
private static int numberOfTasks = 0;
private static Task[] tasks = new Task[100];
private static Scanner in = new Scanner(System.in);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you could use a more descriptive name for the object. Maybe "inScanner" ?

Comment thread src/main/java/Duke.java Outdated
} catch (NumberFormatException e) {
System.out.println("Error! Please input a number");
} finally{
System.out.println(SEPARATOR);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Different errors are handled well here.

Comment thread src/main/java/Duke.java Outdated
*/
public static void handleUnmark(String command) {
try{
int taskPosition = Integer.parseInt(command.substring(7)) - 1;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you could avoid the usage of the magic number, like the 7 here.

Comment thread src/main/java/Duke.java Outdated
System.out.println(SEPARATOR);
do {
try{
if (command.startsWith("list")) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you could store these starting Strings as String variables.

Comment thread src/main/java/Duke.java Outdated
* Creates a new ToDo and prints the new ToDo.
*
* @param command The command given from input
*/

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments are informative.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants