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.

1 comment:

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.