Step 1: Find a case-insensitive string comparison.
boost::remove_erase_if(attributeNameAndValuePairs, [](const auto& nameAndValue) {
return boost::iequals(nameAndValue.name, "foo"); // <-- here
});
return boost::iequals(nameAndValue.name, "foo"); // <-- here
});
Step 2: Trace the use of those strings to find somewhere they are compared with the default comparison. Bug!
if (attributeName == "bar") // <-- bug!
Sometimes it's less obvious:
return std::find(allowedAttributes.cbegin(), allowedAttributes.cend(), attribute.name) != allowedAttributes.cend(); <-- hard to see bug!
I've written this kind of bug many times, and so has everyone else, judging by how often I see it. Recently I wrote it again, and decided to dig a little deeper instead of just fixing this one new case.
Ways to see the problem
Is this a testing problem?
Should we have caught this bug sooner by testing more carefully?
I could write a unit test, asserting that this function handles casing correctly, for each of the dozen or so places that could get this wrong. But if I can remember to write that test, then I can remember to write the code the correct way. Both my code and my unit tests are limited by imagination. How do I catch all the cases?
Getting a second person to do the testing can help: they'll see things I don't see, and that is fantastic. But still, testing is only as good as the imagination of the tester. Also, while that may help us catch the bug before we ship, I want to catch the bug before I check in.
Test Feedback
The real power of Test-Driven Development is not in running the tests to catch defects, but the feedback the tests give you about the design of your code (if you can hear that feedback). What are tests telling us in this case?
With the current design, to eliminate this kind of bug means finding all the places we deal with attribute names, and writing a test. If there's 1000 places, I need 1000 tests. This is a test's way of saying "your business rule is duplicated all over the place". We can look at this as a DRY problem, and can improve the design by refactoring to eliminate that duplication.
Primitive Obsession
We could also look at this as a Primitive Obsession problem: dealing with strings instead of a type that represents a concept from the business domain. Here, the concept is "Attribute Name". The Whole Value pattern says "make and use new objects that represent the meaningful quantities of your business".
Test-as-Spec
Yet another way to look at this problem is from the point of view as "test-as-spec". I want my unit tests to make sense to a person familiar with the problem domain.
The test I want to write is "Attribute names are case-insensitive". With the original design, there's no way to do that, both because the rule is written in many places in the code, and because the rule is embedded in other functions which implement some other business rule.
In order to get test-as-spec, the rule will need a single, canonical home, decoupled from the rest of the system.
Solution
Based on any of the above points of view, we can create a type that represents an attribute name:
struct AttributeName
{
std::string Value;
}
And extract the equality check:
bool operator==(const AttributeName& lhs, const AttributeName& rhs)
{
return boost::iequals(lhs.Value, rhs.Value);
}
And now we can write a single test, named after the business rule that it describes (test-as-spec):
void AttributeNamesAreCaseInsensitive()
{
CPPUNIT_ASSERT(AttributeName{"foo"} == AttributeName{"FOO"});
}
Then we replace any instance of std::string attributeName with AttributeName attributeName and let the compiler tell us what to fix next. Keep chasing compiler errors until they are all gone.
Is this code perfect?
No, but it's better: all the places that compared attribute names before are now slightly simpler / easier to read / easier to write / easier to test.
It's not impossible to write a case-sensitivity bug for attribute names, it's now much harder to do the wrong thing, and much easier to do the right thing.
In the process of carrying out this refactoring I found another instance of this kind of attribute name case-sensitivity bug, which testing had not yet caught. Double win!
What do you think?
