-
Notifications
You must be signed in to change notification settings - Fork 581
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
Introduce ImmutableListFactory.ofNCopiesOf
method with implementation
#1168
base: master
Are you sure you want to change the base?
Conversation
Also I'm not quite sure where/how to add unit tests... this project is surprisingly complex. Any pointers are appreciated. |
Partially addresses eclipse#1166; does not include primitive support yet
Hi @dmlloyd - Apologies for the late response. I took a quick look at your PR to help you with this question. For the changes, you are making in For the changes you are making on the
Hope this helps. Please reach back, if you have any more questions. Thank you for your contribution! |
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.
Needs some test coverage too
/** | ||
* This is a single element immutable List which is created by calling | ||
* Immutable.newListWith(one) method. | ||
*/ |
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.
This Javadoc looks pasted from a singleton list.
/** | ||
* @since 12.0 | ||
*/ | ||
<T> ImmutableList<T> ofNCopiesOf(int copies, T item); |
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.
We have a similar method MutableListFactory#withNValues(). I think it's awkward to have "of" in the name twice. I'm also not sure if "copies" is appropriate in the name when these are all the same item, but then again, that's true of java.util.Collections#nCopies where the name comes from.
Right now I'm leaning toward also using withNValues
as the name but I'd like to hear opinions.
|
||
ImmutableNCopiesList(T item, int copies) | ||
{ | ||
assert copies > 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.
The assertion is fine, but we shouldn't use the assert keyword.
public int indexOf(Object object) | ||
{ | ||
return Objects.equals(this.item, object) ? 0 : -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.
Could delegate to contains() here
public ImmutableList<T> select(Predicate<? super T> predicate) | ||
{ | ||
return predicate.accept(this.item) ? this : Lists.immutable.empty(); | ||
} |
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.
It's odd to see select() implemented without reject() right next to it.
Partially addresses #1166; does not include primitive support yet