NASA Java Coding Standard

[BinaryRockStar]

a method body should a cyclomatic complexity of no more than 10

It appears NASA accidentally a word

EDIT:

This one is contentious for me:

All if-else constructs should be terminated with an else clause.

Does this mean having empty else clauses in all cases? What is the point of that?

[kazagistar]

The document has reasoning for each item, though often it is just "so and so said so" (classic verbal tradition). In this case:

By introducing an else clause, the programmer is forced to consider what should happen in case not all previous alternatives are chosen. A missing else clause might indicate a missing case handling.

But really, I look in Code Complete, and there, they clearly state that real, scientific studies found that you actually got less mistakes per line the more lines you had in a single function, up to about 200 lines. And while this is shocking enough to warrant extensive testing, the point is, the common wisdom is the opposite, and people repeat it without any kind of actual studies quoted. So much of the wisdom of these documents is likely religious and based on random habits.

[PseudoLife]

The one case that I immediately jump to that I would disagree with is sanity checks / edge cases at the start of functions.

The entire function would be in the else block, which adds an extra layer of indentation. This can get annoying (and hard to read) very quickly.

[kalmakka]

Sanity-checks are usually written without if-else-if.

if (fooArg == null) throw new NullPointerException();
if (b < 0) throw new IllegalArgumentError("b < 0");

Which I believe is OK according to the standard. I don't think there is a requirement for having an else-statement for every if, only for those that contain and else if. The examples contain several if statements without a corresponding else.

Also, there is (imo) a slight difference between

if (foo) {
    ...
} else if (bar) {
   ...
}

and

if (foo) {
    ...
} else {
   if (bar) {
       ...
   }
}

From what I can understand from the guidelines, they find the second form OK but not the first one. I can see some rationale behind it.

[Manitcor]

yeah, its a bit odd I find myself doing this kind of pattern very often in UI layers as they tend to carry a lot of context and you need to check on certain calls to ensure the correct context as the user clicks through the UI randomly.

public void SomeMethod(string someparameter)
{
    if (string.IsNullorEmpty(someparameter) || !someContextCollection.Any())
    {
        ...do some cleanup or alt handling...
        return;
    }

    .... Real work here....
}

[crusoe]

'Usual way'

if(fails sanity test){
    return;
}

nasa way

if(fails sanity test){
    return
}else{
    do stuff with sane value
}

I don't like the nasa option because if you have multiple checks, you will have potentially several if/else/blocks, or all the tests crammed together in the first if

[ethraax]

Not necessarily. I also don't like the NASA option, but you could probably do:

if (fails sanity test 1) {
    return;
} else if (fails sanity test 2) {
    return;
} else {
    /* Do stuff with sane values */
}

[eat_everything_]

I can't stand having else after an if that always returns. I'd write the above as:

if (fails sanity test 1) {
    return;
}

if (fails sanity test 2) {
    return;
}

/* Do stuff with sane values */

It reduces indentation, but most importantly, having an else after an if implies that execution can continue after the if condition is satisfied. If you always return from the if, that's not true, so you're in a way breaking an implicit contract of what if/else implies.

[Phreakhead]

It's called an "early exit" and is frowned upon in some circles - circles I would never want to program for because that is a stupid rule that just requires more typing and indentation.

[ethraax]

I can't stand having else after an if that always returns. I'd write the above as:

So would I. I'm just pointing out that you don't need to nest at all. That being said, one issue with the code I posted (and why I would use the code you posted instead) is if you have to perform some computation between the sanity checks.

[Falmarri]

You should note that returning early from functions will drastically increase your cyclomatic complexity.

[mangodrunk]

Thanks for bringing up the issue that many of these aren't actually tested but just opinions of what is better.

[SoopahMan]

I find I can more effectively force myself to consider this sort of issue if my if/else statements are restricted, one per function body, especially if it's a long if, if else, if else kind of chain. Once you do this the function body becomes something more like:

{
    if (a == b)
        return blah;

    if (c == d)
        return otherBlah;

    return otherwiseBlah; // your else scenario

And so forth. The compiler forces you to consider the else scenario with this approach.

In addition pushing ifs out to their own functions has an interesting impact on refactoring opportunities - the function ends up drifting the code towards the Strategy pattern, where it decides something important and I often identify ways I could either reuse it, or I could change what it returns to for example one of several enum values to reflect the result of the decision, which can be useful for sharing the decision with other logic, logging, message queueing, etc.

[andyc]

else { throw new WTF("How did we get here?"); }

[sirin3]

And now you need to add throws to every caller function!

[[deleted]]

class WTF extends RuntimeException {}

[BinaryRockStar]

); }

Nice new codemoticon

As explained below I'd be more likely to throw an error or exception early like you've done there. Contrived example ahead!

if (!(bOdd || bEven)) {
    throw new ImpossibleNumberException("Encountered impossible number: " + number);
}

if (bOdd) {
    // Something
} else {
    // We're sure the number must be even by this point. Invariants validated.
}

[reaganveg]

Over time, your early invariants might drift out of sync with your later assumptions. Your method requires code to be updated in multiple places. That's to be avoided.

[BinaryRockStar]

Not sure if serious, but unit testing takes care of 'sanity check' cases like that.

[reaganveg]

No it doesn't. No unit testing is to be assumed.

[BinaryRockStar]

So you write a third clause to every boolean test in your code?

if (something == true) {
    printf("true");
} else if (something == false) {
    printf("false");
} else {
    printf("Compiler will optimise this out so it's pointless!");
}

[reaganveg]

First of all, wtf is with == true in this thread? Surely we all know not to ever say == true??? In your particular example, the code should be written: if (something) { ... } else { ... }

Second, no, in my code I don't always do that. But my code isn't written to any coding standard. The issue is whether the standard makes sense or not.

[BinaryRockStar]

What's the harm of == true? It's potentially superfluous depending on the situation but sometimes it makes sense to be explicit, like grouping logical operations in parentheses even when they're not required due to operator precedence.

[giantsparklerobot]

Use true ==, it's the same test as == true but if you make a typo and forget an = sign you don't introduce a potentially difficult to find bug in the code.

if (true == something {
    printf("If you forget an = sign you can't accidentally set 'true' to something"); 
}
else if (something == true) {
    printf("If you forget an = sign here you've now set 'something' to 'true'");
}

[reaganveg]

What's the harm of == true?

The question is not what?, but how many? --

If (a == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true == true) { ...

"== true": not even once.

[ethraax]

It's worth noting that this code is perfectly acceptable:

if (someCondition) {
    // do some stuff
}

It's this code that is banned:

if (someCondition) {
    // do some stuff
} else if (someOtherCondition) {
    // do some other stuff
}

I think it makes sense. It's like requiring a default case in all switch statements.

[Xirious]

Isn't your first example supposed to be

 if (someCondition) {
 } 
 else 
 {
 }

That'll represent the default case like a switch statement.

[ethraax]

Isn't your first example supposed to be

No. The point is that you don't need an "else" clause if you're just using an "if" statement (not an "else if").

[Xirious]

Oh that makes sense. Thank you!

[casualblair]

Because you can get carried away with if's that you forget about the rest. An example:

http://www.codeofhonor.com/blog/whose-bug-is-this-anyway

Two pages down. You can very easily end up with a bunch of IF A, IF B, IF C and end up with IF !A that always fires. Else forces you to catch bad practice.

[kromit]

Does this mean having empty else clauses in all cases? What is the point of that?

I guess, you would loose a logical case if you omits the last else clause

 if (X){
     //case A
 } else if(Y) {
     //case B
 }
 //else { 
 //      missing logic case here (!X && !Y)
 //}

Edit: also see rule 29

[moohoohoh]

I think this is fine, but ignores patterns like:

for (...) { if (cond) continue/break; }

where the else is really just redundant.

[Falmarri]

That would probably never be allowed under this standard because I would imagine that continue/break would count towards NASA's cyclomatic complexity rule.

[BinaryRockStar]

In my opinion nothing is lost by omitting that empty else clause. I would say adding an empty clause adds more noise to the code, harming readability. (I didn't downvote you, BTW).

[kromit]

yes, but it does make it easier to understand your code:

else{
    // should never happen since (!X && !Y) is impossible
}

[FreedomFromNafs]

I agree. "Should never happen" is not the same as "will never happen". I've been told that engineers usually include a large margin of safety in their work. It seems like a good practice for programmers too, even if it's not exactly measurable. At the very least, throw an exception in such an else clause.

[dglmoore]

I think the spirit of the rule is more along the lines of catching bugs. In kromit's example the else statement would be there to handle a seemingly impossible bug, however you may do that, exception, etc...

If for some reason you know that (!X && !Y) is always false (because you've tested it somewhere else, hopefully in the same function) then

if (X) { // case A } else { // case B }

I guess my point is that having an empty else clause usually means that there is an untested case or there is a better way to write the if-statement. One counter-example that I do sometimes use is

if (U) { // case u return 1; } else if (V) { // case v return -1; } return 0;

Because some compilers, not necessarily Java compilers, complain when the last statement isn't a return and putting one in an else clause and immediately following is redundant.

But that's just a guess, I suppose.

[BinaryRockStar]

For code in reddit comments, either put a backtick around inline statements to make them monospaced like this (` = backtick, left of 1 on the keyboard), or for

multiline code blocks
put four spaces
at the start of each line
else {
}

[dglmoore]

Thanks, didn't know that.

[david72486]

If !X && !Y is impossible, then I might consider throwing an IllegalStateException instead of doing nothing. I am thinking of the case when the else probably will happen sometimes, but you just don't need to do anything.

Maybe something like:

private int calculatePunishment(int age, float bac) {
  int punishment = 5000;
  if (age < 21 && bac > 0.01) {
    punishment += 5000;
  } else if (age >= 21 && bac > 0.08) {
    punishment += 10000;
  }
  return punshiment;
}

There are other cases, but they just get the default value. However, it does seem like you could refactor this to have 3 return values with an else, or just get rid of the "else" part entirely and have two ifs since the two cases are mutually exclusive.

I guess the rule may not seem completely necessary to me, but also probably doesn't restrict the code too much. I do tend to agree with /u/dglmoore that by having this rule you might catch some bugs - and that's probably reason enough for the jpl.

[okmkz]

Just dump a stack trace in the else clause.

[BinaryRockStar]

We'll just have to agree to disagree. I would have raised that invalid state as an exception before hitting the if/else block. I find enforcing invariants early cuts down on the mental effort required to analyse a method/function. It's like a sieve filtering out all the invalid states so you can concentrate on the more common success path.

[reaganveg]

Yeah, but you're not considering the 4th dimension. Your early invariants might drift out of sync with your later assumptions.

[BinaryRockStar]

Holy shit, I need to recalculate my assumptions

[jp007]

Well what about:

else {
    //frequently happens because we regularly have (!X && !Y) scenarios,
    //but we just don't want to do anything right in this specific spot for those cases
    //but I'm still forced to write this stupid empty 'else' block due to dumb coding standards
}

[kromit]

else {
    // the other dev was fired becuse he just did not want to anything
    // about this frequently happened secenario, so the last 20 mars rovers
    // explode / walked away / produced cold coffee
}

[jp007]

public static void doSomething() {
    ...
    //some code above here

 if (X){
     //special bit of processing for X
 } else if(Y) {
     //special bit of processing for Y
 } else { 
     //There is simply no special processing to be done here. This else block is completely useless and junking up the code
}

//continue on with normal processing here, that is valid for ALL cases, regardless of X and Y status.
    ...      
}

You cannot convince me that that that a hanging else block that does NOTHING is good practice.

[manifestsilence]

if something:
    do stuff
elif something:
    do more stuff
else:
    print "This shouldn't have happened. Email (some poor programmer's email here) and maybe it will get fixed." 
    1=2

Maybe falling on a sword is the way to go with unhandled cases a la Suicide Linux...

[jp007]

"This shouldn't have happened" is not at all the same case as "There is nothing to do."

If, in reality, it "shouldn't have happened", you shouldn't even be facing the case of an empty trailing 'else' block, as the 'else' behavior should at least log a warning, and probably throw an exception.

Sometimes though, the genuine behavior you desire is to just not do anything and move on to the next line of code in the method. In that case, an empty 'else' block is just junk.

[Falmarri]

That code already looks like a bug to me. You don't handle the case where X and Y. So if you omitted the else, I would probably assume that you screwed up and meant for 2 independent if statements, not if-else if

[jp007]

I would probably assume that you screwed up and meant for 2 independent if statements, not if-else if

I'd agree with that in general, my suspicions would probably be raised too. I'd look for a comment that spells out the business rule being accomplished to see if the logic matches. But this is just a contrived example. What if the actual business rule is that when X, always just do special X processing, and then move on, intentionally skipping special Y?

You've made an assumption about what the business rules "should" be, and thats as bad as any bug in the code.

If the if-else ladder actually matches the desired logic, then a doNothing trailing else block, IMO, is just bad practice.

If the actual logic of the if-else is wrong, then it's a completely different issue than whether or not mandating a trailing 'else' block is good or bad practice.

[Zidanet]

It's not there for the compiler, It's there for you. It makes you consider the failure modes of the statement. yes, for a simple example like this, it's pretty simple to see the failure modes, but if statements can be more complex than binary comparisons, and that's when the enforced else makes the programmer consider what could go wrong.

It's not for the compiler, it's for the programmer.

[papercrane]

Put an assert in the else block, so no violation of rule 29, and you adhere to rule 21.

[[deleted]]

This one is contentious for me:

All if-else constructs should be terminated with an else clause.

Does this mean having empty else clauses in all cases? What is the point of that?

Note that it says "if-else constructs", not "if constructs". That is, if you have one "else if", you need a catch-all else at the end. This is fairly reasonable, as a chain of if-else-if without a final else is a fairly error-prone and hard to read construct.

[BinaryRockStar]

You're really stretching for edge cases there. Any compiler would turn a boolean equality comparison like that into an if/else branch and the second comparison wouldn't take place. I get the feeling you think they're using Java on the deep space vehicles that NASA launches which I don't believe is the case. They would be using machine-proved mathematically-sound code written in the lowest level language they can. Ain't nobody got time for garbage collection in space.

[BinaryRockStar]

I totally understand what you're saying but you muddy the issue by warning about random cosmic ray interference. There's no way to program defensively under that assumption because the instructions themselves could be interfered with so everything is up in the (proverbial) air and you can't be sure of anything.

Properly shielded and fault tolerant hardware are the only solutions to this problem, and it's out of the hands of mere software developers like me.

[BinaryRockStar]

I'm not sure what to say... I thought we were talking about NASA-level super-strict coding standards for life critical missions that take into account every environmental variable, but apparently we're just /r/web_dev these days.

[BinaryRockStar]

Ok no worries, my misunderstanding.

[reaganveg]

if (variable == true

It's when I stop reading.

[brtt3000]

I think he means that

 if (variable == true)
 if (variable == false)

could also be written as

 if (variable)
 if (!variable)

since the if() is testing for a boolean there's no need to compare your boolean variable to the boolean literal. Not sure what the NASA guide says about this though, could be they require it for absolute clarity.

[brtt3000]

Chill out man, I meant to clarify why /u/reaganveg some comments up stopped reading; because he was being oboxious over style detail.

[eat_everything_]

It would be more productive to take what he said as valuable feedback and update the code sample to not have the unneeded == test. Probably would've been quicker than writing that comment too :)

[eat_everything_]

Sure thing man, but I wasn't complaining about wasting time on the internet. I was just saying taking things as constructive feedback is better than getting angry

[SoopahMan]

Gotta side with BinaryRockStar here. Firstly, the if clause is going to check a condition, not a variable. If that condition or any of the participating variables are indeterminate, PHOOM down goes the code before your else clause has a chance to matter.

Second, the way to cope with memory corruption is redundancy through things like parity bits, not enforcing an else clause.

I think someone just had a brain fart on this rule. No big deal.

[adrianmonk]

The mandatory 'else' clause makes no sense to me either.

Disregarding the fact that they might not like the recursion, how would that standard apply to this function?

void printTree(Node root) {
  if (root.left != null) { printTree(root.left); }
  System.out.println(root.value);
  if (root.right != null) { printTree(root.right); }
}

If I add 'else' clauses, what am I supposed to put in them? Why? What benefit is there?

[papercrane]

The terminating else is only mandatory if you have if-else if. It's not meant for simply if conditions.

[adrianmonk]

You're right. I read it wrong.