Skip to content
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

[LANG-1634] Add ObjectUtils #applyIfNonNull and #applyFirstNonNull #684

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

bindul
Copy link

@bindul bindul commented Dec 28, 2020

Utility methods that take a java.util.function.Consumer and possibly null value(s). The consumer is invoked if the value is not null or with the first non-null value, respectively.

See LANG-1634 and discussion at [lang] ObjectUtils enhancement - consumer with non-null value

* </p>
*
* <pre>
* ObjectUtils.applyIfNonNull(bean::setValue, null) - setValue not invoked
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo in method name

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Corrected, thanks.

@coveralls
Copy link

coveralls commented Dec 28, 2020

Coverage Status

Coverage increased (+0.005%) to 94.986% when pulling adf1e04 on bindul:LANG-1634_ObjectUtils-applyIfNonNull into 6b03fe5 on apache:master.

@garydgregory
Copy link
Member

garydgregory commented Dec 29, 2020

It seems to me the true test of a new API like applyIfNonNull is where can it be used within Commons Lang, if we don't eat our own dog food, it seems odd to tell others to do so. IOW, I'd like to see this PR include using this API.

I'm not convinced as to the utility of applyFirstNonNull, see above.

Also, for my money, I'd flip the arguments:

ObjectUtils.applyIfNonNull(null, bean::setValue);

Which reads to me like "if the object is non-null, then apply the function" but then maybe the method should be ifNonNullApply, at least that's how my left-to-right reading brain works ;-)

@@ -226,6 +227,62 @@ public static boolean anyNotNull(final Object... values) {
return firstNonNull(values) != null;
}

/**
* <p>
* Invokes the given {@code consumer's} {@link Consumer#accept(Object)} with the first {@code non-null} value from
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, you're not "invoking", you're "accepting", so the Javadoc should say "Accepts...". Actually, the method is misnamed since the underlying method is Consumer#accept(), this method should be use "accept", not "apply".

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have changed the method name to 'accept...' and updated the Javadoc comment accordingly.

* @see #applyFirstNonNull(Consumer, Object...)
* @since 3.12
*/
public static <T> void applyIfNonNull(final Consumer<T> consumer, final T object) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See "accept" comment above and other comment in the Conversation stream of this PR.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have changed the method name to 'accept...', switched the parameter order and updated the Javadoc comment accordingly.

return value;
}
public void setValue(String value) {
this.value = value;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can add a call to Objects.requireNonNull() here instead of the blind assignment:

this.value = Objects.requireNonNull(value, "value");

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This inner class was meant as a simple Java bean with a setter and getter, hence did not have any checks in it. I have changed it now.

bindul added a commit to bindul/commons-lang that referenced this pull request Dec 29, 2020
- Changed method names from applyIfNonNull -> acceptIfNonNull; and
applyFirstNonNull -> acceptFirstNonNull
- Changed parameter order
@bindul bindul force-pushed the LANG-1634_ObjectUtils-applyIfNonNull branch from e5de898 to cbcc405 Compare December 29, 2020 20:32
@bindul
Copy link
Author

bindul commented Dec 29, 2020

@garydgregory

I have renamed the methods to accept from apply and switched the parameter order where possible.

acceptFirstNonNull is the analogous method to existing ObjectUtils.firstNonNull or StringUtils.firstNonBlank. If I remember correctly, I have had one use for this method in my real life code, where a configuration parameter value could be supplied at multiple levels (specific instance, application default or global default; the first two could be null) and I have used a similar method to handle this. If you don't want to include it, I am happy to remove this.

In a real life scenario, the use case for the acceptIfNonNull method is usually when setting configuration values. The driver (say for something like redis, solr, etc.) or library (like a HTTP client library) accepts a configuration bean with some default values set. Your application accepts configuration values, and you only want to set those in the configuration bean if they are not null (or you lose the default). Anyways, I have updated 3 existing commons-lang classes to show how it can reduce 3 lines of code to 1. I did not go and update every possible occurrence, as that would make this PR large and lose focus on the primary change in the PR.

Please review and let me know if I should make further modifications.

bindul added a commit to bindul/commons-lang that referenced this pull request Dec 29, 2020
- Changed method names from applyIfNonNull -> acceptIfNonNull; and
applyFirstNonNull -> acceptFirstNonNull
- Changed parameter order
@bindul bindul force-pushed the LANG-1634_ObjectUtils-applyIfNonNull branch from cbcc405 to 0128401 Compare December 29, 2020 20:44
bindul added a commit to bindul/commons-lang that referenced this pull request Dec 30, 2020
- Changed method names from applyIfNonNull -> acceptIfNonNull; and
applyFirstNonNull -> acceptFirstNonNull
- Changed parameter order
@bindul bindul force-pushed the LANG-1634_ObjectUtils-applyIfNonNull branch from 0128401 to d5cdd4e Compare December 30, 2020 01:49
bindul added a commit to bindul/commons-lang that referenced this pull request Jan 1, 2021
- Changed method names from applyIfNonNull -> acceptIfNonNull; and
applyFirstNonNull -> acceptFirstNonNull
- Changed parameter order
@bindul bindul force-pushed the LANG-1634_ObjectUtils-applyIfNonNull branch from d5cdd4e to 6ae1ac0 Compare January 1, 2021 03:06
bindul added a commit to bindul/commons-lang that referenced this pull request Jan 4, 2021
- Changed method names from applyIfNonNull -> acceptIfNonNull; and
applyFirstNonNull -> acceptFirstNonNull
- Changed parameter order
@bindul bindul force-pushed the LANG-1634_ObjectUtils-applyIfNonNull branch from 6ae1ac0 to bde0870 Compare January 4, 2021 19:47
@garydgregory
Copy link
Member

@bindul Please rebase on master. TY!

bindul added a commit to bindul/commons-lang that referenced this pull request Jan 6, 2021
- Changed method names from applyIfNonNull -> acceptIfNonNull; and
applyFirstNonNull -> acceptFirstNonNull
- Changed parameter order
@bindul bindul force-pushed the LANG-1634_ObjectUtils-applyIfNonNull branch from bde0870 to 7684dac Compare January 6, 2021 21:52
@bindul
Copy link
Author

bindul commented Jan 6, 2021

@garydgregory Done!
Please let me know if you want me to squash commits into logical groups (I am thinking 1 with the change and the changes for review comments and another where the new method is used elsewhere in lang)

bindul added a commit to bindul/commons-lang that referenced this pull request Jan 7, 2021
- Changed method names from applyIfNonNull -> acceptIfNonNull; and
applyFirstNonNull -> acceptFirstNonNull
- Changed parameter order
@bindul bindul force-pushed the LANG-1634_ObjectUtils-applyIfNonNull branch from 7684dac to 1813f18 Compare January 7, 2021 17:07
bindul added a commit to bindul/commons-lang that referenced this pull request Jan 15, 2021
- Changed method names from applyIfNonNull -> acceptIfNonNull; and
applyFirstNonNull -> acceptFirstNonNull
- Changed parameter order
@bindul bindul force-pushed the LANG-1634_ObjectUtils-applyIfNonNull branch from 1813f18 to 2e65abd Compare January 15, 2021 17:55
Utility methods that take a java.util.function.Consumer and possibly
null value(s). The consumer is invoked if the value is not null or with
the first non-null value, respectively.
- Changed method names from applyIfNonNull -> acceptIfNonNull; and
applyFirstNonNull -> acceptFirstNonNull
- Changed parameter order
Update a few existing classes to use ObjectUtils#acceptIfNonNull(Object,
Consumer)
@bindul bindul force-pushed the LANG-1634_ObjectUtils-applyIfNonNull branch from 2e65abd to adf1e04 Compare January 23, 2021 19:20
@singhbaljit
Copy link

We could really use this method. This is helpful because it removes the if/else code branch from the application codes, and therefore, devs don't have to write trivial unit tests to meet the code coverage requirements.

* @see #acceptFirstNonNull(Consumer, Object...)
* @since 3.12
*/
public static <T> void acceptIfNonNull(final T object, final Consumer<T> consumer) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

more user-friendly: Consumer<? super T> consumer.

Also, requireNonNull(consumer, "consumer").

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants