feat: fluent use of `Result` #1837

paullatzelsperger · 2022-08-16T15:29:55Z

What this PR changes/adds

adds some syntactic sugar to the Result and AbstractResult class to make them usable in fluent statements.

Why it does that

Enable code fluency and more descriptive and succinct code pieces.

Further notes

I tried to add the methods to the base class wherever possible to make them available to all subclasses.

Linked Issue(s)

Closes #1823

Checklist

added appropriate tests?
performed checkstyle check locally?
added/updated copyright headers?
documented public classes/methods?
added/updated relevant documentation?
assigned appropriate label? (exclude from changelog with label no-changelog)
formatted title correctly? (take a look at the CONTRIBUTING and styleguide for details)

paullatzelsperger · 2022-08-16T15:31:08Z

@ndr-brt still in draft, but since the issue (and preceding discussion) wasn't too firm on specifics, lets use this PR to align further.

spi/core-spi/src/main/java/org/eclipse/dataspaceconnector/spi/result/Result.java

codecov-commenter · 2022-08-16T15:49:57Z

Codecov Report

Merging #1837 (c544881) into main (08f8d5e) will increase coverage by 0.06%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1837      +/-   ##
==========================================
+ Coverage   62.47%   62.53%   +0.06%     
==========================================
  Files         779      779              
  Lines       16608    16632      +24     
  Branches     1085     1085              
==========================================
+ Hits        10376    10401      +25     
  Misses       5781     5781              
+ Partials      451      450       -1

Impacted Files	Coverage Δ
.../dataspaceconnector/spi/result/AbstractResult.java	`100.00% <100.00%> (+10.00%)`	⬆️
.../eclipse/dataspaceconnector/spi/result/Result.java	`95.83% <100.00%> (+2.50%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

ndr-brt · 2022-08-17T07:22:02Z

spi/core-spi/src/main/java/org/eclipse/dataspaceconnector/spi/result/AbstractResult.java

+    /**
+     * Executes a {@link Consumer} if this {@link Result} is successful
+     */
+    public void ifSuccess(Consumer<T> successAction) {


I would make this return this as it could be used in a fluent context, e.g. to print a log and then the chain could continue, with a map.
same for the ifFailure

the ifSuccess was intended to execute a conditional action, similar to Optional.ifPresent and I would suggest keeping semantic consistency.

The whenSuccessful is intended for usage in fluent blocks. If anything I would consolidate AbstractResult.whenSuccessul and Result.whenSuccess, keeping the latter.

Conversely, we can indeed return this from the ifFailure method, as it is semantically different from orElse and could then be used as initial statement in a fluent block:

return result.ifFailure(...).mapTo(...);

to be honest I have some opinions about the Optional interface :)
the whenSuccessful is different since the action get the whole result (that's probably not needed).
I would keep only one method that will execute an action on success and one that will do the same on failure.

Why do not call them onSuccess and onFailure?

maybe we can even simplify it a bit:

have one method each for success and failure (FWIW lets call them onSuccess and onFailure, as you suggested), receiving either the content or the Failure, returning this

have one method called flatMap that accepts a mapping function and converts one Result into another. This would get rid of the whenSuccess(ful) stuff for now

that way, we can see if that fits the bill and extend later.

ndr-brt · 2022-08-17T07:55:45Z

spi/core-spi/src/main/java/org/eclipse/dataspaceconnector/spi/result/Result.java

+     * @return {@link Result#failure(String)} if the Optional is empty, {@link Result#success(Object)} using the Optional's value otherwise.
+     */
+    @SuppressWarnings("OptionalUsedAsFieldOrParameterType")
+    public static <T> Result<T> ofOptional(Optional<T> opt) {


fromOptional could fit better?

or just from?

ndr-brt · 2022-08-17T07:59:34Z

spi/core-spi/src/main/java/org/eclipse/dataspaceconnector/spi/result/Result.java

+     */
+    public <R> Result<R> mapTo() {
+        if (succeeded()) {
+            return new Result<>(null, null);


I'm not sure about this, I'm wondering what could be the use of this function, mapping it to another type but with the content discarded... how this could be helpful?
Maybe only a mapToEmpty/mapEmpty that returns a Result<Void> would be just enough.

I think the more plausible case would be the failure situation, similar to:

public Result<Void> someMethod(){ Result<String> stringResult = getStringResult(); if(result.failed()){ return result.mapTo(); } // ... other code }

especially when the "other code" cannot use fluent statements

I can't think of a case where I should map the result in a different way for the failure, it should be all part of the same chain.
If we want to have some code/validations to be executed only if the result is not failed we could introduce a compose/flatMap method that would be executed only if the result is successful

I was referring to a situation where the other code stuff is quite lengthy, and may involve calling methods, that do not return a Result, but throw an exception, return a value or null, etc.

so the mapTo is mainly used to convert a failed Result into another Result implicitly.

ndr-brt · 2022-08-17T09:11:03Z

spi/core-spi/src/main/java/org/eclipse/dataspaceconnector/spi/result/AbstractResult.java

+     *
+     * @return this
+     */
+    public AbstractResult<T, F> whenSuccessful(Consumer<AbstractResult<T, F>> content) {


This is the same as ifSuccess, because this pass the whole result, but the failure will be always null as it is successful.
Same for orElseDo, I would keep only the ifSuccess/ifFailure making them return this for their fluent use.

the problem with ifFailure is a purely semantic one: we could then construct blocks like:

res.ifSuccess(...) .ifFailure(...)

which (to my ears) sounds worse than

res.ifSuccess(...) .orElse(...)

even if ifFailure and orElse perform the same function (maybe even call each other).

they could be called onSuccess and onFailure

ndr-brt · 2022-08-17T09:11:15Z

spi/core-spi/src/main/java/org/eclipse/dataspaceconnector/spi/result/AbstractResult.java

+     *
+     * @param exceptionSupplier provides an instance of the exception to throw
+     */
+    public <X extends Throwable> void orElseThrow(Supplier<? extends X> exceptionSupplier) throws X {


It could return this as well

sounds legit, would only be useful in the non-throw case though.

spi/common/core-spi/src/main/java/org/eclipse/dataspaceconnector/spi/result/Result.java

+     * @see org.eclipse.dataspaceconnector.spi.result.Result#map(Function)
+     * @see org.eclipse.dataspaceconnector.spi.result.Result#mapTo()
+     */
+    public <R> Result<R> mapTo(Class<R> clazz) {


feat: add fluent syntax to result

paullatzelsperger added the enhancement New feature or request label Aug 16, 2022

paullatzelsperger requested a review from ndr-brt August 16, 2022 15:30

paullatzelsperger changed the title ~~Feature: fluent use of Result~~ feat: fluent use of Result Aug 16, 2022

github-advanced-security bot found potential problems Aug 16, 2022

View reviewed changes

spi/core-spi/src/main/java/org/eclipse/dataspaceconnector/spi/result/Result.java Fixed Show fixed Hide fixed

ndr-brt reviewed Aug 17, 2022

View reviewed changes

paullatzelsperger force-pushed the feature/1823_fluent_use_of_result branch from 9facf0f to c544881 Compare August 19, 2022 06:02

github-advanced-security bot found potential problems Aug 19, 2022

View reviewed changes

spi/common/core-spi/src/main/java/org/eclipse/dataspaceconnector/spi/result/Result.java Fixed Show fixed Hide fixed

paullatzelsperger force-pushed the feature/1823_fluent_use_of_result branch 4 times, most recently from ac7896f to 345db9e Compare August 19, 2022 06:39

feat: add fluent syntax to result

1fa9d16

paullatzelsperger force-pushed the feature/1823_fluent_use_of_result branch from 345db9e to 1fa9d16 Compare August 19, 2022 06:40

paullatzelsperger marked this pull request as ready for review August 19, 2022 06:41

paullatzelsperger requested a review from ndr-brt August 19, 2022 06:41

github-advanced-security bot found potential problems Aug 19, 2022

View reviewed changes

ndr-brt approved these changes Aug 19, 2022

View reviewed changes

paullatzelsperger merged commit 9d1e5b4 into eclipse-edc:main Aug 19, 2022

janpmeyer pushed a commit to FraunhoferISST/edc-connector that referenced this pull request Aug 23, 2022

feat: fluent use of Result (eclipse-edc#1837)

f397bf6

feat: add fluent syntax to result

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: fluent use of `Result` #1837

feat: fluent use of `Result` #1837

paullatzelsperger commented Aug 16, 2022 •

edited

Loading

paullatzelsperger commented Aug 16, 2022

codecov-commenter commented Aug 16, 2022 •

edited

Loading

ndr-brt Aug 17, 2022

paullatzelsperger Aug 18, 2022 •

edited

Loading

ndr-brt Aug 18, 2022

paullatzelsperger Aug 18, 2022

ndr-brt Aug 17, 2022

paullatzelsperger Aug 18, 2022

ndr-brt Aug 17, 2022

paullatzelsperger Aug 18, 2022

ndr-brt Aug 18, 2022 •

edited

Loading

paullatzelsperger Aug 18, 2022

ndr-brt Aug 17, 2022

paullatzelsperger Aug 18, 2022

ndr-brt Aug 18, 2022

ndr-brt Aug 17, 2022

paullatzelsperger Aug 18, 2022

feat: fluent use of Result #1837

feat: fluent use of Result #1837

Conversation

paullatzelsperger commented Aug 16, 2022 • edited Loading

What this PR changes/adds

Why it does that

Further notes

Linked Issue(s)

Checklist

paullatzelsperger commented Aug 16, 2022

codecov-commenter commented Aug 16, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

paullatzelsperger Aug 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ndr-brt Aug 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feat: fluent use of `Result` #1837

feat: fluent use of `Result` #1837

paullatzelsperger commented Aug 16, 2022 •

edited

Loading

codecov-commenter commented Aug 16, 2022 •

edited

Loading

paullatzelsperger Aug 18, 2022 •

edited

Loading

ndr-brt Aug 18, 2022 •

edited

Loading