If we’ve learned anything from 45 years of C coding, it’s that NULL is evil. At least relying on NULL return values is usually evil, especially when it forces you to write contorted special case handling code. I ran across a case in point today that surprised me because the programmer who wrote it should have known better. And for sure the senior programmer who reviewed the code should have flagged it for rewriting.
There is a method that takes an existing object and updates it based on fields in another object. The code writes a change log entry for each field updated. The code is similar to this:
private bool UpdatePerson(Person person, Models.Person personUpdate)
{
var changes = new List<ChangeLogEntry>();
changes.Add(CompareValues(person.FirstName, personUpdate.FirstName, "First Name"));
changes.Add(CompareValues(person.LastName, personUpdate.LastName, "Last Name"));
// ... other fields here
changes = changes.Where(x => x != null).ToList(); // The CompareValues function returns a ChangeLogEntry if the field has changed. If the field didn’t change, CompareValues returns null.
This is Just Wrong. It works, but it’s wonky. The code is adding null values to a list, knowing that later they’ll have to be removed. Wouldn’t it make more sense not to insert the null values in the first place?
One way to fix the code would be to write a bunch of conditional statements:
ChangeLogEntry temp;
temp = CompareValues(person.FirstName, personUpdate.FirstName, "First Name");
if (temp != null) changes.Add(temp);
temp = CompareValues(person.LastName, personUpdate.LastName, "Last Name");
if (temp != null) changes.Add(temp);
// etc., etc.That’s kind of ugly, but at least it doesn’t write null values to the list.
The real problem is the CompareValues function:
private ChangeLogEntry CompareValues(string oldValue, string newValue, string columnName)
{
if (oldValue != newValue)
{
var change = new ChangeLogEntry(columnName, oldValue, newValue);
return change;
}
return null;
}Yes, it’s poorly named. The method compares the values and, if the values are different, returns a ChangeLogEntry instance. If the values are identical, it returns null. Perhaps that’s the worst thing of all: returning null to indicate that the items are the same. This model of returning null to indicate status is a really bad idea. It forces clients to do wonky things. Of all the horrible practices enshrined in 40+ years of C-style coding, this one is probably the worst. I’ve seen and had to fix (and, yes, written) too much terrible old-style C code to ever advocate this design pattern. null is evil. Avoid it when you can.
And you usually can. In this case it’s easy enough. First, let’s eliminate that CompareValues function:
if (person.FirstName != personUpdate.FirstName) changes.Add(new ChangeLogEntry(person.FirstName, personUpdate.FirstName, "First Name"));
if (person.LastName != personUpdate.LastName) changes.Add(new ChangeLogEntry(person.LastName, personUpdate.LastName, "Last Name"));That’s still kind of ugly, but it eliminates the null values altogether. And we can create an anonymous method that does the comparison and update, thereby removing duplicated code. The result is:
private void UpdatePerson(Person person, Models.Person personUpdate)
{
var changes = new List<ChangeLogEntry>();
// Anonymous method logs changes.
var logIfChanged = new Action<string, string, string>((oldValue, newValue, columnName) =>
{
if (oldValue != newValue)
{
changes.Add(new ChangeLogEntry(columnName, oldValue, newValue));
}
}
logIfChanged(person.FirstName, personUpdate.FirstName, "First Name");
logIfChanged(person.LastName, personUpdate.LastName, "Last Name");
// ... other fields here
if (changes.Any())
{
// update record and write change log
}
}That looks a lot cleaner to me because there are no null values anywhere, and no need to clean trash out of the list.
Here’s the kicker: I didn’t rewrite the code. If the code were as simple as what I presented here, I probably would have changed it. But the code is a bit more complex and it’s currently running in a production environment. I hesitate to change working code, even when its ugliness causes me great mental pain. I’m almost certain that I can make the required changes without negatively affecting the system, but I’ve been burned by that “almost” too many times with this code base. If I had a unit test for this code, I’d change it and depend on the test to tell me if I screwed up. But there’s no unit test for this code because … well, it doesn’t matter why. Let’s just say that this project I inherited has a number of “issues.” The one I’ll relate next time will boggle your mind.