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

Tap seems limited since you cannot halt execution on failure #181

Closed
benprime opened this issue Feb 20, 2020 · 10 comments
Closed

Tap seems limited since you cannot halt execution on failure #181

benprime opened this issue Feb 20, 2020 · 10 comments

Comments

@benprime
Copy link
Contributor

benprime commented Feb 20, 2020

Tap is very useful in that you can use it to return the passed in result and do multiple operations on a single result object. For example.

return getMyResult()
    .Tap(x => DoWork1(x))
    .Tap(x => DoWork2(x))
    .Tap(x => DoWork3(x));

However, sometimes you would like to return the result and still depend on whether the DoWork() method failed. If you could assume that Funcs passed to DoWork all returned the same result type as the "parent" result, that would be useful.

I find myself writing extensions methods like the following:

public static async Task<Result<T>> Tap<T>(this Task<Result<T>> resultTask, Func<T, Task<Result<T>>> func)
{
	var result = await resultTask.ConfigureAwait(Result.DefaultConfigureAwait);
	if (!result.IsSuccess) return result;
	var tapResult = await func(result.Value).ConfigureAwait(Result.DefaultConfigureAwait);
	return tapResult.IsFailure
		? tapResult
		: result;
}

In this case, the flow would be identical, except that execution would stop when a tap Func returns a failed result.

What is the reasoning behind having the result of a Tap operation be ignored in the railway flow?

@vkhorikov
Copy link
Owner

You should be able to use Bind for this. Tap is only for when you want to ignore the result entirely.

The reason for the separation between Bind and Tap (previously, they all were overloads of OnSuccess) is to separate OnSuccess due to it becoming too bloated.

@benprime
Copy link
Contributor Author

benprime commented Mar 8, 2020

I have not come up against many scenarios where I'd want to ignore the result completely, but I have come up against several scenarios where I would like to run an operation, check the result as normal, but return the original result rather than the new result.

Unless I'm missing some context, using bind for this would amount to this code, correct?

return getMyResult()
    .Bind(x => {
        var result = DoWork1(x);
        if (result.IsSuccess) return Result.Success(x);
        return result;
    })
    .Bind(x => {
        var result = DoWork2(x);
        if (result.IsSuccess) return Result.Success(x);
        return result;
    })
    .Bind(x =>
    {
        var result = DoWork3(x);
        if (result.IsSuccess) return Result.Success(x);
        return result;
    });

This works, but the developers that I work with (and myself) usually end up writing extension methods like TapAndCheck that feel like they should be core to the library.

@space-alien
Copy link
Collaborator

space-alien commented Mar 8, 2020

I do it this way:

return GetCustomer()
    .Bind(customer => UpdateCustomer(customer)
        .Map(_ => customer))
    .Bind(customer => DoWork(customer)
        .Map(() => customer));

UpdateCustomer() returns a Result containing a value that we aren't interested in.
DoWork() returns a Result with no value.

@benprime
Copy link
Contributor Author

Thank you. We'll use .Bind as outlined here.

@space-alien
Copy link
Collaborator

@vkhorikov Do you think there is any case for adding a new method for this?

@vkhorikov
Copy link
Owner

Yeah, we should add this to the library. TapAndCheck (@benprime's suggestion) sounds appropriate to me.

@benprime
Copy link
Contributor Author

@vkhorikov Would you like me to create a PR?

@vkhorikov
Copy link
Owner

Please do.

@space-alien
Copy link
Collaborator

Consider whether it would be safer and more intuitive if we added Tap() overloads for Func<Result> that bind the inner result success/failure while ignoring its return value?

See also #174

@benprime
Copy link
Contributor Author

Addressed as part of PR #192

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

No branches or pull requests

3 participants