1
\$\begingroup\$

[This is a repost with entirely new wording and some minor changes in the code, not a duplicate of my previous question which has been deleted.]

Using Linq2DB I have a record class similar the following (cut down for example only):

[Table(Name = "Contacts")]
public class Contact
{
    [PrimaryKey, Identity]
    public int ID { get; set; }
    [Column, NotNull] 
    public string Name { get; set }
    [Column, Nullable]
    public string Email { get; set; }
    [Column, Nullable]
    public DateTime? LastContact { get; set; }
}

I have a model class that wraps an instance of the above and provides write access to only the fields that can be changed, and tracks changes as they are written. The model class for the above is:

public class ContactModel
{
    private static TestDB DB => Databases.GetDB<TestDB>();

    private Contact _contact;

    public int ID => _contact?.ID ?? -1;

    private bool _nameChanged = false;
    private bool _emailChanged = false;
    private bool _lastcontactChanged = false;

    private string _nameCurrent = null;
    private string _emailCurrent = null;
    private DateTime? _lastcontactCurrent = null;

    public string Name
    {
        get => _nameChanged ? _nameCurrent : _contact?.Name;
        set
        {
            _nameChanged = string.Compare(value, _contact?.Name) != 0;
            _nameCurrent = _nameChanged ? value : null;
        }
    }

    public string Email
    {
        get => _emailChanged ? _emailCurrent : _contact?.Email;
        set
        {
            _emailChanged = string.Compare(value, _contact?.Email) != 0;
            _emailCurrent = _emailChanged ? value : null;
        }
    }

    public DateTime? LastContact
    {
        get => _lastcontactChanged ? _lastcontactCurrent : _contact?.LastContact;
        set
        {
            _lastcontactChanged = value != _contact?.LastContact;
            _lastcontactCurrent = _lastcontactChanged ? value : null;
        }
    }

    private ContactModel(Contact contact)
    {
        _contact = contact;
    }

    [IgnoreDataMember]
    public bool Changed => _nameChanged || _emailChanged || _lastcontactChanged;

    public void Reset()
    {
        _nameChanged = _emailChanged = _lastcontactChanged = false;
        _nameCurrent = null;
        _emailCurrent = null;
        _lastcontactCurrent = null;
    }

    public bool Update()
    {
        if (!Changed)
            return true;

        try
        {
            var db = DB;
            int id = ID;
            if (_contact == null)
            {
                id = db.Contacts.InsertWithInt32Identity(() =>
                        new Contact
                        {
                            Name = Name,
                            Email = Email,
                            LastContact = LastContact
                        }
                    );
            }
            else
            {
                var upd = db.Contacts.Where(c => c.ID == id).AsUpdatable();

                if (_nameChanged)
                    upd = upd.Set(_ => _.Name, Name);
                if (_emailChanged)
                    upd = upd.Set(_ => _.Email, Email);
                if (_lastcontactChanged)
                    upd = upd.Set(_ => _.LastContact, LastContact);

                if (upd.Update() < 1)
                    return false;
            }
            _contact = db.Contacts.Single(c => c.ID == id);
            Reset();
            return true;
        }
        catch
        { }
        return false;
    }

    public static ContactModel LoadContact(int id, bool create = true)
    {
        var contact = DB.Contacts.FirstOrDefault(c => c.ID == id);
        return (!create && contact != null) ? null : new ContactModel(contact);
    }

    public static ContactModel NewContact()
    {
        return new ContactModel(null);
    }

    public static ContactModel LoadContactByName(string name, bool create = false)
    {
        var contact = DB.Contacts.FirstOrDefault(c => c.Name == name);
        if (contact == null)
        {
            if (!create)
                return null;
            contact = new Contact { Name = name };
        }
        return new ContactModel(contact);
    }
}

Initially I was writing essentially the same code multiple times to manage the changes, and of course my records tend to have many more fields than the above. After missing a field in the Changed property in one of my models caused me to pull out some of my hair trying to figure out why it wouldn't post updates to the database I decided to simplify things.

Since I have uses for this in a variety of places I've split the base class into two parts:

  • Editable<TValue> - tracks changes, usable in several places
  • RecordEditable<TRecord, TValue> - can apply changes via the ORM I'm using, only works with that ORM.

The Editable<> base class has the following basic properties:

  • Accesses the source value via a Func<TValue> so I don't have to reinitialize when the record is reloaded.
  • Tracks changes on assignment.
  • Can be reset to

This is my current implementation:

/// <summary>Interface for object that can be edited</summary>
public interface IEditable : IDisposable
{
    /// <summary>Display name of the changeable object</summary>
    string Name { get; }
    /// <summary>True if current and source values differ</summary>
    bool Changed { get; }
    /// <summary>Reset to source value</summary>
    void Reset();
}

/// <summary>Interface for editable values of a specific type.</summary>
/// <typeparam name="TValue">Type of contained value</typeparam>
public interface IEditable<TValue> : IEditable
{
    /// <summary>Source value before changes</summary>
    TValue SourceValue { get; }
    /// <summary>Current value: source or new value if changed</summary>
    TValue Value { get; set; }
}

/// <summary>Implement change tracking for a value</summary>
/// <typeparam name="TValue">Type of value to manage</typeparam>
public class Editable<TValue> : IEditable<TValue>
{
    // Always uses the default comparer for the value type
    protected static Comparer<TValue> _comparer = Comparer<TValue>.Default;

    // Function that fetches the source value
    private Func<TValue> _getSource;

    private string _name = null;
    /// <summary>Display name of editable object</summary>
    public virtual string Name
    {
        get => _name;
        set => _name = value;
    }

    private bool _changed = false;
    /// <summary>True if assigned a value different to source</summary>
    public bool Changed => _changed;

    /// <summary>Source value</summary>
    public TValue SourceValue => _getSource == null ? default : _getSource();

    private TValue _value = default;
    /// <summary>Current value</summary>
    public TValue Value
    {
        get => _changed ? _value : SourceValue;
        set
        {
            _changed = _comparer.Compare(value, SourceValue) != 0;
            _value = _changed ? value : default;
        }
    }

    /// <summary>Constructor</summary>
    /// <param name="getSource">Function to return the source value</param>
    public Editable(Func<TValue> getSource)
    {
        _getSource = getSource;
    }

    /// <summary>IDisposable implementation</summary>
    public void Dispose()
        => Dispose(true);

    /// <summary>Release all references, etc</summary>
    /// <param name="disposing">True if disposing, false if finalizing</param>
    protected virtual void Dispose(bool disposing)
    {
        if (disposing)
        {
            _getSource = null;
            _changed = false;
            _value = default;
        }
    }

    /// <summary>Clear any changes and reset to source value</summary>
    public void Reset()
    {
        _changed = false;
        _value = default;
    }
}

And this is a sample model class that uses the Editable type:

public class ContactModel : IDisposable
{
    private static TestDB DB => Databases.GetDB<TestDB>();

    private Contact _contact;

    public int ID => _contact?.ID ?? -1;

    private bool _nameChanged = false;
    private bool _emailChanged = false;
    private bool _lastcontactChanged = false;

    private string _nameCurrent = null;
    private string _emailCurrent = null;
    private DateTime? _lastcontactCurrent = null;

    public string Name
    {
        get => _nameChanged ? _nameCurrent : _contact?.Name;
        set
        {
            _nameChanged = string.Compare(value, _contact?.Name) != 0;
            _nameCurrent = _nameChanged ? value : null;
        }
    }

    public string Email
    {
        get => _emailChanged ? _emailCurrent : _contact?.Email;
        set
        {
            _emailChanged = string.Compare(value, _contact?.Email) != 0;
            _emailCurrent = _emailChanged ? value : null;
        }
    }

    public DateTime? LastContact
    {
        get => _lastcontactChanged ? _lastcontactCurrent : _contact?.LastContact;
        set
        {
            _lastcontactChanged = value != _contact?.LastContact;
            _lastcontactCurrent = _lastcontactChanged ? value : null;
        }
    }

    private ContactModel(Contact contact)
    {
        _contact = contact;
    }

    public void Dispose()
        => Dispose(true);

    protected virtual void Dispose(bool disposing)
    {
        _contact = null;
        Reset();
    }

    [IgnoreDataMember]
    public bool Changed => _nameChanged || _emailChanged || _lastcontactChanged;

    public void Reset()
    {
        _nameChanged = _emailChanged = _lastcontactChanged = false;
        _nameCurrent = null;
        _emailCurrent = null;
        _lastcontactCurrent = null;
    }

    public bool Update()
    {
        if (!Changed)
            return true;

        try
        {
            var db = DB;
            int id = ID;
            if (_contact == null)
            {
                id = db.Contacts.InsertWithInt32Identity(() =>
                        new Contact
                        {
                            Name = Name,
                            Email = Email,
                            LastContact = LastContact
                        }
                    );
            }
            else
            {
                var upd = db.Contacts.Where(c => c.ID == id).AsUpdatable();

                if (_nameChanged)
                    upd = upd.Set(_ => _.Name, Name);
                if (_emailChanged)
                    upd = upd.Set(_ => _.Email, Email);
                if (_lastcontactChanged)
                    upd = upd.Set(_ => _.LastContact, LastContact);

                if (upd.Update() < 1)
                    return false;
            }
            _contact = db.Contacts.Single(c => c.ID == id);
            Reset();
            return true;
        }
        catch
        { }
        return false;
    }

    public static ContactModel LoadContact(int id, bool create = true)
    {
        var contact = DB.Contacts.FirstOrDefault(c => c.ID == id);
        return (!create && contact != null) ? null : new ContactModel(contact);
    }

    public static ContactModel NewContact()
    {
        return new ContactModel(null);
    }

    public static ContactModel LoadContactByName(string name, bool create = false)
    {
        var contact = DB.Contacts.FirstOrDefault(c => c.Name == name);
        if (contact == null)
        {
            if (!create)
                return null;
            contact = new Contact { Name = name };
        }
        return new ContactModel(contact);
    }
}

The Update method still requires writing the tests out in full, which is where the RecordUpdatable<> type comes in.

The IUpdatable.Set extension method uses LINQ Expressions to specify the field that is being updated. With a bit of expression manipulation I can create the getSource function from the field selector expression, with appropriate null checks and such.

Expression modification uses the following expression visitor:

public class ReplaceVisitor : ExpressionVisitor
{
    private Expression _from, _to;

    public ReplaceVisitor(Expression from, Expression to)
    {
        _from = from;
        _to = to;
    }

    public override Expression Visit(Expression node)
        => node == _from ? _to : base.Visit(node);

    public static T Transform<T>(T target, Expression from, Expression to)
        where T : Expression
        => (T)(new ReplaceVisitor(from, to).Visit(target));

    // returns expression in form: () => record != null ? record.field : default(TValue);
    public static Expression<Func<TValue>> SelectorToRecordGuarded<TRecord, TValue>(Expression<Func<TRecord, TValue>> fieldSelector, Expression<Func<TRecord>> fetchRecord)
        => Expression.Lambda<Func<TValue>>(
                Expression.Condition(
                    Expression.NotEqual(Expression.Constant(null), fetchRecord.Body),
                    Transform(fieldSelector.Body, fieldSelector.Parameters[0], fetchRecord.Body),
                    Expression.Default(typeof(TValue))
                )
            );
}

The interface and code below implement the database row update:

/// <summary>Interface for objects that can apply their changes to a compatible record</summary>
/// <typeparam name="TRecord">Type of record this applies to</typeparam>
public interface IApplyable<TRecord> : IEditable
{
    /// <summary>Applies any change to the supplied <see cref="IUpdatable{T}"/>.</summary>
    /// <param name="updatable">Linq2DB <see cref="IUpdatable{T}"/> instance of appropriate record type</param>
    /// <returns>New <see cref="IUpdatable{T}"/> instance with change applied, or <paramref name="updatable"/> if no change.</returns>
    IUpdatable<TRecord> Apply(IUpdatable<TRecord> updatable);
    /// <summary>A lambda expression that selects the field in the record, in the form: <code>r =&gt; r.field;</code></summary>
    LambdaExpression FieldSelector { get; }
}

/// <summary>Implement change tracking for a field of a specific record type</summary>
/// <typeparam name="TRecord">Type of record</typeparam>
/// <typeparam name="TValue">Type of field</typeparam>
public class RecordEditable<TRecord, TValue> : Editable<TValue>, IApplyable<TRecord>
{
    // Expression that selects a field from the record
    // In the form: rec => rec.field;
    private Expression<Func<TRecord, TValue>> _fieldSelector;

    /// <summary>Display name, use field name if no name specified</summary>
    public override string Name
    {
        get
        {
            string res = base.Name;
            if (res == null)
            {
                // get field name from field selector expression
                if (_fieldSelector.Body is MemberExpression m)
                {
                    res = base.Name = m.Member.Name;
                }
            }
            return res;
        }
    }

    // IApplyable implementation: strips type details from the actual field selector
    LambdaExpression IApplyable<TRecord>.FieldSelector => _fieldSelector;

    /// <summary>Construct from expressions</summary>
    /// <param name="fetchRecord">Expression that returns the record containing the field</param>
    /// <param name="fieldSelector">Expression that selects the field from the record</param>
    public RecordEditable(Expression<Func<TRecord>> fetchRecord, Expression<Func<TRecord, TValue>> fieldSelector)
        : base(ReplaceVisitor.SelectorToRecordGuarded(fieldSelector, fetchRecord).Compile())
    {
        _fieldSelector = fieldSelector;
    }

    // Clear references held by the field selector expression
    protected override void Dispose(bool disposing)
    {
        if (disposing)
        {
            _fieldSelector = null;
        }
        base.Dispose(disposing);
    }

    /// <summary>If changed, apply the change to the supplied <see cref="IUpdatable{T}"/> </summary>
    /// <param name="updatable">Linq2DB <see cref="IUpdatable{T}"/> instance of appropriate record type</param>
    /// <returns>New <see cref="IUpdatable{T}"/> instance with change applied, or <paramref name="updatable"/> if no change.</returns>
    public IUpdatable<TRecord> Apply(IUpdatable<TRecord> updatable)
        => Changed ? updatable.Set(_fieldSelector, Value) : updatable;
}

And the new model for the Contact class:

public class ContactModel2 : IDisposable
{
    private static TestDB DB => Databases.GetDB<TestDB>();

    private Contact _contact;

    private IEditable<string> _name;
    private IEditable<string> _email;
    private IEditable<DateTime?> _lastcontact;

    public int ID => _contact?.ID ?? -1;
    public string Name { get => _name.Value; set => _name.Value = value; }
    public string Email { get => _email.Value; set => _email.Value = value; }
    public DateTime? LastContact { get => _lastcontact.Value; set => _lastcontact.Value = value; }

    private IEditable[] _fields;
    private IEditable[] Fields
    {
        get
        {
            if (_fields == null)
                _fields = new IEditable[] { _name, _email, _lastcontact };
            return _fields;
        }
    }

    private IEditable<TValue> Editable<TValue>(Expression<Func<Contact, TValue>> selector)
        => new RecordEditable<Contact, TValue>(() => _contact, selector);

    protected ContactModel2(Contact contact)
    {
        _contact = contact;
        _name = Editable(_ => _.Name);
        _email = Editable(_ => _.Email);
        _lastcontact = Editable(_ => _.LastContact);
    }

    public void Dispose()
        => Dispose(true);

    protected virtual void Dispose(bool disposing)
    {
        if (disposing)
        {
            foreach (var field in Fields)
                field.Dispose();
            _contact = null;
            _fields = null;
            _name = null;
            _email = null;
            _lastcontact = null;
        }
    }

    [IgnoreDataMember]
    public bool Changed => Fields.Any(f => f.Changed);

    public void Reset()
    {
        foreach (var f in Fields)
            f.Reset();
    }

    public bool Update()
    {
        if (!Changed)
            return true;

        try
        {
            var db = DB;
            int id = ID;
            if (_contact == null)
            {
                id = db.Contacts.InsertWithInt32Identity(() =>
                        new Contact
                        {
                            Name = Name,
                            Email = Email,
                            LastContact = LastContact
                        }
                    );
            }
            else
            {
                var upd = db.Contacts.Where(c => c.ID == ID).AsUpdatable();

                foreach (var applyable in Fields.OfType<IApplyable<Contact>>())
                    upd = applyable.Apply(upd);

                if (upd.Update() < 1)
                    return false;
            }
            _contact = db.Contacts.Single(c => c.ID == id);
            Reset();
            return true;
        }
        catch { }
        return false;
    }

    public static ContactModel2 LoadContact(int id, bool create = true)
    {
        var contact = DB.Contacts.FirstOrDefault(c => c.ID == id);
        return (!create && contact != null) ? null : new ContactModel2(contact);
    }

    public static ContactModel2 NewContact()
    {
        return new ContactModel2(null);
    }

    public static ContactModel2 LoadContactByName(string name, bool create = false)
    {
        var contact = DB.Contacts.FirstOrDefault(c => c.Name == name);
        if (contact == null)
        {
            if (!create)
                return null;
            contact = new Contact { Name = name };
        }
        return new ContactModel2(contact);
    }
}

The above works, as far as it goes. When I have a lot of fields there's still a bit of slack, but I can move the bulk of the model code into a base class to further ease the repetition and reduce the likelihood of missing a field. I'd like to get this part improved a little before I focus on that, because there's a lot more expression juggling to get that working just right.

Question is, what can I do to improve on this? I'm not convinced it's the best code I can write for the problem, but I do feel like it's an improvement over the typing, and I'd rather not just write a code generator.

\$\endgroup\$
7
  • \$\begingroup\$ I'd rather not just write a code generator. - Is there any particular reason? It's super easy. I write them actually quite often. here's one that generates an NLog.config from sql-tables. Generating your classes from a database or other source in your case would be a piece of cake - I guess ;-) It would save you hours or days of writing that code and maintaining it. \$\endgroup\$
    – t3chb0t
    Oct 30, 2017 at 15:05
  • \$\begingroup\$ The Try/Catch block with an empty catch { } can mask any type of exception which probably isn't intended. Be explicit with what (and why) you you intend on catching, and narrow the scope of the try/catch block to just that. \$\endgroup\$
    – Brad M
    Oct 30, 2017 at 20:41
  • \$\begingroup\$ Instead of tracking which fields have been changed, would it not be considerably easier to simply keep a copy of the original entity and check for changed values at the end? I feel like a lot of the code you have is dealing with change tracking and making sure your system is consistent. If you instead use a system where the tracking can be calculated at the time of saving an update, then your earlier logic should be less heavy. Not posting this as an answer yet, because I first wanted to ask if you maybe excluded this alternative because of a particular requirement on your part? \$\endgroup\$
    – Flater
    Nov 3, 2017 at 10:48
  • \$\begingroup\$ @Flater I actually have a copy of the record in the model (see the code). I needed some way to automatically map between the model fields and the record fields to do comparisons. It made sense to have the mapping also handle the change tracking. \$\endgroup\$
    – Corey
    Nov 5, 2017 at 22:37
  • \$\begingroup\$ @Corey: I meant to ask: why not compare two record fields, the original and the (possibly) updated one? You'll still need to map your original record to your model, and your model to your (possibly) updated record, but you no longer need to track whether or nkt a value has been set, because you can simply compare afterwards. The boolean change tracking system is complicating your codebase. \$\endgroup\$
    – Flater
    Nov 6, 2017 at 7:29

1 Answer 1

2
\$\begingroup\$

Using the Editable<T> for wrapping the repetitive code was a good idea. I would however change a few things.

  • I'd named it ChangeTracker as it better describes its purpose.
  • I'd named the Changed property to IsDirty. Changed sounds more like an event.
  • I'd named the _getSource field _getSourceValue.
  • Since you are interested in equality and not sorting, you should use the EqualityComparer<TValue>.Default instead.
  • This type does not have to be disposable. There are no resources to be freed and just setting all properties to some values on dispose is meaningless.
private IEditable[] _fields;
private IEditable[] Fields

Having both a private field and a property is unnecessary.


ContactModel2 has become much smaller but it's still to large. The entire database code shouldn't be there. It belongs to a different layer.


RecordEditable - Same as above, it should be a different abstraction layer - not a part of the model and it also does not have to be disposable because it does not free any resources.


You might consider making the properties of the Contact virtual and override them in a new derived type like class ContactWithChangeTracking : Contact. This way you could still use the same old type as before but with additional abilities.

\$\endgroup\$
13
  • \$\begingroup\$ Thanks for the feedback t3chb0t. Implementing IDisposable was a choice I made because the Func and Expression classes hold references that can end up stopping the GC from cleaning up the objects. If I dispose correctly that should help reduce long-term memory consumption on my web server. Anywhere I store an Expression or Func I tend to also implement IDisposable for this reason. \$\endgroup\$
    – Corey
    Oct 30, 2017 at 23:56
  • \$\begingroup\$ Regarding the Fields property... it's kind of an RAII thing, where I don't want to instantiate the array before it is referenced. When I put all that stuff in a base Model class the fields can't be enumerated in the constructor, so I'm partially preparing for that. I can use reflection to get the full list of IEditable (or IChangeTracker) objects to pop in there, but only after the instance is fully constructed. RAII at the base class lets me handle that without having to manually initialize the list. \$\endgroup\$
    – Corey
    Oct 31, 2017 at 0:01
  • \$\begingroup\$ Can you explain a bit more what you mean about the database code being in a different layer? Do you mean that I should have a table or database class that I ask for the model instances? I can do that, but I was looking at this more as a high-level abstraction away from the data so that my MVC controller never knows anything about the underlying database, only the models themselves. \$\endgroup\$
    – Corey
    Oct 31, 2017 at 0:05
  • \$\begingroup\$ @Corey Regarding your answers I have a question too. Do you have any links to articles about Expressions and/or Funcs and the GC? I've never heard of it and for me it's hard to believe that it's necessary to make your life so hard. How many MB of memory will this save? I doubt it's worth the effort and I bet there are dozen of things that are worse than that. \$\endgroup\$
    – t3chb0t
    Oct 31, 2017 at 5:22
  • \$\begingroup\$ @Corey as far as RAII is concerned I prefer the Lazy<T> class which makes the code a little bit cleaner. \$\endgroup\$
    – t3chb0t
    Oct 31, 2017 at 5:24

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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