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

Match: Head/Tail vs. Empty/One/More vs Empty/Seq and variants #275

Closed
wants to merge 1 commit into from

Conversation

StefanBertels
Copy link
Contributor

I stumbled on different Match signatures. It's not very clear what get's matched.

The attached pull request is my suggestion to improve this a bit. I guess there are more occurences and I think there should be some easy / general rule for this.

What's the issue?

I wonder: when is Head matching a 1 element list, when is it matching the head of a list with 1 or more item, when is it matching the head of a list with at least 2 items?
Same for Tail.
For Tail this is even more difficult because some variants match head, too.

The last function I modified is very special because you can match:

  • empty
  • head only list (1 item)
  • tail of a list with at least 2 items

This mixes matching different list sizes (empty, one, more) or (empty, seq) with deconstructing head:tail.

I suggest to make this more clear, perhaps drop variants that are not very clear here.

@StefanBertels
Copy link
Contributor Author

StefanBertels commented Sep 27, 2017

After thinking a bit more I would suggest this general rule:

Match should just do list size pattern matching.
Parameters hould have names like empty, oneOrMore resp. empty, one, twoOrMore.
oneOrMore and twoOrMore should always get the full list (every parameter gets it).

Maybe it's ok to allow variant Func<T, Seq<T>, TResult> oneOrMore but I find it more clean to do deconstruction afterwards. It's not clear whether Func<T, Seq<T>, TResult> is more appropriate for oneOrMore (head and maybeEmptyTail) or twoOrMore (head and tailWithAtLeastOne). If you really think deconstruction should be done in 'Match' I suggest to make parameter name very explicit here or even better name this special 'Match' function different (MatchAndDecons).

BTW: Is there a separate Decons function that turns Func<Seq<T>,TResult> into Func<T, Seq<T>, TResult> (resulting in an error if seq is empty)?

Regarding twoOrMore there might be even a DeDecons function turning Func<Seq<T>,TResult> into Func<T, T, Seq<T>, TResult> (resulting in an error if seq contains 0 or 1 elements).

Copy link
Owner

@louthy louthy left a comment

Choose a reason for hiding this comment

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

The name suggestions in the changes are OK apart from the last ones. To be honest most of this is moot, because mostly I'd expect people to use list matching in the same way as with functional languages:

var r = list.Match(
    ()      => ...,
    x       => ...,
    (x, xs) => ... );

/// <returns>Result of match function invoked</returns>
public virtual B Match<B>(
Func<B> Empty,
Func<A, Seq<A>, B> Tail) =>
Func<A, Seq<A>, B> Seq) =>
Copy link
Owner

Choose a reason for hiding this comment

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

This is OK.

Func<A, B> Head,
Func<A, Seq<A>, B> Tail) =>
Func<A, B> One,
Func<A, Seq<A>, B> More) =>
Copy link
Owner

Choose a reason for hiding this comment

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

This is OK.

/// <returns>Result of match function invoked</returns>
public virtual B Match<B>(
Func<B> Empty,
Func<A, B> Head,
Func<A, B> HeadOnly,
Func<Seq<A>, B> Tail) =>
Copy link
Owner

Choose a reason for hiding this comment

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

Leave these as Head and Tail because it's exactly what's happening.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was just confused because I would intuitively expect

Match(Func<B>, Func<A, B>, Func<Seq<A>, B>)

to give me:
0 elems (= all)
1 elem (= all)
2 or more elems (= all = head included)

this would scale for any number of args and last arg would always be "seq with at least n elems" while args before are "tuple" of 0, 1, ... n-1.

Parameter name Tail is ok (but who looks on arg names to see behaviour, signature should be hint enough) -- IMHO this function could be left out. Having the last arg Func<A, Seq<A>, B> can be used instead and has clear meaning. Fits exactly what you wrote in the beginning of your answer.

Copy link
Owner

Choose a reason for hiding this comment

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

You should look at the implementation, it passes the Tail, not the Head + Tail to the Tail delegate; that's why Tail is the best name here. As I stated before, the names aren't really important for list matching. But in this case it should be Tail because it is literally receiving list.Tail as its argument.

This isn't some obscure use case, it allows the user to work with a complete tail rather than a deconstructed one, which under certain circumstances will be more memory efficient than the variant above that does deconstruct. That's why I added the variant, to help both potential use-cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I looked at implementation.

I'm not sure what kind of memory overhead you see. If the function with this signature would be this:

        /// <summary>
        /// Match empty sequence, or one item sequence, or tail of multi-item sequence
        /// </summary>
        /// <typeparam name="B">Return value type</typeparam>
        /// <param name="Empty">Match for an empty list</param>
        /// <param name="HeadOnly">Match for list with only 1 element</param>
        /// <param name="More">Matches list with at least 2 elements</param>
        /// <returns>Result of match function invoked</returns>
        public virtual B Match<B>(
            Func<B> Empty,
            Func<A, B> HeadOnly,
            Func<Seq<A>, B> More) =>
            IsEmpty
                ? Empty()
                : this.Tail.IsEmpty
                    ? HeadOnly(this.Head)
                    : More(this);

This would match on list size without dropping head or another element in any case as every other Match variant does (yes, this is just one point of view).

If the caller is only interested in Tail he could just call Tail like this:

var resultAsString = Seq(1, 2, 3).Match(
    () => "empty", 
    one => $"only: {one}", 
    more=> more.Tail.Apply(xs => "tail: " + string.Join(",", xs)));

I don't see why it matters in regard of memory or performance.

Anyway this is all just my humble opinion. My primary goal is to understand design decision to avoid bugs due to misunderstanding and this was something I stumbled on.

You probably don't want to change semantics for this function signature for compatibility reasons, anyway. That's why "drop it" came to my mind.

I'm happy with current state with cleaned up parameter names.

@StefanBertels StefanBertels mentioned this pull request Oct 12, 2017
@StefanBertels
Copy link
Contributor Author

I will now close this issue because

  • this isn't a bug and
  • I became accustomed to it
  • I'm sensitized for different variants
  • I try to avoid the IMHO potentially misleading variant where the third parameter is xs => ... (missing head) and use only the signature with (x,xs) => ...

I still would favor if the IMHO potentially misleading variant would be more clearly marked (obsolete, editorbrowsable(never)...) but doesnt' really bug me anymore.

@StefanBertels
Copy link
Contributor Author

StefanBertels commented Oct 12, 2018

Still a question (just for understanding how things could/should be):

Shouldn't this

var r = list.Match( () => ..., x => ..., (x, xs) => ... );

better be like this

var r = list.Match( () => ..., x => ..., (x1, x2, xs) => ... );

because the more case always has at least two elements and xs as a type (list/sequence) in general could be empty. (Of course that means you would need x1.Cons(x2.Cons(xs)) to get back the whole sequence).

@TysonMN
Copy link
Contributor

TysonMN commented Oct 13, 2018

That makes sense to me

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.

None yet

3 participants