3
\$\begingroup\$

Is there a way I can make the code compact and with fewer ifs?

theData = GetData();
if (theData.Rows.Count > 0)
{
    MyModel = new CustomModel();
    dataSetRow = theData.Rows[0];
    if (theData.Columns.Contains("Column1"))
    {
        if ((!object.ReferenceEquals(dataSetRow["Column1"], DBNull.Value)))
        {
            MyModel.Column1 = Convert.ToString(dataSetRow["Column1"]);
        }
    }
    if (theData.Columns.Contains("Column2"))
    {
        if ((!object.ReferenceEquals(dataSetRow["Column2"], DBNull.Value)))
        {
            MyModel.Column2 = Convert.ToString(dataSetRow["Column2"]);
        }
    }
    if (theData.Columns.Contains("Column3"))
    {
        if ((!object.ReferenceEquals(dataSetRow["Column3"], DBNull.Value)))
        {
            MyModel.Column3 = Convert.ToString(dataSetRow["Column3"]);
        }
    }
    if (theData.Columns.Contains("Column4"))
    {
        if ((!object.ReferenceEquals(dataSetRow["Column4"], DBNull.Value)))
        {
            MyModel.Column4 = Convert.ToString(dataSetRow["Column4"]);
        }
    }
\$\endgroup\$
  • \$\begingroup\$ I see 4 very similar blocks. Reduce them to one by using a loop over the constants "Column1", "Column2", "Column3", "Column4" \$\endgroup\$ – MrSmith42 Jan 28 '15 at 12:15
  • 2
    \$\begingroup\$ Is this real code ? \$\endgroup\$ – Heslacher Jan 28 '15 at 12:33
  • \$\begingroup\$ @MrSmith42 MyModel.Column3 would not be replacable by MyModel["Column3"] or something similar as it is a property of a class. \$\endgroup\$ – Abbas Jan 28 '15 at 12:45
  • \$\begingroup\$ @Abbas You are right but you can still use a loop and use the loop index to select 'MyModel.Column1' to 'MyModel.Column4' which you stored in an array. \$\endgroup\$ – MrSmith42 Jan 28 '15 at 15:27
  • \$\begingroup\$ How are you going to select the property with an indexer without using reflection? \$\endgroup\$ – Abbas Jan 28 '15 at 15:33
4
\$\begingroup\$

Useful property names:

I find using names as theData bad practice. It doesn't give any info on the instance. Give it a useful name you, and others, easily understand.

Casing of property names:

Don't use PascalCase for local fields, use camelCase instead. Your MyModel will become myModel. You had already done this correctly for dataSetRow.

Redundant parentheses:

Following:

if ((!object.ReferenceEquals(dataSetRow["Column1"], DBNull.Value)))

can be replaced by:

if (!object.ReferenceEquals(dataSetRow["Column1"], DBNull.Value))

There's no need to encapsulate the whole expression again in parantheses.

Simplifying the code:

Finally, the important part! :) There's only one way I can think of to achieve this: reflection. I created following extension method to get the PropertyInfo of your instance (note that it doesn't have to be placed in an extension method, I only think it looks cleaner and is good for reusability elsewhere if needed):

public static class Extensions
{
    public static PropertyInfo GetProperty<T>(this T obj, string propertyName) where T : class
    {
        return typeof(T).GetProperty(propertyName);
    }
}

The above allows you to do following (get a property by it's name-value):

public class User
{
    public int ID { get; set; }
    public string Name { get; set; }
}

var user = new User { ID = 1, Name = "Simsons" };
var nameProp = user.GetProperty("Name");

And now you can get and/or set the value of that property using the GetValue() or SetValue() methods:

nameProp.SetValue(user, "Abbas", null);

With this in mind, add following changes to your code:

  • place all your column names in an array over which you will loop
  • simplify your code using the reflection part

Here's your code with the above applied:

theData = GetData();

if (theData.Rows.Count > 0)
{
    myModel = new CustomModel();
    dataSetRow = theData.Rows[0];
    var columnNames = new [] { "Column1", "Column2", "Column3", "Column4" };

    foreach(var columName in columnNames)
    {
        if(theData.Columns.Contains(columName))
        {
            if (!object.ReferenceEquals(dataSetRow[columName], DBNull.Value))
            {
                var columnProperty = myModel.GetProperty(columName);
                columnProperty.SetValue(myModel, Convert.ToString(dataSetRow[columnName], null);
            }
        }
    }
}

Update:

As RobH stated in the comments, you can use LinQ to avoid nested if statements. Here's how your code would look like:

var columnNames = new [] { "Column1", "Column2", "Column3", "Column4" };

foreach(var existingColumn in columnNames.Where(x => theData.Columns.Contains(x)))
{
    if (!object.ReferenceEquals(dataSetRow[existingColumn], DBNull.Value))
    {
        var columnProperty = myModel.GetProperty(existingColumn);
        columnProperty.SetValue(myModel, Convert.ToString(dataSetRow[existingColumn], null);
    }
}

You can even take it a final step further to avoid any if statement:

var validColumns = columnNames.Where(x => theData.Columns.Contains(x) &&
                                          !object.ReferenceEquals(dataSetRow[x], DBNull.Value)))

foreach(var column in validColumns)
{
    var columnProperty = myModel.GetProperty(existingColumn);
    columnProperty.SetValue(myModel, Convert.ToString(dataSetRow[existingColumn], null);
}
\$\endgroup\$
  • \$\begingroup\$ +1. You could convert most of that foreach to LINQ though, might look a bit nicer than the nested ifs. \$\endgroup\$ – RobH Jan 28 '15 at 14:34
  • \$\begingroup\$ @RobH Thanks for the tip, updated my answer. ;) \$\endgroup\$ – Abbas Jan 28 '15 at 15:32
1
\$\begingroup\$

If you can refactor your model class so that the column names and values are in a dictionary instead of separate properties everything an be simplified to one LINQ statement:

class CustomModel
{
    public Dictionary<string, string> ColumnValues = new Dictionary<string, string>
        {
            { "Column1", "" },
            { "Column2", "" },
            { "Column3", "" },
            { "Column4", "" }
        };
}

var dataSetRow = theData.Rows[0];
CustomModel MyModel = new CustomModel();
MyModel.ColumnValues = (from DataColumn c in theData.Columns
                    where MyModel.ColumnValues.ContainsKey(c.ColumnName) && dataSetRow[c] != DBNull.Value
                    select new KeyValuePair<string, string>(c.ColumnName, dataSetRow[c].ToString())).ToDictionary(x => x.Key, x => x.Value);
\$\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.