5

I have an application that receives a number of values that need to be applied to various properties of an object (sealed class). Originally I just set the value without checking anything and updated the object, but of course sometimes the new values wouldn't be valid, and other times they would be identical to existing values, so running an update was a waste of system resources, and frankly a rather slow process.

To get around this, I've created a private bool that is set to true if/when a new value is applied, which is determined with a method that checks if the value is different (and valid):

private bool updated = false;
private bool updateValue(string property, string value, bool allowNulls = false)
{
    if ((allowNulls || !string.IsNullOrEmpty(value)) && !property.Equals(value)) { updated = true;  return true; }
    return false;
}

Then, whenever I need to set the value of a property to something new, I have a single If Statement:

if (updateValue(obj.property, newValue))
{ obj.property = newValue; }

And then of course, when all properties have been assigned their new values:

if (updated)
{ obj.commitChanges(); }

So, 1) Is there a better way of doing this, and 2) is there a more concise way to run my If statement? Single-line If clauses like this annoy me; it seems like I should be able to say "Set A to B when C".

  • Can you post more of the updateValue code here? It's missing the part that actually changes the underlying value. Also, is updated used elsewhere? What does its value mean if many properties are getting their values changed within a small time frame? – Dan1701 Jan 10 '18 at 20:18
  • 3
    Ow! Misnomers hurt! – candied_orange Jan 10 '18 at 20:24
  • 2
    I have usually seen this called "dirty" or "isDirty()". e.g. this SO question Much clearer than "update", though "needToUpdate" would be o.k. – user949300 Jan 10 '18 at 20:30
  • 1
    IsUpdateNeeded() gets my vote. – candied_orange Jan 10 '18 at 20:32
  • 1
    @RobertHarvey, I see nothing wrong with either FileExists() or File.Exists(). – Ant_222 Jan 10 '18 at 20:52
9

No.

No.

All the no!

Firstly, don't make your callers do all that work! When I want to set a value, I just want to set it! And you're working in a language that makes it easy to write setters and getters. Use them.

Here's what your callers should write:

obj.propA = valueA;
obj.propB = valueB;

And come time to persist obj, if you're doing something active record style, it's just this:

obj.Save();

Your setters need to throw exceptions if you supply a bad value.

Secondly, don't make your callers care what Save() or commit() does or doesn't do. The persistence method itself needs to check dirty flags or compare hashes or serializations to determine whether it actually does work.

Any really, whatever strategy you choose is probably totally fine. Really! Compare the lines of code, performance implications, and semantic "feeling" of each, and pick the one that feels right.

Heck, what's "right" might even vary between classes or projects.

Either way, don't make your callers care about it.

  • I would have done just this, if I could. Unfortunately, the object I'm updating is a sealed class that I cannot modify. Customizing the setters is simply not an option. – Michael Bailey Jan 11 '18 at 21:29
  • Gotcha. My instinct would be to write a wrapper and default to using serialization comparisons. Take a snapshot on instantiation and/or fetch from the database. Take another snapshot on save. You can probably do this in less than 100 lines with generics. If performance becomes a problem, add a dirty flag (or several) and build your setters as normal. – svidgen Jan 11 '18 at 23:20
  • Yeah... that seemed like more work than what I described in the OP. – Michael Bailey Jan 12 '18 at 19:30
  • Umm ... Maybe it's more work (up front). But, it'll probably save you in debugging not so far down the line. Especially if the class is used in more than one place -- at which point, you can already justify a little boilerplate to keep things "DRY." – svidgen Jan 13 '18 at 0:19
  • Well, the class that I'm writing all of this in is already the interface that should be used in other places. i.e. If you want to update the object in question, it should be through the use of the class I'm writing. It's a type of activity, with methods and properties related to the updates of the object in question. You could almost think of it as a wrapper itself, and thus the need to be more concise in how I handle updates. Creating yet another class just to handle this one aspect seems like overkill. – Michael Bailey Jan 13 '18 at 8:40
3

There are many approaches to this problem:

Third-party Libraries Use a third-party solution, such as linked in the comments. This is bound to be more robust (and testable) than diverting from your primary task to build out this functionality.

Property Get/Set methods Handle the changes within normal property methods:

private bool hasUpdates = false;

...

private string name;
public string Name
{
  get { return name; }
  set
  {
     if (!value.Equals(name) && !string.IsNullOrEmpty(value))
     {
       name = value;
       hasUpdates = true;
     }
  }
}

This requires a bit more code bulk, so might not be preferable if there are a lot of such properties, or you really like typing, or this code isn't being generated from a DSL.

Single Smart-Update Method

private bool hasUpdates = false;

...

private string name;

public bool TryUpdate(string propertyName, T value, out T newValue) {
   var classProperty = this.GetType().GetProperty(propertyName);

   if (classProperty == null) { return false; }

   if (!classProperty.GetValue(this).Equals(value)) {
     classProperty.SetValue(value);
     hasUpdates = true;
     newValue = value;
   }
}

Here you only need one bulky method, rather than one for each property.

Hash Comparison

Another solution would be to maintain a cache of objects and their hash value when retrieved from the DB, and then compare the hash again when commitChanges() is called. Only process those objects whose hashes have changed.

1

If your object were passive and had public fields instead of properties, you could write a more elegant Update() procedure, accepting the field as a ref parameter:

static void Update( ref string curVal, string newVal, ref bool isDirty )
{   if( curVal != newVal )
    {   isDirty = true;
        curVal  = newVal;
    }
}
//...
updated = false;
Update( ref obj.Prop1, "NewValue1", ref updated );
Update( ref obj.Prop2, "NewValue2", ref updated );
//... updates of many other properties...
if( updated )
{   CommitChanges( obj );  }

With the overhead of OOP, however, you have more work to do. The following console application demonstrates one possible solution for properties with trivial setters (the Trivial class) and non-trivial ones (the NonTrivial class), which make arbitrary calculations:

// TODO: Maybe store property names as constants?
using System;
using System.Collections.Generic;

class Program {

class Trivial
{   Dictionary<string, string> Props;

    public bool IsDirty // TODO: Make it writable for resetting?
    {   get; private set;  }

    public void PropSet( string prop, string value )
    {   string currentVal;

        currentVal = Props[ prop ];
        if( currentVal != value )
        {   IsDirty = true;
            Props[ prop ] = value;
        }
    }

    public string Prop1
    {   set
        {   Props["Prop1"] = value;  }      
        get
        {   return Props["Prop1"];  }
    }

    public string Prop2
    {   set
        {   Props["Prop2"] = value;  }      
        get
        {   return Props["Prop2"];  }
    }

    public Trivial()
    {   Props = new Dictionary<string, string>();
        Props.Add("Prop1", "");
        Props.Add("Prop2", "");     
    }
}

class NonTrivial
{   delegate void FPropSet( string value );
    // TODO: Modify PropInfo according to your needs, e.g. add an
    //       allowNull flag...
    class PropInfo
    {   public FPropSet Set;
        public string   Value;
    }

    Dictionary< string, PropInfo > Props;

    public bool IsDirty // TODO: Make it writable for resetting?
    {   get; private set;  }

    public string Prop1
    {   set
        {   PropSet( "Prop1", value );  }       
        get
        {   return PropGet( "Prop1" );  }
    }

    void SetProp1( string value )
    {   // Very heavy calculations!
    }

    public string Prop2
    {   set
        {   PropSet( "Prop2", value );  }       
        get
        {   return PropGet( "Prop2" );  }
    }

    void SetProp2( string value )
    {   // Super heavy calculations!
    }   

    void RegisterProp( string prop, FPropSet setter, string defValue )
    {   PropInfo info = new PropInfo();
        info.Set   = setter;
        info.Value = defValue;
        setter( defValue );
        Props.Add( prop, info );
    }

    string PropGet( string prop )
    {   return Props[ prop ].Value;  }

    public void PropSet( string prop, string value )
    {   PropInfo info;
        info = Props[ prop ];
        // TODO: Modify the comparison according to your needs:
        if( info.Value != value )
        {   info.Set( value );
            info.Value = value;
            IsDirty = true;
        }
    }

    public NonTrivial()
    {   Props = new Dictionary< string, PropInfo >();
        RegisterProp( "Prop1", SetProp1, "A" );
        RegisterProp( "Prop2", SetProp2, "1" );
    }
}

public static void Main(string[] args)
{   Trivial    triv    = new Trivial   ();
    NonTrivial nonTriv = new NonTrivial();

    triv.Prop1 = "A"; // The setter will not change .IsDirty
    triv.Prop2 = "1"; // Should it?
    triv.PropSet("Prop1", "B"); // Update property only if changed
    if( triv.IsDirty )
    {   // This work will not be wasted
    }

    nonTriv.Prop1 = "A"; // The setter will change .IsDirty
    nonTriv.Prop2 = "1"; // Should it?
    nonTriv.PropSet("Prop1", "B");  // Update property only if changed
    if( nonTriv.IsDirty )
    {   // It will not be labor lost
    }   

    Console.ReadKey(true);
}

}

If the caller is not interested in whether actual data has changed, you can unpublish the IsDirty property and use it internally inside your CommitChanges() method, which I should rename into CommitChangesIfAny() to reflect the conditional nature of the commit operation.

Yet another option is Reflection whereby you can do the same thing dynamically (Edit: see the Single Smart-Update Method in Dan1701's answer).

0

What about changing the updateValue method to return null instead of false (and maybe rename it to getNewValue), and then just set my properties using a conditional operator:

obj.property = getNewValue(obj.property, newValue) | obj.property;

The getNewValue method is still responsible for setting the 'IsDirty' bit, which can be used to confirm that the new values need to be committed.

  • Actually, I don't even need the conditional operator, since the getNewValue method could itself return the obj.property if the newValue isn't valid. I think I was overthinking the initial problem. – Michael Bailey Jan 12 '18 at 20:32
  • 1
    I think you don't need | obj.property either because getNewValue() can take care of that too, but I still prefer my method because it keeps the property-setting code cleaner. – Ant_222 Jan 12 '18 at 21:47
  • The problem is that we're dealing with properties, not fields - and they are in a sealed class, so I don't have access to any underlying fields. So I can't use ref parameters (I already tried going that route). – Michael Bailey Jan 13 '18 at 8:41
  • I thought that was your own class and you had full control over it. – Ant_222 Jan 13 '18 at 13:51
  • Another option, although very clumsy, is to clone that sealed class into a Dictionary<string, string> or Dictionary<string, object>, implement the update logic with that, and then copy it back into the sealed class. – Ant_222 Jan 15 '18 at 20:04

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.