Terse is Worse?

A while ago I had to write a C++ routine that checked whether all the characters in a string were hexadecimal. I quickly seized the moment as an opportunity to flex my C++ muscles and write the most trite routine I could:

bool IsHexString(char *sz)
{
	while ( isxdigit(*sz) && *++sz != NULL ) {}
	return *sz == NULL;
}

Once I had written it and basked for a few hours in the beauty of my code I started to feel quite proud of the fact that it would probably take anyone else at least five minutes to figure out how it worked. Then I realised that maybe, just maybe, that wasn’t actually a good thing. So I re-wrote it. This time I would create a vision of clarity: code that would literally document itself…

bool IsStringHex(char *sz)
{
	char* ch = sz;
 	while ( *ch != NULL )
 	{
 		if ( !isxdigit (*ch) )
 		{
 			return false;
 		}
 	ch++;
 	}
 	return true;
}

Much better I thought. Now, before writing this post I thought I’d gather some opinion from Programmers Heaven about which one was best. Of course I discovered that even my super-readable-re-write was, unfortunately, not perfect. The following points were raised:

  • I was testing for NULL when, strictly, I should have checked for ‘\ 0’
  • There was no need to copy sz into ch
  • The routine had multiple exit points

A further re-write (courtesy of Lundin) that addresses these concerns is:

bool IsStringHex (char* sz)
{
	bool result = true;
	while(*sz != '\ 0')
	{
		if( !isxdigit(*sz) )
		{
			result = false;
			break;
		}
	sz++;
	}
	return result;
}

This has all taught me one thing: never ask anyone to look at your code. Sorry. No. What I meant to say was that there are a few lessons I have learned from this:

  • Most developers prefer readability over terseness
  • Most code can be improved (even if you think it’s perfect)
  • The more people who see the code the better it will become (and the more the original author will learn)
Advertisements

2 comments so far

  1. Patrick McMorris on

    Doesn’t isxdigit() return false on the null byte? The string you’re searching is null terminated. So if you get to the end of the string before finding a non hex digit then success otherwise fail. If so why not do something like this:

    bool IsStringHex (char* sz)
    {
        while(isxdigit(*sz))
        {
            ++sz;   
        }
        return *sz == '\ 0';
    }
    

    The advantage is that your inner loop has one less branch and may run faster.

    Patrick

  2. daviddaly on

    Hi Patrick,

    Thanks for your comment.

    You are correct in saying that isxdigit() returns zero on the null byte so the code can be written as you suggest. However is your code more readable? It may (or may not) run faster depending on the compiler you use. It is not as clear as the final re-write I posted (in my opinion) in that it is not obvious that you are looping through a string and checking one char at a time.


Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s

%d bloggers like this: