Converted all lists with no special purpose into iterable/collection … #180
Conversation
…and rewrote some tests to reflect the change. Resolved #156.
| for (var i = 0; i < parameters.length; ++i) { | ||
| if (i >= values.size() || values.get(i) == null) { | ||
| Object currentValue = valueIterator.next(); |
There was a problem hiding this comment.
It looks like this line may throw a NoSuchElementException, due to not checking Iterable#hasNext() before calling Iterable#next().
| @@ -146,7 +146,7 @@ public <S, T> Collection<T> map(Collection<S> collection, Class<T> elementInColl | |||
| * @param <T> the class type of an element in the target list | |||
| * @return the target list with mapped source list elements | |||
| */ | |||
| public <S, T> List<T> map(List<S> list, Class<T> elementInListClass) { | |||
| public <S, T> Iterable<T> map(Iterable<S> list, Class<T> elementInListClass) { | |||
There was a problem hiding this comment.
BeanMapper#map(List, Class) is a convenience method which allows for mapping from a List, to a List. In this case, the occurrence of List is a required part of the functionality. As such, this change should be reverted. However, BeanMapper#map(Iterable, Class) might be a good addition. Adding such functionality would probably be best placed in a separate issue and PR.
| @@ -348,7 +351,7 @@ void mapListCollectionWithNested() { | |||
| add(new Address("MuisLaan", 1, "Frankrijk")); | |||
| }}; | |||
| List<ResultAddress> targetItems = | |||
| beanMapper.map(sourceItems, ResultAddress.class); | |||
| (List<ResultAddress>) beanMapper.map(sourceItems, ResultAddress.class); | |||
There was a problem hiding this comment.
This change, and similar changes would need to be reverted, due to reverting BeanMapper#map(Iterable, Class) back to BeanMapper#map(List, Class).
There was a problem hiding this comment.
Overall, the changes look good, but I would like to request a few changes.
- Revert BeanMapper#map(Iterable, Class) back to BeanMapper#map(List, Class).
- Gracefully handle a possible NoSuchElementException in MapToRecordStrategy#getConstructorArgumentsMappedToCorrectTargetType(Parameter[], Collection), caused by an unsafe call to Iterator#next().
- Revert changes in tests relating to the conversion of BeanMapper#map(List, Class) to BeanMapper#map(Iterable, Class).
Then a little nitpick that wouldn't be cause for blocking the PR:
- The changes leave some unused imports here and there. Could you remove those where applicable?
Lastly, could you PR into dev/4.1.2 rather than master?
Thank you for your contribution!
…and rewrote some tests to reflect the change. Resolved #156.