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

[T4A2][T01-3] Teo Shu Qi #119

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

[T4A2][T01-3] Teo Shu Qi #119

wants to merge 2 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Jul 3, 2017

No description provided.

return new CommandResult(MESSAGE_EXIT_ACKNOWEDGEMENT);
}

public static boolean isExit(Command command) {
return command instanceof ExitCommand; // instanceof returns false if it is null
}

/**
* Print list of tags added or deleted each person

Choose a reason for hiding this comment

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

Prints

for ( Tagging tagging : allTagging) {
System.out.println(tagging.toString());
}
}

Choose a reason for hiding this comment

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

Good job for writing this method.

*/
public void printTagging() {
ArrayList<Tagging> allTagging = addressBook.getAllTagging();
for ( Tagging tagging : allTagging) {

Choose a reason for hiding this comment

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

Pay attention to the whitespace

@@ -24,13 +26,15 @@

private final UniquePersonList allPersons;
private final UniqueTagList allTags; // can contain tags not attached to any person
private ArrayList<Tagging> allTagging;

Choose a reason for hiding this comment

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

Good job for creating the Tagging list here.

/**
* Update Tags added for each person
* @param tag
* @param person

Choose a reason for hiding this comment

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

Better write a short description for each @param

@@ -120,6 +141,12 @@ public UniquePersonList getAllPersons() {
public UniqueTagList getAllTags() {
return new UniqueTagList(allTags);
}
/**
* Returns a new ArrayList of all Tagging in the address book at the time if the call

Choose a reason for hiding this comment

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

Returns latest tagging list or something similar to that is suffice alrady.

import seedu.addressbook.data.person.ReadOnlyPerson;

/**
* Association class representing an addition/deletion of Tags for the person

Choose a reason for hiding this comment

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

A more precise description is Represents an adding or removing a Tag to/from a Person.

private ReadOnlyPerson person;
private String personName;

public Tagging(Tag tag, String action, ReadOnlyPerson person2) {

Choose a reason for hiding this comment

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

what is person2? you better use person


public Tagging(Tag tag, String action, ReadOnlyPerson person2) {
this.tag = tag;
this.action = action;

Choose a reason for hiding this comment

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

Better create different method for adding/removing a tag.

@jeffryhartanto
Copy link

@teoshuqi
Some comments added. Please close the PR after reading comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants