04 March 2006

Readability

Last week in a code review I ended up writing a little code like this:

void Frobs::tidy()
{
    for (PtrVec::size_type n = 0; n < m_vec.size(); )
    {
        if (m_vec[n]->isExpired())
            vec.erase(n);
        else
            n++;
    }
}

The guy I was working with prefered this, saying it would be easier to read:

void Frobs::tidy()
{
    for (PtrVec::size_type n = 0; n < m_vec.size(); )
    {
        m_vec[n]->isExpired() ? vec.erase(n) : n++;
    }
}

Obviously I disagree, or there'd be nothing to write about. What do you think? My take is in the comments.

2 comments:

jto said...

The second version is tidier, but I don't think it's easier to read.

For one thing, I suspect a lot of programmers slow down and mentally change gears when they see ?:. It's rarely used, so it stands out. And it's is an indication that the coder thought he was being clever, so it cues me to try and fully understand what's going on.

More abstractly, I feel like "if" and "?:" have two different use cases. "?:" is for choosing between two values, as in:

printf("flag: %s\n", (flag ? "true" : "false"));

"if" is for choosing between two courses of action. There's nothing in the C++ spec that would support this, just a style thing.

Also: the version using "if" needs less of a change later, when you need to add another line to one branch or the other.

And, personally, I associate ?: with the Obfuscated C context.

I am certain that a very large majority of particularly smart programmers have ideas about readability and usability that are completely at odds with reality.

Mattie said...

I agree with your answer. My preference would actually be:

void Frobs::tidy() {
  for (PtrVec::size_type n = 0; n < m_vec.size(); ) {
    if (m_vec[n]->isGood()) {
      vec.erase(n);
    }
    else {
      n++;
    }
  }
}

(I can't use pre/code tags in the comments so I can't say for sure if this formatted right)

I prefer to put blocks around items even if I only have one statement. Any average programmer wouldn't have problems with it, but I don't want to risk that someone coming to add conditions later will take the if/else indention as a sign of blocks and just add another line they expect to be in the "else" clause. Working with junior programmers/interns/contractors has driven me to worry about silly things like that.

The gist is that I tend to think more for maintainability than for tidyness.

I'd rarely use the ?: operator in the way your coworker recommended and it feels abnormal to me. I tend to use it as a choice for values and not a choice of operations. While I use it religiously in quick-and-dirty PHP to save my typing time, I actually avoid using ?: in C/C++ because I feel it reduces the readability and maintainability of code. It's rare when I feel it simplifies understanding of the code in question.

-Mattie