Skip to content
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

[Zhao Lingshan] iP #110

Open
wants to merge 36 commits into
base: master
Choose a base branch
from

Conversation

zhaolingshan
Copy link

No description provided.

Copy link

@dearvae dearvae left a comment

Choose a reason for hiding this comment

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

Great, all codes followed the java coding standard!

}

public static void parseFind(String s) {

Copy link

Choose a reason for hiding this comment

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

why is this method empty?


public class Ui {

private final Scanner sc;
Copy link

Choose a reason for hiding this comment

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

Naming for final should be all capital letter?

public void showAddedTask(Task task, int length) {
length++;
System.out.println("Got it. I've added this task:\n" + task + "\n"
+ "Now you have " + length + " tasks in the list.\n");
Copy link

Choose a reason for hiding this comment

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

you can use ++ length on here directly instead of writing line 48

public void showList(TaskList taskList) {
for (int i = 0; i < taskList.taskListLength(); i++) {
if (i == 0) {
System.out.println("Here are the tasks in your list:\n");
Copy link

Choose a reason for hiding this comment

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

maybe you can write this outside the for loop before line 58?

Choose a reason for hiding this comment

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

Yes, if you could write that line before line 58, then the conditional check will no longer be needed and the code will be easier to read.

Copy link

@rachel170 rachel170 left a comment

Choose a reason for hiding this comment

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

Overall, I like how readable and organized your code is!
The way you made a check for details after a command is also quite interesting. However,
I can't help but notice that as a result of the method you use, you use a lot of 'magic numbers'.
As in "a number that does not explain the meaning of the number" instead of a constant.
Maybe you could consider using several constants instead?

Other than that, your code LGTM!

* @return an event.
*/
public static Event parseEvent(String display) throws DukeException {
if (display.length() == 5 || display.length() == 6) { // "event" or "event "

Choose a reason for hiding this comment

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

I like how you did the check for the command's follow-up details!
However, I can't help but notice you use a lot of numbers for your comparison and substrings. Is there any reason you do this instead of using a constant?


public void findTask(String display) {
ArrayList<String> matches = new ArrayList<>();
String task = display.substring(5);

Choose a reason for hiding this comment

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

Like previous, is there any reason you avoid using a constant for your substring() method calls?

public void showList(TaskList taskList) {
for (int i = 0; i < taskList.taskListLength(); i++) {
if (i == 0) {
System.out.println("Here are the tasks in your list:\n");

Choose a reason for hiding this comment

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

Yes, if you could write that line before line 58, then the conditional check will no longer be needed and the code will be easier to read.


public Task readFiles(String data) throws DukeException {
if (data.startsWith("T")) {
String description = data.substring(8);

Choose a reason for hiding this comment

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

Any reason why you don't use a constant?

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.

None yet

3 participants