-
Notifications
You must be signed in to change notification settings - Fork 192
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
[T4A5][T11-B3] #1685
base: master
Are you sure you want to change the base?
[T4A5][T11-B3] #1685
Conversation
…stal and block classes validate whether the correct number has been entered and if so how many digit numbers have been added (postal code has 6 digits).
…Phone classes are extended/inherited.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dheerajaraj Some comments added. Please close the PR after reading
Avoid accumulating unrelated changes into a single PR.
Tip: If you are currently on branch1
and want to create a new branch2
, you need to switch back to master
branch before creating branch2
. If not, changes in branch1
will appear in the PR created from branch2
later.
import seedu.addressbook.data.person.*; | ||
import seedu.addressbook.data.tag.UniqueTagList.DuplicateTagException; | ||
|
||
public class Tagging { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing header comment
public class Tagging { | ||
private int status; | ||
private Person person; | ||
private UniqueTagList alltags; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
based on the diagram given, this should not be here. Tagging
is just the association between a Person
and a Tag
import seedu.addressbook.data.tag.UniqueTagList.DuplicateTagException; | ||
|
||
public class Tagging { | ||
private int status; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better to use enum
} | ||
|
||
public String toString(){ | ||
String action = (status==1) ? "+" : "-"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
avoid magic numbers. what is 1?
for(Tag tag: alltags){ | ||
result += tag.toString(); | ||
} | ||
return result; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coding standard violation! One more more of these problems detected:
- extra/missing spaces.
- brace style problem.
- missing braces.
- name doesn't follow naming convention
|
||
} | ||
|
||
public Tagging(int status, ReadOnlyPerson targetPerson, Tag tag) throws DuplicateTagException{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be Person
, not ReadOnlyPerson
No description provided.