Abyss-lord commented on PR #1411: URL: https://github.com/apache/commons-lang/pull/1411#issuecomment-3066721602
> Hi @Abyss-lord, > > Thank you for your PR! > > At first glance, I'm not convinced that the proposed methods offer a clear improvement over existing options. For example, consider the following comparisons: > > ```java > Iterator<String> iterator = ... > Object[] objectArray = ArrayUtils.iteratorToArray(iterator); > String[] stringArray = ArrayUtils.iteratorToArray(iterator, String.class); > ``` > > versus the equivalent using existing `Streams` support: > > ```java > Iterator<String> iterator = ... > Object[] objectArray = Streams.of(iterator).toArray(); > String[] stringArray = Streams.of(iterator).toArray(String[]::new); > ``` > > Functionally, these are very similar. The main difference is that the `ArrayUtils` versions return `null` when the input is `null`, which—depending on perspective—can be a pro or a con. Personally, I view this as a downside: I prefer to eliminate `null`s early and treat empty arrays and `null`s as equivalent. > > Could you share more context on your use case or motivation for introducing these methods? That might help clarify their intended value. > > Also, setting aside the question of inclusion, I believe the proposed implementations need some adjustments. I've left comments on the relevant lines for review. > > Thanks again for the contribution—looking forward to your thoughts! > Hi @Abyss-lord, > > Thank you for your PR! > > At first glance, I'm not convinced that the proposed methods offer a clear improvement over existing options. For example, consider the following comparisons: > > ```java > Iterator<String> iterator = ... > Object[] objectArray = ArrayUtils.iteratorToArray(iterator); > String[] stringArray = ArrayUtils.iteratorToArray(iterator, String.class); > ``` > > versus the equivalent using existing `Streams` support: > > ```java > Iterator<String> iterator = ... > Object[] objectArray = Streams.of(iterator).toArray(); > String[] stringArray = Streams.of(iterator).toArray(String[]::new); > ``` > > Functionally, these are very similar. The main difference is that the `ArrayUtils` versions return `null` when the input is `null`, which—depending on perspective—can be a pro or a con. Personally, I view this as a downside: I prefer to eliminate `null`s early and treat empty arrays and `null`s as equivalent. > > Could you share more context on your use case or motivation for introducing these methods? That might help clarify their intended value. > > Also, setting aside the question of inclusion, I believe the proposed implementations need some adjustments. I've left comments on the relevant lines for review. > > Thanks again for the contribution—looking forward to your thoughts! @ppkarwasz , thank you for the detailed review and thoughtful feedback! The reason I added this method is because I came across a related request in a JIRA issue, where someone expressed the need for this functionality. As a newcomer to the Commons Lang community, I wanted to start contributing by picking up something small but potentially useful. Also, could you please advise where I can find guidelines or existing examples to help identify the kinds of utility methods that are considered suitable for inclusion in Commons Lang? That would really help me better align future contributions. Thanks again! -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@commons.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
