Usage and meaning of Tap extension method

[mingazhev]

We're using CSharpFunctionalExtensions package in our projects more than a year and we appreciated the changes of many OnSuccess methods to separate methods with different meaning, such as Tap, Bind etc.

After some using our team got an intuitive understanding of when to use each of them and we wrote not totally functional, but easy-readable and clear constructions such as:

await GetAvailability(request, languageCode)
                    .Bind(ConvertCurrencies)
                    .Map(ApplyMarkups)
                    .Map(Convert)
                    .Tap(SaveResult)

When I read these lines I know that GetAvailability returns a Result<T1>, then passes it to the ConvertCurrencies, which returns result of another type (Result<T2>) and so on. When I see a usage of Tap it looks like a method that doing nothing with returning result, just doing some job with no result (or having result as a Task). And as I understand, Tap has the same meaning in other languages.

But during one of the code reviews we've found out that there are some Tap overloads that do change the returning result and this was totally surprising:

/// <summary>
///     If the calling result is a success, the given function is executed and its Result is checked. If this Result is a failure, it is returned. Otherwise, the calling result is returned.
/// </summary>
public static async Task<Result<T, E>> Tap<T, K, E>(this Task<Result<T, E>> resultTask, Func<T, Result<K, E>> func)
{
     Result<T, E> result = await resultTask.DefaultAwait();
     return result.Tap(func);
}

Could you tell why it was done in such way and what is the new meaning of the Tap in terms of the library?

[vkhorikov]

It doesn't change the result. The method you are referencing is an async version of the following method (the async version calls that method using result.Tap(func)) :

public static Result<T, E> Tap<T, K, E>(this Result<T, E> result, Func<T, Result<K, E>> func)
{
    return result.Bind(func).Map(_ => result.Value);
}

Here, this combination of Bind and Map works the same as

public static Result<T, E> Tap<T, K, E>(this Result<T, E> result, Func<T, Result<K, E>> func)
{
    if (result.IsSuccess)
        func(result.Value);

    return result;
}

That's because the subsequent Map discards the result of func and instead returns the initial result. The combination of Bind and Map are used here just to reuse as much code as possible.

[mingazhev]

Yes, my example is not so clear, I've created tests to explain better:

       [Fact]
        public void Succeeding_test()
        {
            var successResult = Result.Success("ok");
            Assert.Equal(successResult.Tap(s => DoSomeJob()), successResult);

            static void DoSomeJob()
            {
            }
        }

        [Fact]
        public void Failing_test()
        {
            var successResult = Result.Success("ok");
            Assert.Equal(successResult.Tap(s => Result.Failure("error")), successResult);
        }

I expect that both of these test will succeed, but the second fails, result is changed from success to failure

[vkhorikov]

Ah, I see what you mean. There should be equality members (including IEquatable implementation) in Result, similar to how it's done for Maybe.

[hankovich]

Let me add my 5 cents here.
I think that the mentioned overload (successResult.Tap(s => Result.Failure("error"))) is handy and extremely powerful. It's a very nice filter for your results.

We use it rather wide like that:

var result = _service.Get(id).ToResult(Error.NotFound) // Result<T>
    .Tap(t => ValidatePermissions(user, t))  // ValidatePermissions: User -> T -> Result<Unit, Error>
    .Tap(ValidateSomethingElse)  // ValidateSomethingElse: T -> Result<Unit, Error>
    .Tap(ThrottleRequests);  // ThrottleRequests: T -> Result<Unit, Error>

result // Result<T, Error>

Where both Unit and Error are our own custom types.

[mingazhev]

@vkhorikov sorry, may be I was not clear enough again. I mean such situation:

        [Fact]
        public void Succeeding_test()
        {
            var successResult = Result.Success("ok");
            Assert.True(successResult.Tap(s => DoSomeJob()).IsSuccess);

            static void DoSomeJob()
            {
            }
        }

        [Fact]
        public void Failing_test()
        {
            var successResult = Result.Success("ok");
            Assert.True(successResult.Tap(s => DoSomeJob()).IsSuccess);


            static Result DoSomeJob() => Result.Failure("error");
        }

There are two methods, one returns void and the second returns result.
For example we have the following high-level code:

        public Result SomeMethod()
        {
            return ExecuteSomeAction()
                .Tap(DoSomeJob);
        }
        
        static Result<string> ExecuteSomeAction() => Result.Success("Ok");

there are two variants of DoSomeJob method to make code compile:

// 1
static Result DoSomeJob(string result) => Result.Failure("error");
// 2
static void DoSomeJob(string result) { }

This method

        public Result SomeMethod()
        {
            return ExecuteSomeAction()
                .Tap(DoSomeJob);
        }

will look same in both situations, but in the 1-st case the result (failure or success) do depend on DoSomeJob signature and logic. In the second case result does not depend.

This looks confusing, because the Tap method has different semantics in these two overloads.

@hankovich I'm agree that this is powerful and handy, but maybe we need another name for it?

[hankovich]

@mingazhev ah I see your point, Name is confusing indeed :)

Several days ago I was discussing Taps and TapIfs with one of my colleagues and he admitted that he does not understand code with Taps exactly because of the different semantics.

[vkhorikov]

@mingazhev @hankovich looks like I didn't understand Tap either. I was under impression that it always returns the original result and now I see that the Bind().Map() sequence does change that result from Success to Failure in some scenarios.

I think Tap only makes sense for void signatures (Action and Func<Task>). We need another name for scenarios where it flips the result from Success to Failure. Any suggestions?

/cc @space-alien

[vkhorikov]

@hankovich Could you please post your implementation of Unit? This may be useful for #239

[Raibo]

@vkhorikov @mingazhev @hankovich
This implementation:
Behaves like Tap when its func has void signature.
Behaves like Tap when its func has Result signature and returns Success.
Behaves like Bind when its func has Result signature and returns Failure.

How about naming it BindIfFailure (only for funcs with Result signatures)?

[Raibo]

@hankovich
Could you please give an example of implementation of method ValidateSomethingElse?
As I understand it, since it has signature Result<Unit, Error>, then in case of successful validation, it must return Result.Success(UnitValue), probably with same value that was given as an input parameter.

If so, then why even use Tap, when Bind would work perfectly fine?
Then this complex behavior of "passing self result only if it is failure, else passing calling result" would not be required at all, keeping it simple.

[space-alien]

Originally these methods were called TapAndCheck().

Please have a look at #181 where these methods were originally added, and also the discussion around naming here: #189 (comment)

[space-alien]

How about renaming to Check() and CheckIf()? Just a thought.

[mingazhev]

Check() sounds good for me. I can make a PR if everybody agree

[hankovich]

@vkhorikov we use very straightforward one

    public sealed class Unit
    {
        public static Unit Instance { get; } = new Unit();
    }

[hankovich]

@Raibo let me add some details here

public Result<Metadata, Error> Upload(File file, Guid token)
{
    var tokenInfoResult = _authorizationManager.ValidateToken(token) // Returns Result<TokenInfo, Error> where TokenInfo contains some metainformation about the token while Error can contain ErrorType (NotFound/Forbidden/Validation/Throttling/etc) with additional details
        .Tap(_ => _fileValidator.ValidateFile(file)) // ValidateFile returns Result<Unit, Error>
        .Tap(_ => _throttleManager.TryStart(token)); // TryStart returns Result<Unit, Error>

     // do your stuff with tokenInfoResult
}

As you can see we use Tap as a filter for our results. We can not use Bind instead of Tap because we want our chain to return Result<TokenInfo, Error>, but not Result<Unit, Error>.

Another C# feature that we use frequently is local functions. So ValidateSomethingElse could easy be a local function (i.e. it could skip it's input and use local variables instead).

Does it make sense to you?

[mingazhev]

@hankovich looks useful, we use something like this:

    public sealed class VoidObject
    {
        private VoidObject()
        { }


        public static readonly VoidObject Instance = new VoidObject();
    }

but I like your naming more

[vkhorikov]

I like Check() too. I was thinking about Validate() but Check sounds better.

@mingazhev please go ahead with the PR. Please make sure the APIs are backward compatible (i.e the old methods need to be marked as obsolete; there are some examples of that in the code base).

[mingazhev]

@vkhorikov I'm ready with the changes but cannot push my branch (403)

[hankovich]

@mingazhev I think you should create a fork first, commit/push your changes and open a PR then.
This guide covers all the steps : https://github.com/MarcDiethelm/contributing/blob/master/README.md

[nike61]

@vkhorikov @hankovich @mingazhev "Check" sounds like method with no side effect. But what if I have a method that returns Result<Unit, Error> and it has side effects. For example I want to call external service or update something in database but I want to keep initial input as output for that operation. For example language-ext uses linq query language so you can create nested selection

from order in GetOrders()
from lastPrice in GetLastPrice(order)
from discount in CalculateDiscount(order, lastPrice)
select discount

[vkhorikov]

Do GetOrders() and GetLastPrice return Result or a plain value? If latter, I'd just create two explicit variables and pass them to the CalculateDiscount method:

var order = GetOrders();
var lastPrice = GetLastPrice(order);
return CalculateDiscount(order, lastPrice);

This is more readable than LINQ queries, IMO.

[nike61]

They return Result. You don't check for failure in your example. In that case you need to add at least two more rows for each check.
But my main point here is that Check method has no side effect. I want to do something like that:

var order = _repo.GetOrder(id)
   .Bind(order => DoSmthWithOrder(order))
   .Tap(order => UpdateOrder(order))
   .Check(price =>                    // <-- here I have to use "Check" instead of "Tap"
        _repo.GetDiscount(order.Items)
             .Bind(discount => CalculateDiscount(discount))
             .Tap(discount => UpdateDiscount(discount)));
return order;

[hankovich]

@nike61 It's up to you how to implement it, right? Nobody stops you from introducing side effects from any method (for example your Func<T, Task<T2>> for Map could be implemented with side effects).

Which name do you suggest?

[nike61]

I mean "Tap" or "Func" or "Bind" don't sound like I can't change anything using them. But "Check" sounds like no side effect.
I suggest keeping "Tap" name. And create separate class and functions for validation. Because there are other things to consider in validation (like collecting multiple validation errors)

[space-alien]

I think I'm with @hankovich on this one. Side effect (or not) in the external method is not within the control of this library. Check was separated from Tap because it has a slightly different behaviour (it targets methods retuning Result only and binds a failure. Tap would ignore a failure.) Having different names for different behaviours is helpful when reasoning about code that uses these methods.

[mingazhev]

I'm agree with @nike61 that in some cases Check looks confusing.
This method internally does Bind and Map, and such usage is applicable not only for "checking" something, it could be useful in plenty of different situations (where we would not like to pass the object through the chain explicitly).

Keeping method's name as Tap is worse because of different semantics. But maybe there would be more common name for such operations? Maybe it should be something starting with a Bind..?

@space-alien @hankovich @vkhorikov what do you think?

Something meaningful for such operation. Chain?

[vkhorikov]

@nike61 If you have a better name, let's discuss, we can rename Check into something else without keeping backward compatibility with Check as not too many people have upgraded yet.

@mingazhev I don't quite like Chain because it's too broad -- the sole purpose of all extension methods in the library is to chain operations together in 1 LINQ sequence.

[mingazhev]

I think all done there, no new ideas

[jeffward01]

Hey all, not sure if its still relevant, but what if we do this:

Based on the examples from @mingazhev : #242 (comment)

This would be 'Tap'
Tap would expect a return value of void

    [Fact]
        public void Succeeding_test()
        {
            var successResult = Result.Success("ok");
            Assert.True(successResult.Then(s => DoSomeJob()).IsSuccess);

            static void DoSomeJob()
            {
            }
        }

This would be any of the following names:

• TapAndGet
• TapAndReturn
• TapAndContinue
• Then
• Continue
• ContinueWIth
• ExecuteWith

I have not decided on a name that suits it best, personally, I like Then because its familiar based on the Entity Framework Then. Continue is also a good one based on the async method ContinueWith

These would expect to execute a method that does not return void

        [Fact]
        public void Failing_test()
        {
            var successResult = Result.Success("ok");
            Assert.True(successResult.Tap(s => DoSomeJob()).IsSuccess);


            static Result DoSomeJob() => Result.Failure("error");
        }

I propose to implement a method with the Then or ContinueWith keyword

I prefer Then, but the more I think about it, I think ContinueWith is more appropriate because in theory, this is the same action as the async ContinueWith

---

Disregard the above ^^

I did not realize the code was updated - I left the above text and did not erase it for context

Proposal

I propose that CheckIf is changed to ContinueWith

Reasons for change:

I see that these changes have been made:

• Tap (void) => Check
• Tap (not void) => CheckIf

Proposal

Personally - I feel as if Check and CheckIf perform (2) very different things

Check
Check performs a binary expression of True or False, while thats fine.

*CheckIf

CheckIf -- Executes a func and can return a success or failure. a func can be a series of actions, or what we call an Execution

This is not a binary expression of True Or False, its similar - but not.

It is, however its so much more than that. It can execute a series of actions. And actions result in true or false, yes, but actions are not validations. Actions are executions.

The word Check implies some sort of validation, not execution..

A Check is a binary result, not an execution

While, CheckIf is a binary result AND an execution.

---

[jeffward01]

I'm not quite sure if I understand Check, but please let me know if my understanding of Check is correct. Thanks