Mark Needham

Thoughts on Software Development

Coding: Keep method/variable names positive

with 8 comments

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.

Be Sociable, Share!

Written by Mark Needham

June 11th, 2009 at 7:44 am

Posted in Coding

Tagged with

  • 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

  • Pingback: Reflective Perspective - Chris Alcock » The Morning Brew #366()

  • 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 Good point I think that would make it even more readable in this case.

  • 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.

  • Pingback: Daily Links for Friday, June 12th, 2009()

  • Uttam Kini

    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);
    }

  • Guest

    I feel as though method names should be verbs, because they *do* something. Properties and variables are nouns, because they *are* something. Therefore, the method here should be called IsValidPolicyNumber().