Photo by Jon Tyson on Unsplash
The other day, during a code review I came across code that looked like this in a Controller’s action method.
foreach (var fooRow in importRows.Select(importRow => composer.Compose<FooRow>()
.UsingParameters(new FooRowParameters {Fields = model.FooListFields, ImportRow = importRow})))
{
numberOfRecordsInSourceFile++;
var fooNumber = 0;
var sequenceNumber = 0;
fooListImportIsValid = int.TryParse(fooRow.FooNumber, out fooNumber);
fooListImportIsValid = fooListImportIsValid &&
int.TryParse(fooRow.SequenceNumber, out sequenceNumber);
verificationRows.Add(new Verify.FooListVerificationRow
{FooNumber = fooNumber, FooTypeCode = fooRow.FooTypeCode, SequenceNumber = sequenceNumber});
previouslyImportedFooListNumbers = numberedFoosInSystem.Any(nfis => nfis.Spec.Number == fooNumber);
fooListImportIsValid = fooListImportIsValid && !previouslyImportedFooListNumbers;
fooListImportIsValid = fooListImportIsValid && fooListEntries.All(fle => fle.FooNumber != fooNumber);
fooListImportIsValid = fooListImportIsValid && fooListEntries.All(fle => fle.SequenceNumber != sequenceNumber);
fooListImportIsValid = fooListImportIsValid && numberedFoosInSystem.All(nfis => nfis.Spec.SequenceNumber != sequenceNumber);
fooListImportIsValid = fooListImportIsValid && fooListEntries.All(fle => fle.SequenceNumber > 0);
fooListImportIsValid = fooListImportIsValid && fooListEntries.All(fle => fle.FooNumber > 0);
fooListImportIsValid = fooListImportIsValid && fooTypes.Any(ft => ft.Code == fooRow.FooTypeCode);
if (!fooListImportIsValid)
break;
}
This code is running a bunch of validation on something called a FooList (I’ve replaced a keyword with Foo here b/c of where the code comes from). What is a FooList is is not important, but it’s important to know this is a bulk import into a system, potentially hundreds of thousands of rows.
Where to begin.
- I have a list of validations, and unless I am VERY familiar with this part of the system, I have no way of figuring out which is these LINQ statements enforces which validation.
- Because this import is in foreach loop, it’s hard to tell which one of these validation checks corresponds back to the unit test I’m running without putting a breakpoint one one line, running the test and checking for the value of “fooListImportIsValid” above and putting a breakpoint below that line as well to figure out on which line the “fooListImportIsValid” variable was “flipped” to false.
- Overall, this code is giving me a headache
So I refactored it:
var fooListImportRows = importRows.Select(importRow => composer.Compose<FooRow>()
.UsingParameters(new FooRowParameters { Fields = model.FooListFields, ImportRow = importRow }));
foreach (var fooRow in fooListImportRows)
{
numberOfRecordsInSourceFile++;
int fooNumber;
int sequenceNumber;
if (!int.TryParse(fooRow.FooNumber, out fooNumber))
fooListImportIsValid = false;
if (!int.TryParse(fooRow.SequenceNumber, out sequenceNumber))
fooListImportIsValid = false;
verificationRows.Add(new Verify.FooListVerificationRow
{FooNumber = fooNumber, FooTypeCode = fooRow.FooTypeCode, SequenceNumber = sequenceNumber});
if (FooListEntriesContainsPreviouslyImportedFooListNumbers(numberedFoosInSystem, fooNumber))
{
previouslyImportedFooListNumbers = true;
fooListImportIsValid = false;
}
if (FooListEntriesContainsDuplicateFooNumbers(fooListEntries, fooNumber))
fooListImportIsValid = false;
if (FooListEntriesContainsDuplicateSequenceNumbers(fooListEntries, sequenceNumber))
fooListImportIsValid = false;
if (FooListEntriesContainsPreviouslyImportedSequenceNumber(numberedFoosInSystem, sequenceNumber))
fooListImportIsValid = false;
if (FooListEntriesContainsANegativeSequenceNumber(fooListEntries))
fooListImportIsValid = false;
if (FooListEntriesContainsANegativeFooNumber(fooListEntries))
fooListImportIsValid = false;
if (FooListEntriesDoesNotContainAFooCodeAlreadyInTheSystem(fooTypes, fooRow.FooTypeCode))
fooListImportIsValid = false;
if (!fooListImportIsValid)
break;
}
Much better! Now, instead of a bunch of LINQ statements strung together with a boolean flag that could be flipped to false on any line through each validation check, I’ve encapsulated the LINQ in methods that are NAMED AFTER THE VALIDATION.
private bool FooListEntriesContainsPreviouslyImportedFooListNumbers(IEnumerable<NumberedFoo> numberedFoosInSystem, int fooNumber)
{
return numberedFoosInSystem.Any(nfis => nfis.Spec.Number == fooNumber);
}
More importantly, I’ve captured business reasons for making these checks in the first place. FooListEntriesDoesNotContainAFooCodeAlreadyInTheSystem
is going to make a lot more sense upon first glance than a LINQ statement, especially for any developer not familiar with this part of the system.
I’ve also gained the ability when debugging my unit tests to put a breakpoint on the fooListImportIsValid = false;
line and KNOW that the reason I’ve hit the validation failure is because of the method directly above. This will help ensure during my code review that I can prove all of this coded is tested much easier than it was before.
I’ve gained both readability and speed for whoever has to look at this code next to have a better chance at understanding what’s going on. It took me a couple hours of painstaking refactoring to get the code in this shape.
Imagine how much easier it would have been if the original developer had just written the code like this to begin with?
Which code would you rather code review/maintain? I bet I can guess it would be the refactored version.