Saving complex business objects with reflection and recursion

Question

In our application we have complex objects with many nesting levels, each with various properties that are important to the business. We currently serialize and persist these objects along with their corresponding workflows (Windows Workflow 4.5), but, due to issues with updating active workflows (WF4.5 Dynamic Update - Issues when updating large, long-running flows with custom activities), we are making a plan for storing the important properties in our SqlServer database in preparation of moving away from Windows Workflow.

Each object is identified by a Filetype and Filenumber, which the business uses to move the data through a long-running Business Process. Some of the child objects and their properties are specific to each Filetype/Filenumber (for example, Customer Name), while others can occur multiple times for each Filetype/Filenumber (for example, objects representing Ad-hoc correspondence).

I've created two custom attributes, [Savable] and [SavableWithContext], and placed them on child objects and their properties where appropriate. I'm then using reflection and recursion to "walk" any complex object sent in, and saving those properties and their values where marked. If they are marked [Savable], I simply save the property name and value, but if they're marked [SavableWithContext], I tack on each parent-object's name as I walk down the object to basic types, so that in the database we can identify which properties those values correspond with. In the logic layer, I build a list with these important properties, which I then send down to our DAL to insert/update as appropriate.

In tests, this code works, and takes about 3 seconds with an object that builds a list of 180 CaseDataItems. This is much faster than the current workflow serialization, but in production there will be many more properties so I'm looking at ways to speed this up. I'm also looking into having this saving happen asynchronously, but because I'm using a lot of reflection/recursion, I'm sure there are many improvements to be made here as well. I've also decided to just store each property, regardless of whether it has a value or not, so there are a lot of empty strings in the Database. My thinking is that its faster to just store rather than query the database first, which I would have to do in case there was originally a value, but then the user cleared the textbox at a later point in the process. Would love a second opinion on whether this is the way to go or not.

(Also, I should note that I'm a junior dev and this has been my most complex task yet, so there may be many basic mistakes that I've made)

The Gist: I'm using reflection and recursion to store complex data objects, and would like to know:

1. Are there any noobish mistakes/improvements I can make with this code?
2. Are there some ways to speed this up?
3. Should I worry about handling empty values rather than storing them in the db no matter what? if so, then:
4. Is there a quick way to handle properties that previously had a value in the database, but now are null/empty from the user?

I've left some of my comments for now, I wasn't sure if I should remove them or not. If you need to see other parts of the code, or an example of what an object looks like, let me know!

Main Code:

Logic Layer:

public static class ObjectPropertyFunctions
{
    private static List<CaseDataItem> caseDataItems;

    private static string parentContextName;

    public static void SaveProperties<T>(T instance)
    {
        try
        {
            caseDataItems = new List<CaseDataItem>();

            AddPropertiesRecursive(typeof(T), instance);

            CaseDataServices.SaveCaseData(caseDataItems);
        }
        catch (Exception ex)
        {
            // Just log the message, but for now don't alert the User.
            Logging.logger.Error(ex.Message, ex);
        }
    }

    private static void AddPropertiesRecursive<T>(Type classType, T instance)
    {
        bool needsContext = false;

        IEnumerable<PropertyInfo> propsToAdd = instance.GetType().GetProperties().Where(
            prop => (Attribute.IsDefined(prop, typeof(SavableAttribute)) ||
                    (Attribute.IsDefined(prop, typeof(SavableWithContextAttribute)))));

        foreach (PropertyInfo property in propsToAdd)
        {
            needsContext = Attribute.IsDefined(property, typeof(SavableWithContextAttribute));

            // Set parentContextname on first object iteration, and skip once base type is hit
            if (needsContext && !(BasicTypes.List.Any(x => x.IsAssignableFrom(property.PropertyType))))
            {
                if (parentContextName.IsNullOrEmpty())
                {
                    parentContextName += property.Name;
                }
                else
                {
                    parentContextName += "." + property.Name;
                }
            }

            // Add basic types, while recursing properties that are objects
            if (BasicTypes.List.Any(x => x.IsAssignableFrom(property.PropertyType)))
            {
                if (property.PropertyType.IsEnum)
                {
                    AddEnum(property, instance, needsContext ? true : false);
                }
                else
                {
                    AddBasicType(property, instance, needsContext ? true : false);
                }
            }
            else if (property.PropertyType.IsClass)
            {
                object value = property.GetValue(instance);
                if (value != null)
                {
                    string tempParent = parentContextName.IsNullOrEmpty() ? "" : parentContextName + ".";

                    AddPropertiesRecursive(property.PropertyType, value);
                }
            }
        }
        // Only remove the parentcontextname if one was added
        if (needsContext)
        {
            RemoveObjectFromParentContextName();
        }
    }

    private static void RemoveObjectFromParentContextName()
    {
        if (parentContextName.IsNullOrEmpty())
            return;

        // Determine whether to reset the string (there was only one object in it), or subtract the last one (there are still objects above)
        int periods = parentContextName.Count(x => x == '.');
        if (periods > 0)
        {
            string[] parentContextParts = parentContextName.Split(new char[] { '.' }, StringSplitOptions.RemoveEmptyEntries);
            parentContextParts = parentContextParts.Take(parentContextParts.Count() - 1).ToArray();
            parentContextName = string.Join(".", parentContextParts);

        }
        else
        {
            parentContextName = "";
        }
    }

    private static void AddBasicType(PropertyInfo property, object instance, bool saveWithContext = false)
    {
        TypeCode typeCode = Type.GetTypeCode(property.PropertyType);
        switch (typeCode)
        {
            case TypeCode.String:
                caseDataItems.Add(new CaseDataItem()
                {
                    DataName = (saveWithContext ? parentContextName + "." : "") + property.Name,
                    DataType = "STRING",
                    StringValue = (string)property.GetValue(instance, null)
                });
                break;
            case TypeCode.DateTime:
                caseDataItems.Add(new CaseDataItem()
                {
                    DataName = (saveWithContext ? parentContextName + "." : "") + property.Name,
                    DataType = "DATETIME",
                    DateTimeValue = (DateTime)property.GetValue(instance, null)
                });
                break;
            case TypeCode.Decimal:
                caseDataItems.Add(new CaseDataItem()
                {
                    DataName = (saveWithContext ? parentContextName + "." : "") + property.Name,
                    DataType = "DECIMAL",
                    DecimalValue = (decimal)property.GetValue(instance, null),
                });
                break;
            case TypeCode.Boolean:
                caseDataItems.Add(new CaseDataItem()
                {
                    DataName = (saveWithContext ? parentContextName + "." : "") + property.Name,
                    DataType = "BOOLEAN",
                    BooleanValue = (bool)property.GetValue(instance, null)
                });
                break;
            case TypeCode.Int16:
            case TypeCode.Int32:
            case TypeCode.Int64:
                caseDataItems.Add(new CaseDataItem()
                {
                    DataName = (saveWithContext ? parentContextName + "." : "") + property.Name,
                    DataType = "INTEGER",
                    IntegerValue = (int)property.GetValue(instance, null)
                });
                break;
            default: break;
        }
    }

    private static void AddEnum(PropertyInfo property, object instance, bool saveWithContext = false)
    {
        object propertyValue = property.GetValue(instance, null);
        string enumString = property.ToString();
        caseDataItems.Add(new CaseDataItem()
        {
            DataName = (saveWithContext ? parentContextName + "." : "") + property.Name,
            DataType = "STRING",
            StringValue = propertyValue.ToString()
        });
    }
}

Data Access Layer:

    public static bool SaveCaseData(List<CaseDataItem> caseDataItems)
    {
        bool ok = false;
        string fileType = Config.Settings.FileType;
        string fileNumber = Config.Settings.FileNumber;

        int totalcount = caseDataItems == null ? -1 : caseDataItems.Where(c => !c.DataName.IsNullOrEmpty() && !c.DataType.IsNullOrEmpty()).Count();
        if (totalcount < 1)
        {
            Logging.logger.Warn("CaseDataItems input is null or empty. Nothing to save.");
        }
        else if (fileType.IsNullOrEmpty() || fileNumber.IsNullOrEmpty())
        {
            Logging.logger.Warn("Configuration does not have file type and number. Unable to save data.");
        }
        else
        {
            using (SqlConnection connection = new SqlConnection(dbConnectionString))
            {
                using (SqlCommand cmd = new SqlCommand())
                {
                    connection.Open();
                    cmd.Connection = connection;
                    int successCount = 0;

                    cmd.CommandText =
                        @"UPDATE [Case].CaseData SET StringValue = @StringValue,
                            IntegerValue = @IntegerValue,
                            BooleanValue = @BooleanValue,
                            DateTimeValue = @DateTimeValue,
                            DecimalValue = @DecimalValue,
                            UpdatedBy = @UserId,
                            UpdatedDate = GetDate()
                        WHERE DataName = @DataName
               AND DataType = @DataType
               AND CaseFTFNId = (SELECT TOP 1 CaseFTFNId 
                                    FROM [Case].CaseFTFN
                                    WHERE FileType = @FileType
                               AND FileNumber = @FileNumber) 
                    IF @@ROWCOUNT=0
                        INSERT INTO [Case].CaseData (
                            CaseFTFNId
                            ,DataName
                            ,DataType
                            ,StringValue
                            ,IntegerValue
                            ,BooleanValue
                            ,DateTimeValue
                            ,DecimalValue
                            ,CreatedBy
                            ,CreatedDate
                            ,UpdatedBy
                            ,UpdatedDate)
               VALUES ((SELECT TOP 1 CaseFTFNId 
                            FROM [Case].CaseFTFN
                            WHERE FileType = @FileType
                           AND FileNumber = @FileNumber) 
                            ,@DataName
                            ,@DataType
                            ,@StringValue
                            ,@IntegerValue
                            ,@BooleanValue
                            ,@DateTimeValue
                            ,@DecimalValue
                            ,@UserId
                            ,GetDate()
                            ,@UserId
                            ,GetDate())";
                    cmd.Parameters.AddWithValue("UserId", Environment.UserName);
                    cmd.Parameters.AddWithValue("FileType", fileType);
                    cmd.Parameters.AddWithValue("FileNumber", fileNumber);
                    cmd.Parameters.Add("DataName", SqlDbType.VarChar);
                    cmd.Parameters.Add("DataType", SqlDbType.VarChar);
                    cmd.Parameters.Add("StringValue", SqlDbType.VarChar);
                    cmd.Parameters.Add("IntegerValue", SqlDbType.Int);
                    cmd.Parameters.Add("BooleanValue", SqlDbType.Bit);
                    cmd.Parameters.Add("DateTimeValue", SqlDbType.DateTime);
                    cmd.Parameters.Add("DecimalValue", SqlDbType.Decimal);

                    foreach (CaseDataItem cdi in caseDataItems.Where(c => !c.DataName.IsNullOrEmpty() && !c.DataType.IsNullOrEmpty()))
                    {
                        cmd.Parameters["DataName"].Value = cdi.DataName;
                        cmd.Parameters["DataType"].Value = cdi.DataType;
                        cmd.Parameters["StringValue"].Value = (object)cdi.StringValue ?? DBNull.Value;
                        cmd.Parameters["IntegerValue"].Value = (object)cdi.IntegerValue ?? DBNull.Value;
                        cmd.Parameters["BooleanValue"].Value = (object)cdi.BooleanValue ?? DBNull.Value;
                        cmd.Parameters["DateTimeValue"].Value = (object)cdi.DateTimeValue ?? DBNull.Value;
                        cmd.Parameters["DecimalValue"].Value = (object)cdi.DecimalValue ?? DBNull.Value;
                        successCount += cmd.ExecuteNonQuery();
                    }
                }
            }
        }
        return ok;
    }

Other objects I'm using in this code:

BasicTypes

static class BasicTypes
{
    public static readonly Type[] List;

    static BasicTypes()
    {
        var types = new[]
        {
            typeof (Enum),
            typeof (string),

            typeof (bool),
            typeof (int),
            typeof (double),

            typeof (DateTime),
            typeof (TimeSpan),
        };

        var nullTypes = from t in types
                        where t.IsValueType
                        select typeof(Nullable<>).MakeGenericType(t);

        List = types.Concat(nullTypes).ToArray();
    }
}

Case Data Item

 public class CaseDataItem
{
    public CaseDataItem() { }

    public bool HasData()
    {
        bool result = false;
        try
        {
            result = !(DataId == 0
                || DataName.IsNullOrEmpty() || DataType.IsNullOrEmpty() 
                || UserId.IsNullOrEmpty());
        }
        catch (Exception ex)
        {
            Logging.logger.Error(ex.Message, ex);
        }
        return result;
    }

    public object GetValue()
    {
        switch (DataType)
        {
            case "STRING":
                return StringValue;
            case "INTEGER":
                return IntegerValue;
            case "DATETIME":
                return DateTimeValue;
            case "BOOLEAN":
                return BooleanValue;
            case "DECIMAL":
                return DecimalValue;
            default:
                return null;
        }
    }

    public int DataId { get; set; }
    public string DataName { get; set; }
    public string DataType { get; set; }
    public string UserId { get; set; }
    public string StringValue { get; set; }
    public int IntegerValue { get; set; }
    public DateTime? DateTimeValue { get; set; }
    public bool BooleanValue { get; set; }
    public decimal DecimalValue { get; set; }
    public DateTime? ModifiedDate { get; set; }       

}

Answer 1

Redundancy

Redundant variables

You have a few unused variables :

private static void AddPropertiesRecursive<T>(Type classType, T instance)

int successCount = 0;

And lastly - bool ok = false;, SaveCaseData return bool but in reality it always returns false. It makes more sense to make the method void. As most save methods are of type void ,it's still fine to have boolean Save method but if the return value changes depending if the saving succeeded but that's not the case in your method.

If you still want to keep the bool method for some reason you should just return false at the end.

General redundancy

if (property.PropertyType.IsEnum)
{
    AddEnum(property, instance, needsContext ? true : false);
}
else
{
    AddBasicType(property, instance, needsContext ? true : false);
}

You don't need to manually check the value of needsContext and depending on that assign a boolean value as needsContext is a boolean variable anyway you can use that :

if (property.PropertyType.IsEnum)
{
    AddEnum(property, instance, needsContext);
}
else
{
    AddBasicType(property, instance, needsContext);
}

switch case with default case which contains only break is redundant

default:break;

Non nullable value type variables compared again null ?

This doesn't makes much sense as value type variables cant have value of null anyway (unless marked as nullable - ?)

cmd.Parameters["DecimalValue"].Value = (object) cdi.DecimalValue ?? DBNull.Value;
cmd.Parameters["IntegerValue"].Value = (object)cdi.IntegerValue ?? DBNull.Value;

You can shorten it to this :

cmd.Parameters["DecimalValue"].Value = (object) cdi.DecimalValue;
cmd.Parameters["IntegerValue"].Value = (object) cdi.IntegerValue;

But wait, cmd.Parameters["DecimalValue"].Value is of type object which means we have boxing going on here, this usually happens implicitly, the opposite is explicit, there is no need of the direct cast to object here.

Shortening the code

Ternary operator

You can use the ternary operator here to shorten the if/else :

if (parentContextName.IsNullOrEmpty())
{
    parentContextName += property.Name;
}
else
{
    parentContextName += "." + property.Name;
}

Like this :

parentContextName += parentContextName.IsNullOrEmpty() ? property.Name : "." + property.Name;

Count with predicate

int totalcount = caseDataItems == null
    ? -1
    : caseDataItems.Where(c => !c.DataName.IsNullOrEmpty() && !c.DataType.IsNullOrEmpty()).Count();

There is no point of first calling .Where() and than calling .Count() as both of those methods accept predicates which means you can reduce it to a single .Count() call like this :

int totalcount = caseDataItems == null
    ? -1
    : caseDataItems.Count(c => !c.DataName.IsNullOrEmpty() && !c.DataType.IsNullOrEmpty());

But you can shorten it even further with the null propagation + null coalescing operator like this :

int totalcount = caseDataItems?.Count(c => !c.DataName.IsNullOrEmpty() && !c.DataType.IsNullOrEmpty()) ?? -1;

string.Join(char separator, IEnumerable<T> value)

You don't need the .ToArray() call here as string.Join() accepts IEnumerable<T> not strictly T[] :

string[] parentContextParts = parentContextName.Split(new char[] {'.'},
    StringSplitOptions.RemoveEmptyEntries);
parentContextParts = parentContextParts.Take(parentContextParts.Length - 1).ToArray();
parentContextName = string.Join(".", parentContextParts);

Can become :

string[] parentContextParts = parentContextName.Split(new char[] {'.'},
    StringSplitOptions.RemoveEmptyEntries);
parentContextName = string.Join(".", parentContextParts.Take(parentContextParts.Length - 1));

• Bonus tip

  Generally, using .Take() to take all but the last element isn't recommended in my opinion as .Take() is suitable for all IEnumerable<T> but in order to obtain the length of a pure IEnumerable<T> you need to call .Count() which iterates the collection once and you need to iterate the collection once again to perform the O(n) operation .Take().

  It might be better to implement your own extension method which ensures that there is a single iteration over the IEnumerable<T> for example you might utilise the enumerator ?

  This doesn't concerns you however since you are using array and obtaining the length of array is O(1) operation while the the same operation on pure IEnumerable<T> would be O(n).

Suggestions

1. You can certainly use some generics here and there to avoid the casting.
2. You should wrap TypeCode in a Dictionary combined with a simple Action resulting in Dictionary<TypeCode, Action>, so you can lookup the key and invoke the value in a single line, which is a lot more readable. But again generics will remove the need of all of those lines anyway.

Answer 2

public static class ObjectPropertyFunctions

Classes should have singular names. They also should not be static if they don't provide some stateless general purpose functions. This one however isn't stateless. It works with private static fields. If SaveProperties would be called by multiple threads strange things will happen.

---

DataType = "STRING",

You use the TypeCode.String for the switch. Why don't you use it everywhere but the ugly "STRING" value?

---

public bool HasData()
{
    bool result = false;
    try
    {
        result = !(DataId == 0
            || DataName.IsNullOrEmpty() || DataType.IsNullOrEmpty() 
            || UserId.IsNullOrEmpty());
    }
    catch (Exception ex)
    {
        Logging.logger.Error(ex.Message, ex);
    }
    return result;
}

I don't think this could throw an exception. If it does then you have a bug somewhere.

---

switch (DataType)
{
    case "STRING":
        return StringValue;
    case "INTEGER":
        return IntegerValue;
    case "DATETIME":
        return DateTimeValue;
    case "BOOLEAN":
        return BooleanValue;
    case "DECIMAL":
        return DecimalValue;
    default:
        return null;
}

More hard-coded strings. Use the TypeCode like you do with the other switch.

And for this one too:

public string DataType { get; set; }

---

public int IntegerValue { get; set; }

There is not such type as Integer. It's called Int32.

Answer 3

If I read this correctly you have one table for data that can be any data type (integer, string, decimal ...) and yet it can really only be one of those? That is massively wasteful. Not just db space but time for update and insert. Have a table for each datatype with FK to a master table just like the hierarchy of the data object below.

I an not following this

string fileType = Config.Settings.FileType;
string fileNumber = Config.Settings.FileNumber;

You are reading business object data from Config? Why not have a business object with those and List as properties?

Looking up CaseFTFNId inside the loop is just plain bad.

Update insert based on @@rowcount is inefficient. Google upsert. It uses TSQL MERGE.

My take is you are doing too much with public class CaseDataItem
Currently you could set DecimalValue on DataType of INTEGER

Use an abstract class
For one you get value datatype enforced

public enum DataType { String, Integer, DateTime, Decimal}
public abstract class CaseDataItem
{
    public CaseDataItem(int id)
    {
        ID = id;
    }
    public CaseDataItem(int id, string name)
    {
        ID = id;
        Name = name;
    }               
    public int ID { get; }
    public string Name { get; set; }
    public abstract DataType DataType { get; }
    public abstract string StringValue { get; }
    public abstract object GetObjectValue();
}
public abstract class CaseDataItemString : CaseDataItem
{
    public CaseDataItemString(int id)  : base(id)
    { }
    public CaseDataItemString(int id, string name) : base(id, name)
    { }
    public CaseDataItemString(int id, string name, string value) : base(id, name)
    {
        Value = value;
    }
    public string Value { get; set; }
    public override string StringValue
    {
        get
        {
            return Value;
        }
    }
    public override object GetObjectValue()
    {
        return Value;
    }
    public override DataType DataType
    {
        get
        {
            return DataType.Integer;
        }
    }
}
public abstract class CaseDataItemInteger : CaseDataItem
{
    public CaseDataItemInteger(int id) : base(id)
    { }
    public CaseDataItemInteger(int id, string name) : base(id, name)
    { }
    public CaseDataItemInteger(int id, string name, int? value) : base(id, name)
    {
        Value = value;
    }
    public int? Value { get; set; }
    public override string StringValue
    {
        get
        {
            return Value == null ? string.Empty : Value.ToString();
        }
    }
    public override object GetObjectValue()
    {
        return Value;
    }
    public override DataType DataType
    {
        get
        {
            return DataType.Integer;
        }
    }
}