richdougherty commented on PR #1435: URL: https://github.com/apache/commons-lang/pull/1435#issuecomment-3219106200
Hi @garydgregory Thanks for the quick feedback! > That said I would simply call the method "apply" and the leave non-null semantics as Javadoc. Sure, happy to fit into the pattern of the class and name it `apply`. > Why 3 overloads? As soon as there are n overloads, someone will submit a request for n+1. > > I'd just go with a vararg and be done with it. OK I see your point. How about I go with a single-function version only, so there are no overloads? We can always chain like follows, which is type-safe and avoid multiple overloads. ``` ObjectUtils.apply(ObjectUtils.apply(peopleMap.get(key), Person::getPet), Pet::getName) ``` I did consider varargs, but the issue is that the type of the input, intermediate and return values can differ, and we can't represent that safely in an array of functions. All the functions would need to have the same time - or no type! It would be easy for users to make mistakes and get runtime errors that could otherwise be avoided. So I think a single overload is cleaner if that works. > There are edge cases that could be surprising: What if one of the given functions throws an NPE? Should that cause 'apply' to return null? It might be surprising if a method designed to handle throws an NPE. My preference is to throw any exceptions that the function throws. Perhaps that is surprising though - I can document it, especially that NPEs can be thrown.. I think the method has a simple clear behaviour - it handles null-ness of the `value` parameter, calling if non-null. Doing more than this might decrease the utility of the method because it's making stronger assumptions about the behaviour that the user wants. By having a more minimal behaviour it satisfies those who want a very basic behaviour, but it's still possible to combine the method with others to get more complex behaviour, e.g. make an `npeToNull(mapper)` combinator for example. > What if I want to call a method that throws a checked exception? I guess we'd need a FailableFunction version or only have a FailableFunction version (since you can default it to a runtime exception). I did create FailableFunction versions at first, but then removed them to simplify. How about - especially if we're just looking at a single `apply` method - I add a `failableApply` method as well? A different name rather than an overload of `apply` to avoid ambiguity when using type inference on lambda expressions. ``` ObjectUtils.failableApply(value, v -> dangerousOp(v)) ``` > Is the AI you used compatible with the Apache License? If you don't know, then a clean room implementation is the way to go. Why in the world do you need AI for such simple code btw? Just to be clear I wrote the (very simple) logic myself - I definitely didn't need AI to do that! However, I'm declaring AI usage just to be upfront, because I just used it as a peer review to check for any typos, inconsistencies etc. e.g. "can you please review this code and identify any errors or inconsistencies in comments or code". It had a couple of suggestions, which I fixed, e.g. repeating mapper2 in one place, instead of mapper3. (I wrote the fixes) To be safe I have double checked the terms of usage for the AI. No ownership is claimed, and it's not copyright, since the code is original and not what the AI suggested. Again - thanks for the quick feedback. Let me know if I should go ahead with these suggestions: - single overload - `apply` (remove others) - extra version - `failableApply` I think even this simple `apply` method would be really handy! I think others would find it useful as well. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
