-
Notifications
You must be signed in to change notification settings - Fork 5
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
Add checks for assign, unassign and reassign commands #110
Add checks for assign, unassign and reassign commands #110
Conversation
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.
LGTM, just a couple of very small comments only!
* This defines a stronger notion of equality between two role requirements. | ||
*/ | ||
@Override | ||
public boolean equals(Object other) { | ||
return other == this | ||
|| (other instanceof RoleRequirement | ||
&& role.equals(((RoleRequirement) other).role) | ||
&& quantity == ((RoleRequirement) other).quantity); | ||
&& quantityRequired == ((RoleRequirement) other).quantityRequired | ||
&& quantityRequired == ((RoleRequirement) other).quantityRequired); |
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.
Possible duplication?
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.
Good catch, thank you!
* Returns true if the quantity filled has not reached the quantity required. | ||
*/ | ||
public boolean isFilled() { | ||
assert quantityFilled <= quantityRequired; |
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.
Would it be more meaningful to put this assertion in the constructor instead? Just a suggestion, not sure if it's good practice
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.
Makes sense, edited
Resolves #55