CSharpFunctionalExtensions icon indicating copy to clipboard operation
CSharpFunctionalExtensions copied to clipboard

Check/CheckIf on UnitResult?

Open mnissl opened this issue 3 years ago • 7 comments

Check() and CheckIf() are supposed to check the returned value of a Result<T>.

UnitResult<E> does not return a value, but there are overloads for it:

public static UnitResult<E> Check<E>(this UnitResult<E> result, Func<UnitResult<E>> func)
{
  return result.Bind(func).Map(() => result);
}

public static UnitResult<E> CheckIf<E>(this UnitResult<E> result, Func<bool> predicate, Func<UnitResult<E>> func)
{
  if (result.IsSuccess && predicate())
    return result.Check(func);
  else
    return result;
}

IMHO, these overloads are superfluous and wrong as they do not check a value.

mnissl avatar May 05 '22 09:05 mnissl

Why do they not? The Check one looks into the UnitResult and if it's a success, the method runs func and returns the resulting result if that result is failure, otherwise, it returns the original successful result.

vkhorikov avatar May 05 '22 10:05 vkhorikov

Either the intention of Check() and CheckIf() is to check the returned value (Result and UnitResult don't feature a value) ... or the overload for Result is missing.

mnissl avatar May 05 '22 11:05 mnissl

I see what you mean. Not sure what the use cases here are, haven't personally used Check or CheckIf methods.

@ruudhe if you see can this, maybe you could chime in?

vkhorikov avatar May 09 '22 11:05 vkhorikov

Our use case is that we start with a UnitResult<E> and need to Check a Result<T, E>, but we do not want to bind to this Result<T, E>

ruudhe avatar May 13 '22 10:05 ruudhe

Maybe the overload should be changed to make this more explicit:

public static UnitResult<E> Check<T, E>(this UnitResult<E> result, Func<Result<T, E>> func)
{
    var check = result.Bind(func);

    if (check.IsFailure)
        return check.Error;

    return result;
}

And for UnitResult<E> to UnitResult<E> you can use Bind

ruudhe avatar May 13 '22 10:05 ruudhe

@ruudhe Could you please share a code snippet from your project where you are currently using this overload? Want to understand the use case better.

vkhorikov avatar May 15 '22 13:05 vkhorikov

This seems like a strange naming convention, I don't know why Execute or ExecuteNoValue aren't available.

Instead of having Check and CheckIf as (2) new methods, imo - it is perhaps better to have:

  • Execute (on success) == Extension for Maybe<T> and UnitResult<T>
  • ExecuteNoValue (no value) == Extension for Maybe<T>
  • ExecuteOnFailed == Extension for Maybe<T> and UnitResult<T>

This library is great, but it has alot of strange nuances with overloads or predicates that are expected to be returning Task in some cases, or in other cases UnitResult, or even in other cases Maybe<T>`, the more I work with it, the more I get confused with what should be returned and what should not be

jeffward01 avatar Dec 09 '22 16:12 jeffward01