views:

870

answers:

8

I really enjoyed Jeff's post on Spartan Programming. I agree that code like that is a joy to read. Unfortunately, I'm not so sure it would necessarily be a joy to work with.

For years I have read about and adhered to the "one-expression-per-line" practice. I have fought the good fight and held my ground when many programming books countered this advice with example code like:

while (bytes = read(...))
{
   ...
}

while (GetMessage(...))
{
   ...
}

Recently, I've advocated one expression per line for more practical reasons - debugging and production support. Getting a log file from production that claims a NullPointer exception at "line 65" which reads:

ObjectA a = getTheUser(session.getState().getAccount().getAccountNumber());

is frustrating and entirely avoidable. Short of grabbing an expert with the code that can choose the "most likely" object that was null ... this is a real practical pain.

One expression per line also helps out quite a bit while stepping through code. I practice this with the assumption that most modern compilers can optimize away all the superfluous temp objects I've just created ...

I try to be neat - but cluttering my code with explicit objects sure feels laborious at times. It does not generally make the code easier to browse - but it really has come in handy when tracing things down in production or stepping through my or someone else's code.

What style do you advocate and can you rationalize it in a practical sense?

+4  A: 

One expression per line.

There is no reason to obfuscate your code. The extra time you take typing the few extra terms, you save in debug time.

jjnguy
+5  A: 

I think the ideal solution is to find a balance between the extremes. There is no way to write a rule that will fit in all situations; it comes with experience. Declaring each intermediate variable on its own line will make reading the code more difficult, which will also contribute to the difficulty in maintenance. By the same token, debugging is much more difficult if you inline the intermediate values.

The 'sweet spot' is somewhere in the middle.

pkaeding
+3  A: 

I tend to err on the side of readability, not necessarily debuggability. The examples you gave should definitely be avoided, but I feel that judicious use of multiple expressions can make the code more concise and comprehensible.

amrox
+6  A: 

In The Pragmatic Programmer Hunt and Thomas talk about a study they term the Law of Demeter and it focuses on the coupling of functions to modules other than there own. By allowing a function to never reach a 3rd level in it's coupling you significantly reduce the number of errors and increase the maintainability of the code.

So:

ObjectA a = getTheUser(session.getState().getAccount().getAccountNumber());

Is close to a felony because we are 4 objects down the rat hole. That means to change something in one of those objects I have to know that you called this whole stack right here in this very method. What a pain.

Better:

Account.getUser();

Note this runs counter to the expressive forms of programming that are now really popular with mocking software. The trade off there is that you have a tightly coupled interface anyway, and the expressive syntax just makes it easier to use.

Dan Blair
The fluent APIs you're talking about generally don't walk down a tree of stateful objects (like your first example does), rather, they return the same object or new internal objects encapsulating state. So while they violate the letter of the LoD, they don't violate its spirit.
kyoryu
@Kyoryu, I agree with your statement, but the question was focused on 1 statement per line. The fluent API's still make it a little harder to debug because you are executing multiple operations on a single line.
Dan Blair
@Dan - I get your point entirely. Good fluent interfaces use the 'multiple statements' as simply a better way to achieve a single logical statement, rather than actually being multiple, atomic calls. For your example, I'd even argue that Account.GetUser() isn't a particularly good example, as one would typically then act on the user. I'd rather see Account.ActualOperation(). (still upvoted as I think we agree in general, we're quibbling on the 2%)
kyoryu
+1  A: 

I'm usually in the "shorter is better" camp. Your example is good:

ObjectA a = getTheUser(session.getState().getAccount().getAccountNumber());

I would cringe if I saw that over four lines instead of one--I don't think it'd make it easier to read or understand. The way you presented it here, it's clear that you're digging for a single object. This isn't better:

obja State = session.getState();
objb Account = State.getAccount();
objc AccountNumber = Account.getAccountNumber();
ObjectA a = getTheUser(AccountNumber);

This is a compromise:

objb Account = session.getState().getAccount();
ObjectA a = getTheUser(Account.getAccountNumber());

but I still prefer the single line expression. Here's an anecdotal reason: it's difficult for me to reread and error-check the 4-liner right now for dumb typos; the single line doesn't have this problem because there are simply fewer characters.

Michael Haren
But if a NullPointerException is raised with ObjectA a = getTheUser(session.getState().getAccount().getAccountNumber()); which object in the train is null? Session, State, Account, AccountNumber or the User being returned from the original funciton?
iAn
+1  A: 
FlySwat
A: 

Maintainability, and with it, readability, is king. Luckily, shorter very often means more readable.

Here are a few tips I enjoy using to slice and dice code:

  • Variable names: how would you describe this variable to someone else on your team? You would not say "the numberOfLinesSoFar integer". You would say "numLines" or something similar - comprehensible and short. Don't pretend like the maintainer doesn't know the code at all, but make sure you yourself could figure out what the variable is, even if you forgot your own act of writing it. Yes, this is kind of obvious, but it's worth more effort than I see many coders put into it, so I list it first.
  • Control flow: Avoid lots of closing clauses at once (a series of }'s in C++). Usually when you see this, there's a way to avoid it. A common case is something like

:

if (things_are_ok) {
  // Do a lot of stuff.
  return true;
} else {
  ExpressDismay(error_str);
  return false;
}

can be replaced by

if (!things_are_ok) return ExpressDismay(error_str);
// Do a lot of stuff.
return true;

if we can get ExpressDismay (or a wrapper thereof) to return false.

Another case is:

  • Loop iterations: the more standard, the better. For shorter loops, it's good to use one-character iterators when the variable is never used except as an index into a single object.

The particular case I would argue here is against the "right" way to use an STL container:

for (vector<string>::iterator a_str = my_vec.begin(); a_str != my_vec.end(); ++a_str)

is a lot wordier, and requires overloaded pointer operators *a_str or a_str->size() in the loop. For containers that have fast random access, the following is a lot easier to read:

for (int i = 0; i < my_vec.size(); ++i)

with references to my_vec[i] in the loop body, which won't confuse anyone.

Finally, I often see coders take pride in their line number counts. But it's not the line numbers that count! I'm not sure of the best way to implement this, but if you have any influence over your coding culture, I'd try to shift the reward toward those with compact classes :)

Tyler
A: 

Good explanation. I think this is version of the general Divide and Conquer mentality.

epatel