Clever Code != Friendly Code

by Seanba on November 7, 2010

The other day I was looking at some old code I’ve written.

Ugh.

Now, I tend to agree with Jeff Atwood’s Suck Less Every Year mantra when he says:

You should be unhappy with code you wrote a year ago. If you aren’t, that means either A) you haven’t learned anything in a year, B) your code can’t be improved, or C) you never revisit old code. All of these are the kiss of death for software developers.

But it’s strange how even the “good” or “smart” code from times gone by has a horrible code smell to it now.

Case in point:

using namespace std;

for_each(obsFirst, obsLast, bind2nd(mem_fun(&Observer::LightMoved), light));

Quick – what does that code above perform? Unless you’re an old-school Standard Template Library user (the usage has been simplified with Technical Report 1) then you’re probably going to have to look something up.

std::for_each is itself easy enough to grok:

using namespace std;

for_each(first, last, someFunction);

This simply invokes a function or function object (anything supporting the parentheses operator) for each object within the range from first to last, passing in the object as an argument to someFunction.

But say you need to pass in two arguments to the function called? Then you’ll need std::bind2nd.

using namespace std;

for_each(first, last, bind2nd(someFunction, arg2));

This will call someFunction(arg1, arg2), getting each instance of arg1  from the range of first to last.

Actually, that code above won’t compile, because std::bind2nd  requires, as its first argument, a functor object with particular class traits, so we have to wrap the function appropriately with an adaptor.

using namespace std;

for_each(first, last, bind2nd(ptr_fun(someFunction), arg2));

You can see things are starting to get complicated here. Now say you don’t want to call a free function for each object in a range with a second argument, but rather a member function on the object. That’s why std::mem_fun is needed, to wrap a member function into a form needed by std::bind2nd.

using namespace std;

for_each(first, last, bind2nd(mem_fun(&Foo::SomeMemberFunction), arg2));

Yuck, what a mess! But written another way, this is exactly what that code above does …

for (fooIterator = first; fooIterator != last; ++fooIterator)

{

    *fooIterator->SomeMemberFunction(arg2);

}

… which is actually a much better way of explaining the whole eyesore. So why not just write the code that way to begin with and be done with it?

To be honest, I read about for_each, bind2nd, and mem_fun in some book about STL and thought it was pretty clever, so I started a new habit of using those template functions whenever I could.

And, unfortunately for me, the very first time I had a code review with a for_each  loop of this fashion in it, the reviewer had these reactions:

  • First impression: “What the f*** is this s***?”
  • After explaining: “Oh cool, I didn’t even know you could do that with STL.”

I wish he had of just kept with his first impression and told me to rewrite that line of code into something readable, but he was impressed with the new knowledge I shared with him, and I couldn’t help but feel smart about the whole stupid thing.

Funny enough, I was asked to add a code comment though:

using namespace std;

 

// Call observer->LightMoved(light) for all observers

for_each(obsFirst, obsLast, bind2nd(mem_fun(&Observer::LightMoved), light));

Bleh. I just think that comment makes it all the more obvious that there is something wrong with the code.

Keep it simple, stupid

There was a time in my development as a programmer where I really cared if people thought I was being clever, and had beguiled myself into thinking that the intelligence of a programmer was best demonstrated by the complexity of his code. I mean, hey, if I’m writing code that just any programmer can easily understand then I mustn’t be that advanced, right? Besides, it’s their job as engineers to keep up on the latest libraries or standards so they can speak my language anyways. Can’t easily understand my code? Pfft … then you mustn’t be very 1337.

I’m embarrassed that I ever used to think that way. Writing software is hard. So hard, in fact, that I’d wager a majority of programmers are on software projects that will fail. Some of that failure may be out of our direct influence, but we can at least control the complexity of our own work, and make it easy to understand, test, and (gulp) modify by our fellow peers.

The following code may not convince my teammates that I’m particularly smart or gifted …

for (ObserverList::iterator itr = first; itr != last; ++itr)

{

    Observer * observer = *itr;

    observer->LightMoved(light);

}

… but they will at least quickly understand what it does, and perhaps be grateful for that much.

{ 3 comments… read them below or add one }

Shay Pierce January 5, 2011 at 9:05 am

Great article. I think almost every programmer (like the entire Perl community) could stand to be reminded that making yourself look or feel clever is of no value to a project or a team. The ability to do something clever when necessary is fantastic; actually doing something clever usually means.
Of course, it might be a part of programmer psychology that we’re driven by the desire to prove our cleverness – I know I’m guilty of that. I try to remind myself that my definition of “clever code” should really mean “code that does the job well and efficiently AND is readable by any other programmer.” Being able to write code that is so readable that it doesn’t need comments (which happens less rarely than some people would admit) is now my definition of cleverness!

Seanba January 5, 2011 at 8:47 pm

Shay, no doubt about it: in moments of weakness I’m certainly guilty of needing people around me to perceive me as smart. It’s just ironic that such efforts are self-defeating.

I like how code reviews help us keep it real. If I can get another programmer to review my work and s/he understands it with minimal guidance then I’ve won the day.

Cory Reece February 13, 2012 at 6:58 pm

About the code you posted… I understood only one line of code in that, and that would be the ‘using namespace std;’ part. Well, at least it’s something.

Leave a Comment

Previous post:

Next post: