IF statement formatting best-practise, what's your style?

Question

Hi,

Looking to improve my IF statement, and I want to keep my code looking pretty

This is what I am currently doing, is it readable, any room for improvement?

SomeObject o = LoadSomeObject();


if( null == o
    ||
    null == o.ID || null == o.Title
    ||
    0 == o.ID.Length || 0 == o.Title.Length
 )

I don't have anyone to ask around. That's why I came here in first place. Please don't close my question :(

Answer 1

Your verbosity is leading to a less readable code, I think the following format is best:

if ( null == o || null == o.ID || null.Title || 0 == o.ID.Length || 0 == o.Title.Length )
{
  // do stuff
}

We all have high resolution/widescreen displays for a reason, there's no reason to lock your code at some horribly short syntax. Also, I would simply create a function named IsIDEmpty so that the code could look like

if ( IsIDEmpty(o) )
{
  // do stuff
}

to keep the code simpler & cleaner. The function would perform the actual checks and return a boolean. I'm sure this is something you might have re-use for anyways, plus it serves as a simple way for code to be more self documenting/commenting.

Answer 2

I would either put it all on one line if it fits (which this one clearly doesn't). With this, I would put the || consistently at the start or end of line:

if( null == o ||
    null == o.ID ||
    null == o.Title ||
    0 == o.ID.Length ||
    0 == o.Title.Length
)

or

if( null == o
    || null == o.ID
    || null == o.Title
    || 0 == o.ID.Length
    || 0 == o.Title.Length
)

You could have >1 condition on a line, the positioning of || is more important I think.

I'm ignoring the fact that null.Title doesn't seem to make much sense

Answer 3

I find it quite distracting, to be honest. Mostly because the '||' start making funny patters.

I much rather prefer something like

if ( o == null || o.ID == null || null.Title ||
     o.ID.Length == 0 || o.Title.Length )

or even better, keep it in a single line if possible.

Answer 4

I always try and avoid complex boolean expressions for the sake of the next guy, but if I had to write an expression that didn't easily go on one line I would format it as follows:

if (value1 == value2 ||
    value3 == value4 ||
    value5 == value6 ||
    value7 == value8) {

  executeMyCode();
}

Answer 5

I usually do something like:

if(x < 0 || x >= width
|| y < 0 || y >= height)
{
    /* Coordinate out of range ... */
}

The first y and x line up in a monospace font, which is nice, and I'm not confused by half-indentions.

This method works best when doing similar comparisons. Otherwise, I usually split up my if's.

Answer 6

I think it's pretty unreadable.

The affection of putting the constant first always seems a bit odd to me - most compilers can be persuaded to warn if they find an assignment in a conditional.

Then you're testing for null for two different things, then for zero length for two different things - but the important thing is not the length check, but the member you're checking. So I'd write it as

if (o == null       ||
    o.ID == null    || o.ID.length == 0 ||
    o.Title == null || o.Title.Length == 0)

Answer 7

For the simplest formatting of what you have, I would go with one per line.

if(null == o
  || null == o.ID
  || null == o.Title
  || 0 == o.ID.Length
  || 0 == o.Title.Length)

Even better would be if you could refactor the condition such that it fits on one line. I find that a large number of || or && is usually difficult to read. Perhaps you can refactor it out into a function and be left with:

if(myFunction(...))

Answer 8

It is not readable. This is how I do Really Long Ifs(or those I have to twiddle a lot).

if(
  o == null  ||
  o.ID == null || 
  o.Title == null ||
  o.ID.Length == 0 || 
  o.Title.Length == 0
 )

For yours, I would do a single line.

if(o == null  || o.ID == null || o.Title == null || o.ID.Length == 0 ||  o.Title.Length == 0)

Or, if you are using C++, I'd do something like this:

if(!o)
{}
if(! (o.ID && o.Title && o.Length))
{}

...since it separates creation from correctness.

However, caveat emptor, I've been accused of bloated LOC due to my fondness for newlines.

Answer 9

Generally I'm with TravisO on this, but if there are so many conditions in your if() statement that it just gets crazy long, consider putting in its own little function instead:

bool wereTheConditionsMet()
{
  if( NULL == 0 )
    return true;
  if( NULL == o.ID )
    return true;
  :  :   // and so on until you exhaust all the affirmatives
  return false;
}

if ( wereTheConditionsMet() )
{
  // do stuff
}

It is a lot easier to convey the intent of a well-named predicate function than an endless string of ||s and &&s.

Answer 10

My rule of thumb: Avoid any format that a semi-smart auto-formatter can't reproduce.

Having a defined set of formats and a automated tool/template/configuration that actually produces code in that format is a big plus, in my opinion.

And if your code is still unreadable after it has been auto-formatted, then chances are that you need to refactor anyway.

Answer 11

Rather than making up a standard for just this one question - I would suggest adopting an existing coding standard for whatever language you are using.

For example:

GNU Coding Standards
http://www.gnu.org/prep/standards/

Code Conventions for the Java Programming Language
http://java.sun.com/docs/codeconv/

.NET Framework General Reference Design Guidelines for Class Library Developers
http://msdn.microsoft.com/en-us/library/czefa0ke.aspx

Answer 12

1. Use an automated code formatter, and configure it appropriately.
2. Write a method like isPresent(String) that checks the String argument for not null and for not empty (zero length).
3. Rewrite the original conditional to use the new isPresent(String) method, probably all on one line.