16
\$\begingroup\$

I was asked to update a business rule to allow a specific column to be added. Because our datalayer uses only DataSets as an interface to the database that is what we work with in the business layer.

Here I have created a couple of DataTable and DataRow extensions to use for this businessrule and I would like your opinion on things I might have missed. This is the method I use to search for changes:

private static bool hasColumnChanged(StringComparison stringComparison, bool ignoreWhitespace, DataRow row, DataColumn col)
{
    bool isEqual = true;
    if (row[col, DataRowVersion.Original] != DBNull.Value && row[col, DataRowVersion.Current] != DBNull.Value)
    {
        if (ignoreWhitespace)
            isEqual = row[col, DataRowVersion.Original].ToString().Trim().Equals(row[col, DataRowVersion.Current].ToString().Trim(), stringComparison);
        else
            isEqual = row[col, DataRowVersion.Original].ToString().Equals(row[col, DataRowVersion.Current].ToString(), stringComparison);
    }
    else
        isEqual = row[col, DataRowVersion.Original].Equals(row[col, DataRowVersion.Current]);

    return !isEqual;
}

And these are simply the extensions using the code:

public static List<DataColumn> GetChangedColumns(this DataTable table) 
{
    return table.GetChangedColumns(StringComparison.InvariantCultureIgnoreCase, false);
}
public static List<DataColumn> GetChangedColumns(this DataTable table, bool ignoreWhitespace)
{
    return table.GetChangedColumns(StringComparison.InvariantCultureIgnoreCase, ignoreWhitespace);
}
public static List<DataColumn> GetChangedColumns(this DataTable table, StringComparison stringComparison, bool ignoreWhitespace)
{
    if (table == null) throw new ArgumentNullException("table");

    List<DataColumn> columnsChanged = new List<DataColumn>();
    foreach (DataRow row in table.GetChanges().Rows)
    {
        foreach (DataColumn col in row.Table.Columns)
        {
            if (!columnsChanged.Contains(col) && hasColumnChanged(stringComparison, ignoreWhitespace, row, col))
                columnsChanged.Add(col);
        }
    }
    return columnsChanged;
}

public static List<DataColumn> GetChangedColumns(this DataRow row)
{
    return row.GetChangedColumns(StringComparison.InvariantCultureIgnoreCase, false);
}
public static List<DataColumn> GetChangedColumns(this DataRow row, bool ignoreWhitespace)
{
    return row.GetChangedColumns(StringComparison.InvariantCultureIgnoreCase, ignoreWhitespace);
}
public static List<DataColumn> GetChangedColumns(this DataRow row, StringComparison stringComparison, bool ignoreWhitespace)
{
    if (row == null) throw new ArgumentNullException("row");

    List<DataColumn> columnsChanged = new List<DataColumn>();
    foreach (DataColumn col in row.Table.Columns)
    {
        if (!columnsChanged.Contains(col) && hasColumnChanged(stringComparison, ignoreWhitespace, row, col))
            columnsChanged.Add(col);
    }
    return columnsChanged;
}

To test the above code I use this simple unit test:

[TestMethod]
public void DataTableAndDataRowGetChangedColumns()
{
    DataSet ds = GetDummyDataSet();
    ds.Tables[0].Rows[0][3] = DateTime.Now;
    ds.Tables[0].Rows[0][2] = ds.Tables[0].Rows[1][2].ToString() + " ";
    ds.Tables[0].Rows[0][1] = DBNull.Value;
    List<DataColumn> changesForRow = ds.Tables[0].Rows[0].GetChangedColumns(true);
    List<DataColumn> changesForTable = ds.Tables[0].GetChangedColumns(true);

    // For now we just verify if the amount of changes is the same (ez way out)
    Assert.IsTrue(changesForRow.Count.Equals(changesForTable.Count));
}

In the business rule I have implemented this piece of code which verifies if there are other datacolumns that have changed:

List<DataColumn> columnsChanged = dsChanges.Tables[0].GetChangedColumns(true);
if(columnsChanged.Any(c=>!c.ColumnName.Equals("DateUntill", StringComparison.InvariantCultureIgnoreCase)))
    throw new BusinessException("This premium can not be changed, only DateUntill can still change");
\$\endgroup\$

1 Answer 1

13
\$\begingroup\$
  1. hasColumnChanged method. Inside first if you have two almost same lines. Duplicated code should be extracted:

    if (row[col, DataRowVersion.Original] != DBNull.Value && row[col, DataRowVersion.Current] != DBNull.Value)  
    {  
        string originalVersionToCompare = row[col, DataRowVersion.Original].ToString();
        string currentVersionToCompare = row[col, DataRowVersion.Current].ToString();
        if (ignoreWhitespace)
        {
            originalVersionToCompare = originalVersionToCompare.Trim();
            currentVersionToCompare = currentVersionToCompare.Trim();
        }
        isEqual = originalVersionToCompare.Equals(currentVersionToCompare, stringComparison);
    }    
    
  2. If you have .Net 4.0 then 6 GetChangedColumns methods I would refactor into 2 with optional parameters.

  3. foreach inside GetChangedColumns for DataRow looks like a copypaste. I cannot imagine a situation when columnsChanged.Contains(col) will be true in this method.

  4. hasColumnChanged seems to be named incorrectly. It should be hasCellChanged since it checks for intersection of row and column. Maybe also PascalCase?

  5. LINQify it!

    Original:

    List<DataColumn> columnsChanged = new List<DataColumn>();
    foreach (DataRow row in table.GetChanges().Rows)
    {
        foreach (DataColumn col in row.Table.Columns)
        {
            if (!columnsChanged.Contains(col) && hasColumnChanged(stringComparison, ignoreWhitespace, row, col))
                columnsChanged.Add(col);
        }
    }
    return columnsChanged;
    

    Result:

    return table.GetChanges().Rows.Cast<DataRow>()
        .SelectMany(dr => table.Columns.Cast<DataColumn>(), (row, column) => new {row, column})
        .Where(c => hasColumnChanged(stringComparison, ignoreWhitespace, c.row, c.column))
        .Select(c => c.column)
        .Distinct()
        .ToList();
    
  6. DateUntill - double l?

  7. Why do you compare items by casting them to string? Why don't you compare them as objects?

\$\endgroup\$
6
  • \$\begingroup\$ Wauw this is some really good feedback. I thought my initial code was a very good start so I wouldn't get much suggested changes. But you've made it much clearer and taught me a thing or two :-) I'm using camelCasing on the private methods as this is our inhouse coding convention. Thanks! \$\endgroup\$
    – Peter
    Mar 17, 2011 at 10:40
  • \$\begingroup\$ 7. I compare them by string so I'm able to specify a StringComparison type, else I have to check if the column datatype is of string first. In the end I don't see the problem in doing this as the result of "1"=="1" equals 1==1. And this is true for all datatypes since the conversion from datatype to string is the same for both. \$\endgroup\$
    – Peter
    Mar 17, 2011 at 10:47
  • \$\begingroup\$ @Peter, I would better add string type check other than remembering how different objects are converted to string. For example usually DateTime.ToString() doesn't print milliseconds so your comparison will not catch changes in milliseconds. That's just an example, probably it won't be an often situation. \$\endgroup\$
    – Snowbear
    Mar 17, 2011 at 10:57
  • 1
    \$\begingroup\$ Actually, optional arguments is not a feature of .net 4.0, but of the c# 4.0 compiler (which is included in VS2010 and can be used also with .net 3.5 for example). \$\endgroup\$
    – Teejay
    Nov 11, 2013 at 10:42
  • 1
    \$\begingroup\$ Though I have a question, shouldn't you simply use: !Equals(row[col, DataRowVersion.Original], row[col, DataRowVersion.Current])? \$\endgroup\$ Jul 9, 2015 at 20:37

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.