Skip to content

[JinbaoAlex] iP#186

Open
JinbaoAlex wants to merge 28 commits intonus-cs2113-AY2425S2:masterfrom
JinbaoAlex:master
Open

[JinbaoAlex] iP#186
JinbaoAlex wants to merge 28 commits intonus-cs2113-AY2425S2:masterfrom
JinbaoAlex:master

Conversation

@JinbaoAlex
Copy link
Copy Markdown

No description provided.

Copy link
Copy Markdown

@jiajun2002 jiajun2002 left a comment

Choose a reason for hiding this comment

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

The code is short and concise! Nice to read :) I made some minor notes regarding naming and using constants!

Comment on lines +6 to +10
public static String formatResponse(String msg) {
return " ____________________________________________________________\n"
+ msg + "\n"
+ " ____________________________________________________________\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.

I like that you have a function for formatting the code! Perhaps consider adding a string constant for the linebreak instead of manually typing out "_______________" each time

Comment on lines +15 to +18
String msg = " Got it. I've added this task:\n"
+ " " + t.toString() + "\n"
+ " Now you have " + storeTaskIndex + " tasks in the list.";
return formatResponse(msg);
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 consider storing the " " spaces in a string as well! Could save some time so you don't have to manually type out the spaces, and also minimises the risk of typing an incorrect number of spaces

String endDate = input.substring(indexOfEndDate + 4);
Event t = new Event(taskDesc, startDate, endDate);
System.out.println(formatTaskMsg(t));

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 like the way you name your variables! They are very clear :)


} else if (input.equals("list")) {
StringBuilder sb = new StringBuilder();
boolean first = true;
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 renaming the boolean to isFirst would be clearer?

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 like that the code complies with the Java Coding Standard.

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 would recommend including getters and setters methods in the Task class.

int indexOfDeadline = input.indexOf(" /by");
String taskDesc = input.substring(9, indexOfDeadline);
String deadline = input.substring(indexOfDeadline + 4);
Deadline t = new Deadline(taskDesc, deadline);
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 should assign a meaningful name to the deadline and event Tasks instead of 't'.

System.out.println(formatResponse(sb.toString()));

} else if (input.startsWith("mark")) {
int taskNo = Integer.parseInt(input.substring(5)) - 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 'taskNumber' would be a clearer variable name than 'taskNo'?

Copy link
Copy Markdown

@aditichadha1310 aditichadha1310 left a comment

Choose a reason for hiding this comment

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

overall good work!

StringBuilder sb = new StringBuilder();
sb.append(" Here are the tasks in your list:");
for (int i = 0; i < listOfTasks.size(); i++) {
sb.append("\n ").append(i + 1).append(".").append(listOfTasks.get(i));
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Avoid usage of magic strings and numbers

if (taskDesc.trim().isEmpty()) {
throw new MissingDescription("MISSING DESCRIPTION, reenter with description");
}
Todo t = new Todo(taskDesc);
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 might want to name "t" to a more intuitive name so that it increases readability.

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