8
\$\begingroup\$

There is a way in Scala to selectively change a field while making a copy of the immutable object:

case class Phone(areaCode: String, number: String) // declare type
val original = Phone("832","123-4567") // instantiate 
val changed = original.copy(areaCode = "425") // get an updated copy

While in C# it looks like a real pain as usual. One need to create copy ctor and modifiers:

public class Phone
{
    Phone(Phone source)
        : this(source.AreaCode, source.Number)
    {
    }

    public Phone(string areaCode, string number)
    {
        AreaCode = areaCode;
        Number = number;
    }

    public string AreaCode { get; private set; }
    public string Number { get; private set; }

    public Phone WithAreaCode(string value) => 
        new Phone(this) { AreaCode = value };

    public Phone WithNumber(string value) => 
        new Phone(this) { Number = value };
}

We are probably supposed to charge per line…

Here is what I use to mitigate the problem. The immutable type is as easy as it could be:

public class Phone
{
    public Phone(string areaCode, string number)
    { AreaCode = areaCode; Number = number; }

    public string AreaCode { get; }
    public string Number { get; }
}

But there is an extension method to do the trick:

var original = new Phone("832", "123-4567");
var changed = original.With(p => p.AreaCode, "425");

which is defined as:

public static class Cloneable
{
    public static TSource With<TSource, TProperty>(this TSource source, Expression<Func<TSource, TProperty>> selector, TProperty value) => 
        source.With(new Binding(selector.Target(), value));

    static TSource With<TSource>(this TSource source, Binding change) =>
        (TSource)Activator.CreateInstance(typeof(TSource), source.Parameters(change));

    static object[] Parameters<TSource>(this TSource source, Binding change)
    {
        var values = source.Values().Update(change)
            .ToDictionary(b => b.Name, b => b.Value, StringComparer.OrdinalIgnoreCase);

        return typeof(TSource).GetConstructors().Single().GetParameters()
            .Select(p => values[p.Name])
            .ToArray();
    }

    static IEnumerable<Binding> Values<TSource>(this TSource source) =>
        typeof(TSource).GetProperties()
            .Select(p => new Binding(p.Name, p.GetValue(source)));

    static IEnumerable<Binding> Update(this IEnumerable<Binding> values, Binding change) =>
        values.Select(b => b.Name == change.Name ? change : b);

    static string Target<TSource, TProperty>(this Expression<Func<TSource, TProperty>> selector) =>
        selector.Body is MemberExpression member && member.Member is PropertyInfo property
            ? property.Name
            : throw new ArgumentException();   

    class Binding
    {
        public Binding(string name, object value)
        { Name = name; Value = value; }

        public string Name { get; }
        public object Value { get; }
    }
}  
\$\endgroup\$
  • \$\begingroup\$ It looks like in future you won't be needing this extension. Have you seen the new record that is to come in C# 8? E.g: public struct Pair(object First, object Second); this is quite nice :-) \$\endgroup\$ – t3chb0t May 8 '18 at 6:37
10
\$\begingroup\$

You should be able to cut the code length and improve performance, by simplifying the algorithm. Here are some of the problems I've noticed in your implementation:

  • You're currently processing all of the type's properties, this is not necessary as you only need few concrete ones. There are few other problems deriving from this one.

  • You're creating the instance through the Activator class while you're also requesting the constructor of the class, since you already have the ConstructorInfo, it doesn't make much sense to call Activator.CreateInstance. Especially since the Activator is the slowest way to create an instance of a type. You should only stick to one of them, in this case you need information that is contained in the ConstructorInfo, I guess the choice is obvious.

A simpler version (using your Target method):

public static TSource With<TSource, TProperty>(this TSource source,
    Expression<Func<TSource, TProperty>> selector, TProperty value)
{
    var constructor = typeof(TSource).GetConstructors().Single();
    var parameters = GenerateParameters(source, constructor,
        new KeyValuePair<string, object>(selector.Target(), value)).ToArray();
    return (TSource) constructor.Invoke(parameters);
}

private static IEnumerable<object> GenerateParameters<TSource>(
    TSource source, ConstructorInfo ctor,
    KeyValuePair<string, object> parameterToModify)
{
    var type = typeof(TSource);
    foreach (var parameter in ctor.GetParameters())
    {
        if (string.Equals(parameter.Name, parameterToModify.Key, StringComparison.OrdinalIgnoreCase))
        {
            yield return parameterToModify.Value;
        }
        else
        {
            yield return type.GetProperty(parameter.Name,
                BindingFlags.IgnoreCase | BindingFlags.Public | BindingFlags.Instance)?.GetValue(source);
        }
    }
}

I'm not a fan of long ternary operators, but if you prefer that you can always switch it. I don't really like KeyValuePair<,> either, but it makes sense to keep things coupled and adding a separate class seems somewhat redundant as it will be used only in 1 occasion.

This is the cleaner approach, but if you really want to squeeze a bit more performance you can avoid the .ToArray() call by creating an object[] instead of IEnumerable<object>:

public static TSource With<TSource, TProperty>(this TSource source,
    Expression<Func<TSource, TProperty>> selector, TProperty value)
{
    var constructor = typeof(TSource).GetConstructors().Single();
    return (TSource) constructor.Invoke(GenerateParameters(source, constructor,
        new KeyValuePair<string, object>(selector.Target(), value)));
}

private static object[] GenerateParameters<TSource>(
    TSource source, ConstructorInfo ctor,
    KeyValuePair<string, object> parameterToModify)
{
    var type = typeof(TSource);
    var ctorParameters = ctor.GetParameters();
    var parameters = new object[ctorParameters.Length];
    for (int i = 0; i < parameters.Length; i++)
    {
        var ctorParameter = ctorParameters[i];
        if (string.Equals(ctorParameter.Name, parameterToModify.Key, StringComparison.OrdinalIgnoreCase))
        {
            parameters[i] = parameterToModify.Value;
        }
        else
        {
            parameters[i] = type.GetProperty(ctorParameter.Name,
                BindingFlags.IgnoreCase | BindingFlags.Public | BindingFlags.Instance)?.GetValue(source);
        }
    }
    return parameters;
}

It's a bit longer, but it's not necessarily less readable, you can pick whatever suits you better.

P.S

Perhaps some caching mechanism would be useful, to improve performance of multiple sequential calls.

| improve this answer | |
\$\endgroup\$
  • 5
    \$\begingroup\$ In newer versions of C# you can use (string, object) instead of a KeyValuePair. \$\endgroup\$ – Nikita B May 7 '18 at 7:29
  • \$\begingroup\$ @NikitaB That's better, I'm still not in the habit of using those new tuples. \$\endgroup\$ – Denis May 7 '18 at 11:28
6
\$\begingroup\$

I don't like the concept that you have to chain the .With()-calls for every property you want to modify in the copy process because you create a new instance for each call.

A simple non-generic solution could be to use the System.Runtime.InteropServices.OptionalAttribute and named parameters:

public class Phone
{
  public Phone()
  {

  }

  Phone(Phone source) : this(source.AreaCode, source.Number, source.Id)
  {
  }

  public Phone(string areaCode, string number, int id)
  {
    AreaCode = areaCode;
    Number = number;
    Id = id;
  }

  public string AreaCode { get; private set; }
  public string Number { get; private set; }
  public int Id { get; private set; }

  public Phone Copy([Optional] string areaCode, [Optional] string number, [Optional] int? id)
  {
    return new Phone(areaCode ?? this.AreaCode, number ?? this.Number, id ?? this.Id);
  }

  public override string ToString()
  {
    return $"{AreaCode} - {Number} - {Id}";
  }
}

Usage

  Phone phone = new Phone("AAAA", "123-456", 1);      
  Phone copy = phone.Copy(number: "456-789", id: 2);
  Console.WriteLine(copy);

A generic approach could be to use tuple params in a way like this:

1) With simple (string, object)-name-value-pairs:

public static T Copy<T>(this T source, params (string Name, object Value)[] parameters) where T: new() // TODO support for objects without parameterless constructor (type.GetConstructors()... etc.)
{
  T copy = new T();
  Type type = typeof(T);
  // TODO: Fields?
  foreach (PropertyInfo pi in type.GetProperties())
  {
    pi.SetValue(copy, pi.GetValue(source));
  }

  foreach (var pair in parameters)
  {
    PropertyInfo pi = type.GetProperty(pair.Name);
    pi.SetValue(copy, pair.Value);
  }

  return copy;
}

Usage

  Phone phone = new Phone("AAAA", "123-456", 1);
  Phone copy = phone.Copy(("Number", "456-789"), ("AreaCode", "BBBB"), ("Id", 2));
  Console.WriteLine(copy);

2) With a member expression selector like:

public static T Copy<T>(this T source, params (Expression<Func<T, object>> NameSelector, object Value)[] parameters) where T: new() // TODO support for objects without parameterless constructor (type.GetConstructors()... etc.)
{
  Type type = typeof(T);
  T copy = new T();

  // TODO: Fields?
  foreach (PropertyInfo pi in type.GetProperties())
  {
    pi.SetValue(copy, pi.GetValue(source));
  }

  foreach (var pair in parameters)
  {
    string name = MemberNameFinder.GetMemberName(pair.NameSelector);

    if (name != null)
    {
      PropertyInfo pi = type.GetProperty(name);
      pi.SetValue(copy, pair.Value);
    }
    else
    {
      throw new ArgumentException("Invalid member name for Value", pair.Value.ToString());
    }
  }

  return copy;
}

Usage

  Phone phone = new Phone("AAAA", "123-456", 1);
  Phone copy = phone.Copy((p => p.Number, "456-789"), (p => p.AreaCode, "BBBB"), (p => p.Id, 2));
  Console.WriteLine(copy);

(The meat of the extension methods can surely be done more sophisticated than the above.)

MemberNameFinder is an ExpressionVistor with a rudimentary definition like:

  internal sealed class MemberNameFinder : ExpressionVisitor
  {
    List<string> m_namePath = new List<string>();

    private MemberNameFinder()
    {

    }

    internal static string GetMemberName(Expression expression)
    {
      MemberNameFinder provider = new MemberNameFinder();
      return provider.FindName(expression);
    }

    private string FindName(Expression expression)
    {
      Visit(expression);
      m_namePath.Reverse();
      return string.Join(".", m_namePath);
    }

    protected override Expression VisitMember(MemberExpression node)
    {
      m_namePath.Add(node.Member.Name);
      return base.VisitMember(node);
    }

    protected override Expression VisitMethodCall(MethodCallExpression node)
    {
      m_namePath.Add(node.Method.Name);
      return base.VisitMethodCall(node);
    }
  }
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Why do you need the VisitMethodCall override? It doesn't make much sense in this context since you're interested only in properties... aren't you? \$\endgroup\$ – t3chb0t May 7 '18 at 7:59
  • 1
    \$\begingroup\$ I wouldn't go with private setters and pi.SetValue(copy, pair.Value);. This kills the reaonly-character of the class. \$\endgroup\$ – t3chb0t May 7 '18 at 8:01
  • \$\begingroup\$ @t3chb0t: No, you're right. I just copied the class from another context. \$\endgroup\$ – Henrik Hansen May 7 '18 at 8:01
  • \$\begingroup\$ @t3chb0t: You may be right. I'm just suggesting a possible way to go. You can easily make some appropriate filters on type.GetProperties() with BindingFlags \$\endgroup\$ – Henrik Hansen May 7 '18 at 8:08
  • 2
    \$\begingroup\$ I think original code was made to copy an immutable class, while overriding some of the constructor parameters. So approach that assumes that class has a default constructor and/or properties with public setter is kind of missing the point. Or maybe I misunderstood the question. \$\endgroup\$ – Nikita B May 7 '18 at 11:43
3
\$\begingroup\$

I find this helper extension is not necessary at all. Having immutable types with multiple properties (>3) makes it quickly ugly to use because of the huge constructor. Up to three properties it can be quickly initialized with new values anyway.

Where Phone seems to be ok to be immutable because it looks like it was correctly addressing the primitive-obsession; above that limit I'd think twice before making a type immutable to not find oneself trapped in the immutable-obsession.

As far as your implementation is concerned I don't really like it:

  • the body-expression methods are very difficult to read; I prefer full body methods if it's not something trivial
  • .With looks like it was a builder; the Copy extension from Scala is more intuitive
  • .Single() restricts types to only one constructor; the matching logic should be more sopthisticated
  • .Update does not update anything; it shouldn't be called like that
| improve this answer | |
\$\endgroup\$
2
\$\begingroup\$

I like Denis's answer as a C# solution. However, if you could implement this part of your system in F#, you could make this really easy on yourself and still provide interoperability with your other C# projects.

The F# type system has a feature called Record Types. These are immutable by default, and support simple copy-with-update syntax. The following example demonstrates your use-case for phone numbers implemented in F#:

type Phone = {AreaCode: string; Number: string}

let original = {AreaCode = "832"; Number = "123-4567"}

let changed = {original with AreaCode = "425"}

The first line declares an immutable type called Phone with AreaCode and Number properties. The second line instantiates a Phone with the given values. The third line creates a copy of the original Phone instance with a different area code. This syntax can be used to copy any record type and re-assign any number of properties by separating them with semicolons, as in the original instantiation.

One other nice benefit you get from record types is the automatic structural-equality checking. Instead of having to override the Equals method for your Phone type and compare the contents explicitly, record types automatically give you structural equality, so if you use an = operator (or == in C#) between two instance of the Phone type, it will compare the AreaCode and the Number of the two instances for you.

| improve this answer | |
\$\endgroup\$

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.