5
\$\begingroup\$

I encountered a use case where it was needed to store each element of a tuple into a list of their respective type of the tuple element. To streamline this feature I wrote a method extension that I overloaded for different sized tuples. The code is quite basic and does not do a lot but I thought you could give me some feedback on how to improve it and/or how to do it in a more elegant way

using System;
using System.Collections.Generic;

public static class UserSpecificTupleExtension
{
    /// <summary>
    /// Deconstructs a tuple and stores each indivdual value into a provided non-null List
    /// </summary>
    /// <typeparam name="T1">type of tuple element</typeparam>
    /// <param name="value">Instance which calls this method</param>
    /// <param name="List1">instance of a List which holds type T1 </param>
    public static void Deconstruct<T1>(this Tuple<T1> value, ref List<T1> List1)
    {
        var a = value.Item1;
        List1.Add(a);
    }
    /// <summary>
    /// Deconstructs a tuple and stores each indivdual value into a provided non-null List
    /// </summary>
    /// <typeparam name="T1">type of tuple element</typeparam>
    /// <param name="value">Instance which calls this method</param>
    /// <param name="List1">instance of a List which holds type T1</param>
    public static void Deconstruct<T1>(this ValueTuple<T1> value, ref List<T1> List1)
    {
        var a = value.Item1;
        List1.Add(a);
    }

    /// <summary>
    /// Deconstructs a tuple and stores each indivdual value into a provided non-null List
    /// </summary>
    /// <typeparam name="T1">type of tuple element</typeparam>
    /// <typeparam name="T2">type of tuple element</typeparam>
    /// <param name="value">Instance which calls this method</param>
    /// <param name="List1">instance of a List which holds type T1</param>
    /// <param name="List2">instance of a List which holds type T2</param>
    public static void Deconstruct<T1, T2>(this Tuple<T1, T2> value, ref List<T1> List1, ref List<T2> List2)
    {
        var (a, b) = value;
        List1.Add(a);
        List2.Add(b);
    }

    /// <summary>
    /// Deconstructs a tuple and stores each indivdual value into a provided non-null List
    /// </summary>
    /// <typeparam name="T1">type of tuple element</typeparam>
    /// <typeparam name="T2">type of tuple element</typeparam>
    /// <param name="value">Instance which calls this method</param>
    /// <param name="List1">instance of a List which holds type T1</param>
    /// <param name="List2">instance of a List which holds type T2</param>
    public static void Deconstruct<T1, T2>(this ValueTuple<T1, T2> value, ref List<T1> List1, ref List<T2> List2)
    {
        var (a, b) = value;
        List1.Add(a);
        List2.Add(b);
    }

    /// <summary>
    /// Deconstructs a tuple and stores each indivdual value into a provided non-null List
    /// </summary>
    /// <typeparam name="T1">type of tuple element</typeparam>
    /// <typeparam name="T2">type of tuple element</typeparam>
    /// <typeparam name="T3">type of tuple element</typeparam>
    /// <param name="value">Instance which calls this method</param>
    /// <param name="List1">instance of a List which holds type T1</param>
    /// <param name="List2">instance of a List which holds type T2</param>
    /// <param name="List3">instance of a List which holds type T3</param>
    public static void Deconstruct<T1, T2, T3>(this Tuple<T1, T2, T3> value, ref List<T1> List1, ref List<T2> List2, ref List<T3> List3)
    {
        var (a, b, c) = value;
        List1.Add(a);
        List2.Add(b);
        List3.Add(c);
    }

    /// <summary>
    /// Deconstructs a tuple and stores each indivdual value into a provided non-null List
    /// </summary>
    /// <typeparam name="T1">type of tuple element</typeparam>
    /// <typeparam name="T2">type of tuple element</typeparam>
    /// <typeparam name="T3">type of tuple element</typeparam>
    /// <param name="value">Instance which calls this method</param>
    /// <param name="List1">instance of a List which holds type T1</param>
    /// <param name="List2">instance of a List which holds type T2</param>
    /// <param name="List3">instance of a List which holds type T3</param>
    public static void Deconstruct<T1, T2, T3>(this ValueTuple<T1, T2, T3> value, ref List<T1> List1, ref List<T2> List2, ref List<T3> List3)
    {
        var (a, b, c) = value;
        List1.Add(a);
        List2.Add(b);
        List3.Add(c);
    }
}

And here a small sample program:

public class Program
{

    public static void Main()
    {

        var listA = new List<int>();
        var listB = new List<string>();

        var listC = new List<char>();

        (1, "4", '_').Deconstruct(ref listA, ref listB, ref listC);
        listA.ForEach(Console.WriteLine);
        listB.ForEach(Console.WriteLine);

        Console.ReadKey();
    }
}

```
\$\endgroup\$
  • 1
    \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. Feel free to post a follow-up question if the code has changed significantly enough. \$\endgroup\$ – Mast Dec 6 '19 at 22:27
  • \$\begingroup\$ @Mast i will keep that in mind for the future \$\endgroup\$ – ExOfDe Dec 6 '19 at 22:31
7
\$\begingroup\$

ref parameters

Make sure you fully understand what ref does and when it's appropriate to use it.

ref gives you an alias to the variable passed into the method, so only use ref when you need to modify the original variable passed by the caller.

List<T> is already a reference type: anything passed to a List<T> parameter will already be a reference to an object of that type. You can happily modify the object via that reference without needing ref.

Parameter types

It's a good rule of thumb to accept the least specific type possible as a parameter.

Because you are only calling Add on the given lists, you don't actually require a concrete List<T> type to be passed. All lists that you'll ever be interested in implement IList<T> which has an Add method, so you should accept IList<T> instead.

You could go even further: IList<T>.Add is inherited from ICollection<T> so if you don't care about order then go ahead and loosen those parameter types to ICollection<T>.

Method header

Good on you for documenting your public methods! It's a great practice to get into; keep on doing it.

I do a have a few suggestions:

/// <summary>
/// Deconstructs a tuple and stores each indivdual value into a provided non-null List
/// </summary>
/// <typeparam name="T1">type of tuple element</typeparam>
/// <param name="value">Instance which calls this method</param>
/// <param name="List1">instance of a List which holds type T1 </param>

You (rightly) specify in the summary that the list must be non-null, but you don't actually check for null anywhere: your code will crash if null is passed.

Common convention in C# is to perform argument validation at the start of the method and throw an appropriate ArgumentException (or one of its child types):

public static void Deconstruct<T1>(this Tuple<T1> value, List<T1> List1)
{
    if (List1 is null) throw new ArgumentNullException(nameof(List1));
    // ...
}

You could claim, "What's the difference? It'll just crash anyway with an ArgumentNullException instead!"

The difference is:

  1. The exception is more clear to the user that they got the argument wrong, and
  2. By performing validation at the start you fail fast, rather than at some point in the middle. Imagine what happens in your Deconstruct<T1, T2> if the first list is non-null but the second one is null -- you'll have modified only one of the given lists! Who gets to clean up the state of your now half-modified input arguments?

While we're at it, change that parameter name from List1 to list. C# convention dictates camelCase for parameter names, and the 1 is redundant for this overload.

Deconstruction

public static void Deconstruct<T1, T2>(this ValueTuple<T1, T2> value, ref List<T1> List1, ref List<T2> List2)
{
    var (a, b) = value;
    List1.Add(a);
    List2.Add(b);
}

You gain nothing by doing var (a, b) = value;. It just adds an unnecessary deconstruction of value. You can already access the individual elements using the ItemN fields:

public static void Deconstruct<T1, T2>(this ValueTuple<T1, T2> value, IList<T1> list1, IList<T2> list2)
{
    list1.Add(value.Item1);
    list2.Add(value.Item2);
}

I might also suggest using a slightly different name from Deconstruct, since the compiler sometimes gives special meaning to methods called Deconstruct. It wouldn't actually do anything special in this case because it doesn't meet all the requirements, but it might still mislead or confuse someone who's familiar with that feature. Maybe name them DeconstructToLists or something -- it avoids that ambiguity while making it more clear what it does and what its side effect is.


Aside from that, I don't mind the repeatedness of the code. C# does not have first-class support for iterating over generic type arguments so any attempt to generalize your code will certainly result in a mess of reflection and boxing. There is such a small, finite set of generic flavours of Tuple and ValueTuple that I wouldn't invest the time in generalizing it further.

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ Thanks for this great answer. The reason for using ref ... well, I just had in mind I must change obj and the change must be visible outside of the method. I think rust starts to creep into my thought patterns XD I totally forgot about ICollection in c#. currently, I just use IEnumerable for almost everything. regarding the naming that is certainly a good point didn't know about the special meaning of that method name thanks for that information. Regarding the null check... forgot to write them down but it was intended^^ as you have seen in my comments. Again thanks for taking a look :D \$\endgroup\$ – ExOfDe Dec 6 '19 at 22:07
2
\$\begingroup\$

In addition to @Jeff's comments, you could also change your approach a bit. The reason you use List<T1> is you want to do this with multiple Tuple<T1> don't you? You might have lists of tuples in your code here and there e.g. List<Tuple<T1, T2>>. Instead of creating methods which need to accept other objects as parameters and change their state - construct and return right in your extension method.

/// <summary>
/// Takes a list of tuples and returns a tuple of lists.
/// </summary>
public static Tuple<List<T1>, List<T2>> ToListTuple<T1, T2>( this IEnumerable<Tuple<T1, T2>> tupleList )
{
    return new Tuple<List<T1>, List<T2>>(
        tupleList.Select( inputObj => inputObj.Item1 ).ToList(),
        tupleList.Select( inputObj => inputObj.Item2 ).ToList()
    );
}

It might seem an overkill at first. Writing methods as pure functions is however something I would highly recommend. Pure functions are functions which do not modify state of any object, they just return things and their results are repeatable. Also if you take this approach the operation can be easily reversed:

/// <summary>
/// Takes a tuple of lists and returns a list of tuples.
/// </summary>
public static List<Tuple<T1, T2>> ToTupleList<T1, T2>(this Tuple<IEnumerable<T1>, IEnumerable<T2>> listTuple)
{
    return listTuple.Item1.Zip(listTuple.Item2, MakeNewTuple).ToList();
}

/// <summary>
/// Returns a tuple containing two objects.
/// </summary>
private static Tuple<T1, T2> MakeNewTuple<T1, T2>(T1 t1Item, T2 t2Item)
{
    return new Tuple<T1, T2>(t1Item, t2Item);
}

To support other sizes of Tuple<...> objects you will need to write additional overloads.

|improve this answer|||||
\$\endgroup\$
  • 1
    \$\begingroup\$ Very interesting. If the given use case would contain tuples containing list this is certainly a way to go. Unfortunately in my case I need to take reference from pre-existing lists and insert the the elements accordingly because we cannot change the given api. Nonetheless I really like your approach. Thanks a lot. \$\endgroup\$ – ExOfDe Dec 10 '19 at 10:55

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.