Coding: Keep method/variable names positive
Something which I've come across a few times recently in code is method names which describe the negative aspect of something and for me at least these are very difficult to understand since I need to keep remembering that we are dealing with the negative and not the positive which I think is significantly easier to reason about.
A recent example of this which I came across was in some acceptance test code which among other things was asserting whether or not the policy number that had been created was in a valid format and returning the result of that assertion back to our Fitnesse fixture.
The code to do this was similar to this – we are using the 'Given, When, Then' syntax for acceptance testing and this code came under the 'Then' section:
public bool ThenCustomerShouldSeePageWithPurchasedPolicy() { ... var policyNumber = GetPolicyNumber(); if(InvalidPolicyNumber(policyNumber)) { return false; } ... }
private bool InvalidPolicyNumber(string policyNumber) { string integerPattern = "^[0-9]{7}$"; return policyNumber.Substring(0,5) != "POLIC" && !Regex.IsMatch(policyNumber.Substring(6,7), integerPattern) && policyNumber.Length != 12 }
Originally the lines of code in 'InvalidPolicyNumber' were actually inlined in the 'ThenCustomerShouldSeePageWithPurchasedPolicy' method but we pulled them out to try and understand the code more easily.
I still found it quite difficult to reason about what made an invalid policy since the reasoning of what makes something invalid from reading this code is:
- The first 5 characters are not 'POLIC'
- The next 7 characters are not digits
- The total length is not 12
The actual rule for a valid policy number are:
- It should start with the characters 'POLIC'
- The next 7 characters should be digits
- The total length should be 12
So in fact our 'InvalidPolicyNumber' method as well as being quite difficult to read will only tell us that a policy number is invalid if all 3 of those conditions have not been met when in actual fact if any of them are not met the policy number is invalid. A perhaps subtle error has crept in!
We refactored the code to refer to what we know is valid rather than looking at the reverse which I think is much more difficult to reason about.
public bool ThenCustomerShouldSeePageWithPurchasedPolicy() { ... var policyNumber = GetPolicyNumber(); if(!ValidPolicyNumber(policyNumber)) { return false; } ... }
private bool ValidPolicyNumber(string policyNumber) { string integerPattern = "^[0-9]{7}$"; return policyNumber.Substring(0,5) == "POLIC" && Regex.IsMatch(policyNumber.Substring(6,7), integerPattern) && policyNumber.Length == 12 }
I've come across a few other examples and the underlying theme is that I find it much easier to reason about code which is written using positive method/variable names and then make use of language constructs if we need to invert that somewhere.
I find it takes me much longer to understand methods which refer to the negative and every time I come back to them in the future I have to reason through each line slowly to make sure I understand what's going on.
Hi Mark,
My 2 cents on this is, I like the method name InvalidPolicyNumber because it's easier to read in your if statement (I don't have to pay and attention to the '!'), I don't disagree that writing a negative can be a little trickier, but I find that '&&' or '||' bugs are so common it helps to always split them out. For ex:
bool invalid = policyNumberSubstring(0,5) != "POLIC";
invalid |= Regex.IsMatch(…) == false;
invalid |= policyNumber.Length != 12;
return invalid;
I find much easier to read than either of the above
Zach Shaw
11 Jun 09 at 11:52 am
[...] Coding: Keep method/variable names positive – Mark Needham talks about a few techniques for keeping names of methods and variables easily understandable [...]
Reflective Perspective - Chris Alcock » The Morning Brew #366
11 Jun 09 at 5:21 pm
Hi Mark,
I always prefer to prefix my methods that return bool with Is, Has or Can it's more readable. Take a look at these methods
IsValidPhoneNumber()
IsAccessAllowed()
CanConnectToServer()
HasMultipleComments()
in my opinion they have great names
Petar Petrov
12 Jun 09 at 4:42 pm
@Petar Good point I think that would make it even more readable in this case.
Mark Needham
12 Jun 09 at 4:46 pm
In this case, I agree with Mark that the InvalidPolicyNumber is hard to grok, and with Zach that the if statement reads better with the original name.
I would implement IsValidPolicyNumber as above, and then have IsInvalidPolicyNumber return !IsValidPolicyNumber.
You get the readability in both client and code then.
Andy Palmer
12 Jun 09 at 8:38 pm
[...] Coding: Keep method/variable names positive at Mark Needham [...]
Daily Links for Friday, June 12th, 2009
12 Jun 09 at 9:39 pm
Why not have a ValidPolicyNumber() method as well as InValidPolicyNumber, if readability is the concern.
private bool ValidPolicyNumber(string policyNumber)
{
string integerPattern = "^[0-9]{7}$";
return policyNumber.Substring(0,5) == "POLIC" && Regex.IsMatch(policyNumber.Substring(6,7), integerPattern) && policyNumber.Length == 12
}
private bool InValidPolicyNumber(string policyNumber){
return !IsValidPolicyNumber(policyNumber);
}
Uttam Kini
25 Feb 10 at 3:51 am