Why do I do that

One of the good things about working on a code base of questionable quality is that you get a chance to review the way you work as well as they way the original authors worked. It’s my job is to improve the quality of the code and the existing code is so different to how I would have structured it that I find myself questioning the way that I do things. Is my way better or just different?

I find that I’m always analysing my actions, so here’s a bit of why do I code like that analysis… Join in and tell me why I’m wrong.

I still spend most of my time in C++ as the people I write code for aren’t ready to move to .Net just yet. My coding style evolves over time, but right now my C++ tends to look like this:

// Example 1

bool CValidationData::ValidateKey(
   const CString &key) const
{	
   bool valid = false;

   ValueMap::const_iterator it = m_map.find(key);

   if (it != m_map.end())
   {
      valid = true;
   }

   return valid;
}

rather than like this:

// Example 2

bool CValidationData::ValidateKey(
   const CString &key) const
{	
   return m_map.end() != m_map.find(key);
}

Note: the examples above aren’t anything to do with what I’m refactoring to and what I’m refactoring from. They’re just different ways that my own code has looked over the years. The code we’re cleaning up would likely have the validation test inline in the middle of a 200 line function and the variables would be called tmpy and yoy.

My current thinking is that it’s better to be verbose and obvious than terse and clever. Verbose and obvious might take me slightly longer to write, but it’s often quicker to read and understand. If it’s less efficient then we deal with it when we profile. The problem with terse and clever is that the level at which things become obscure will vary with each individual reader. What is obvious to me may be impenetrable to the guy doing maintenance on the code in 6 weeks time - especially if that’s also me. I know it’s nice to make assumptions about the level of skill that people reading your code will posess, but sometimes you can’t - is example 2 still good if you don’t know how STL containers work?

Some would say that I should go with the terse style and add comments to help out, but comments don’t compile; so comments rot quicker than code. Besides, what would the comment say that the code doesn’t, if the terse code is obvious then it possibly doesn’t require a comment either…

What’s more the code in example 1 would have initially been written like this:

// Example 3

bool CValidationData::ValidateKey(
   const CString &key) const
{	
   bool valid = false;

   // validate the key, somehow...

   return valid;
}

I may have been working on some other code at the time, decided I needed to be able to validate the key and stubbed out the code quickly so that I could go back to where I was in the calling code without interrupting the flow. If I were test driven then I’d have written a test for the above first…

I expect quite a few people to hate the wordiness of example 1. It does have advantages though; if I happen to step into the code in a debugger I can see what’s going on relatively easily. It has all of the context that I might need to understand what’s happening, the outcome, and how we got there. All of it is viewable in the debugger at that point. I don’t need to step out of the function to know if we found the key and I don’t need to worry about not being able to look inside the STL containers because they’re all templatey and twisty…

Likewise, I tend to choose this:

// Example 4

bool CGamingTable::UserIsPlaying(
   const CUser *pUser) const
{
   bool found = false;

   const size_t numSeats = m_seats.size();

   for (size_t i = 0; !found && i < numSeats; ++i)
   {
      const CUser *pThisUser = m_seats[i];

      if (pUser == pThisUser)
      {
         found = true;
      }
   }

   return found;
}

or this:

// Example 5

bool CGamingTable::UserIsPlaying(
   const CUser *pUser) const
{
   bool found = false;

   for (Seats::const_iterator it = m_seats.begin(); !found && it != m_seats.end(); ++it)
   {
      const CUser *pThisUser = *it;

      if (pUser == pThisUser)
      {
         found = true;
      }
   }

   return found;
}

over this:

// Example 6

bool CGamingTable::UserIsPlaying(
   const CUser *pUser) const
{
   for (size_t i = 0; i < m_seats.size(); ++i)
   {
      if (pUser == m_seats[i])
      {
         return true;
      }
   }
   return false;
}

And it’s a close thing between 4 and 5. As with 1 we have the visibility in the debugger advantage. It’s annoying to wonder which user we’re currently looking at and not be able to dereference the iterator in the watch window - why doesn’t VC6 let you do that? The actual choice between them would depend if I might be interested in the fact that m_seats is a std::vector and perhaps the seat index could be important to me during testing and debugging? Generally I think example 5 is better because it hides the nature of the container and we could switch to using a std::list more easily. I wasn’t sure I thought that way until reading the first draft of this entry…

Discuss.