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

[W8.6b][T09-B3] Kang Anmin #966

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

Conversation

EdwardKSG
Copy link

The add/delete tag command is not implemented.
All changes are based on an assumption that the add/delete tag command already exists.
Tests and docs are not updated.

EdwardKSG and others added 5 commits February 8, 2018 11:13
All changes are based on an assumption that the add/delete tag command already exists.
Tests and docs are not updated.

StringBuffer tagLog = new StringBuffer();

for(Tagging tagging: addressBook.tagLog) {

Choose a reason for hiding this comment

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

Coding standard violation. Missing an empty space.

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

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

Choose a reason for hiding this comment

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

Good, this is a correct place to put the list based on the UML.. May be tagList is a better name.

import seedu.addressbook.data.person.Person;

/**
* It records a transaction of a tag addition/deletion

Choose a reason for hiding this comment

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

Can start with Records ....

public class Tagging {
private Person person;
private Tag tag;
private boolean addition; // true means it is an addition, false means deletion

Choose a reason for hiding this comment

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

Coding standard violation for boolean.
Using boolean is fine now since this LO requires only two states. What if later you need to capture more than two states?

}

public String toString() {
return (addition?"+ ":"- ") + person.getName() + " " + tag.toString();

Choose a reason for hiding this comment

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

I think this ternary operator violates our coding standard. Missing empty spaces?

@jeffryhartanto
Copy link

@EdwardKSG
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants