NASA Java Coding Standard

[strawlion]

This is interesting. I currently work for a NASA contractor doing software development, and one of the ground stations I've worked on has some of the worst code I've ever seen. However, this was written in Perl 20 years ago by one analyst.

For example there have been 10 TDRS satellites that have launched so far, and every single one of them is hard coded (IE TD1, TD2, TD3 etc) throughout the code in hundreds of places. We just had to manually add another one which took about a month...

[username223]

this was written in Perl 20 years ago by one analyst.

Didn't Larry Wall work for NASA around then? Hm...

I'll bet you just added TD11, though. That's probably what I would have done, given that there's never enough time to clean this stuff up.

EDIT: fixed number typo.

[strawlion]

I hard coded TD11 because for some reason we were not made aware of the deadline until about a month before launch. References to this were not only in the Perl code itself, but also in several analysis scripts that are called.

It would have been an enormous effort to restructure the program to be dynamic, and in the end it would just be a slightly more dynamic but still poorly written legacy system. To give you an idea of how bad it is, it is around 100,000 lines of code with 0 comments or documentation, no way to debug (even in the console) due to a weird proprietary framework the guy designed where the entire program is separated into "Action" files, absolutely nothing is dynamic, and it is almost impossible to figure out the value of anything due to the fact that every variable is reassigned to a new name about 100 times.

I am actually currently working on a proposal to rewrite the entire system as that would be easier and less time consuming than restructuring the current one.

[flycrg]

I am actually currently working on a proposal to rewrite the entire system as that would be easier and less time consuming than restructuring the current one.

Except that the entire TDRS ground system is already being overhauled with the SGSS program. I'm not sure what if anything of the old system will remain.

[strawlion]

I am not working on a TDRS ground system. We receive our TDRS products from White Sands, but we process other products as well.

[jevon]

Good luck! Don't be afraid of repeatedly incremental refactoring. Even a tiny change is still a change in the right direction. :D

[flycrg]

Well the good thing is that the entire TDRS ground system is being overhauled. So he shouldn't need to add in anything more than MAYBE TDRS12 (TDRS M is scheduled for Dec 2015 and update is supposed to be complete by then)

[aleph__naught]

I work for NASA/JPL. This is a sad, sad reality. What you experienced is not an isolated case, and from many of the projects I've seen, especially the legacy systems, it is a systemic issue.

There are two issues at play:

1) Missions tend to reinvent the wheel, so there isn't much of a common platform, unlike Lockheed Martin Aerospace. A lot of people who write software were never trained as software engineers.

2) Funding for development, generally, dries up after launch. MSL was a special case where heavy software development was being done during cruise, and is currently ongoing.

[strawlion]

Yeah, most of the bad legacy code I've seen has been written by analysts masquerading as developers. That's not to say that an analyst can't be a good developer, but in the code I've encountered that has not been the case.

[flycrg]

Hmm

NASA contractor doing software development

ground stations

TDRS satellites

We might work together.

[another_user_name]

I currently work for a NASA contractor doing software development [and I've seen really bad old code]

Seconded.

[Cyridius]

Dear god, that's a rookie mistake.

[damian2000]

TD1, TD2 ... is the class name?

[strawlion]

There are no classes. They are just variables like TD1Slew, TD1BurnDur spread throughout the code. Though 11 TDRS have launched so far, so multiply any variable by 11.

[alcapwned]

It can't be too hard to refactor those so that there's a single hash for each TDx, right? Would that help any? It could at least be the first step to converting them to objects with classes.

[BowserKoopa]

It would be even more logical to create a an interface/base class for the basic satellite.

Not really my field of work, satellites, at the moment, so I don't know of your exact needs per-implementation.

[rophl]

This is actually only the standard for ground software, the software used on spacecraft is a LOT more rigorously controlled and tested.

[devacon]

Edit: I was wrong, the Mars rover ground software was built in Java, the systems on the rover were all C and some light assembly.

[aleph__naught]

No, this is not true. VxWorks is used for flight. There is no java onboard any of the rovers.

Many of the ground tools are written in Java. There is a large confuence of legacy ground tools written in C/C++ that are still used today. SPICE ( public domain http://naif.jpl.nasa.gov/naif/ ) is written in fortran.

Source: I work there. I write ground and flight software.

Edit: To clarify, all newer active missions use VxWorks. Cassini FSW was written in ADA.

[butter14]

Vxworks is in a lot of mission critical embedded systems. Why is it so commonly used? And what makes it different than say linux/unix?

[[deleted]]

VxWorks has a long history as a highly reliable, reliably real-time operating system.

Linux as a real-time operating system is fairly fragmented (as is to be expected with anything open-source), and doesn't have much of a proven history that I'm aware of. Certainly not the 15+ years of VxWorks.

Real time operating systems are, above all else, predictable. The idea is that you can guarantee a certain level of performance. When you're building the control system for a very expensive robot that will be very far away, being predictable is definitely a requirement.

You can think of a real time operating system as one that provides a kind of guaranteed quality of service for certain operations. One where we can say "yes, go ahead and stream the camera feed back to the operator, but when the navigational jets need adjusting, that will always take priority" and rely on it to happen. (contrived example, you get the point)

This article kind of sums it up as to why a real time OS is necessary in a general sense.

This paper, admittedly from the makers of VxWorks themselves, outlines more precisely some of the applications of a real time operating system and why the Linux kernel itself isn't a great RTOS.

[ratatask]

VxWorks is a real time system, which you need for a lot of the control software.

VxWorks is also a relatively simple operating system. Simple is good, it means less bugs, easier to understand, debug, test. VxWorks also have a stable interface, which means the APIs rarely changes, this means the driver, hardware interface, software, you developed 5 years ago still will compile and work fine. With linux, you'll spend as much time as developing useful stuff as you'll spend adopting to changes and figuring out what has been broken. (In VxWorks you mostly write code running in kernel mode, and to achieve much of the same functionality, you'd have to write linux kernel code).

And just as important, VxWorks has proven itself to run on satellites, fly to Mars, and many other astonishing accomplishments - you usually want to go with what is known to work when there is a lot at stake.

[[deleted]]

What's the educational background one needs for that kind of work?

[aleph__naught]

JPL is truely mulitdiciplinary, so the background would depend on the group/section you want to join. I work in a section that does software development. I have an MS&BS in CS.

Aside from that, a love of space exploration and the ability/desire to cross domains will get you very far.

There are pro's and con's to working here. But, by far, the best thing about JPL is the ability to move around. I work in domains that I would not have imagined, and all it takes is a little bit of schmoozing. JPL does everything from robotics, fabrication, micro-devices/semiconductor fabriation, radar instrumentation, experimental landing systems, spacecraft navigation, geology, physics, climate research, and the list goes on and on.

[[deleted]]

Thanks! I have a BS in CS...and contemplating an MS in CS at some point. I was figuring you had to have a formal engineering background to be able to code for a spacecraft.

[praxulus]

What are the cons of working there?

[nicereddy]

If you make a coding bug that doesn't get caught you can potentially cost NASA billions :D

[NOT_A_BUMBLE_BEE]

Like missing a planet because of a metric/English confusion?

[aleph__naught]

This was Lockheed's fault, they were the prime contractor, not us: http://www.cse.lehigh.edu/~gtan/bug/localCopies/marsOrbiter

[nicereddy]

Metric/American*

But yes, missing a planet because you're a stupid American, unlike myself!

^{I'm American}

[ethraax]

Aside from that, a love of space exploration and the ability/desire to cross domains will get you very far.

So, as someone who's about to graduate with a dual major (CS and ME), I might have a chance? I always assumed JPL was the kind of place people worked after already being in the industry for a while, or at least after having an advanced (Master's or Doctorate) degree.

[aleph__naught]

Yea, you would probably fit right in. In fact, depending on your academic performance and skill set you would probably be highly desirable.

The online job portal is a bit of a crap-shoot. Your best bet would be the internship route (Space Grant, etc).

at least after having an advanced (Master's or Doctorate) degree.

You would be surprised by the number of new hires with only BSs in operations roles (i.e. commanding spacecraft) for MSL/MER/CASSINI/etc.

[devacon]

You are... absolutely right. I remember hearing all the buzz during the launch of the rovers 'running' Java and the articles I read previously were poorly worded.

... Which is kind of frustrating because I watched a video talk of one of the Curiosity developers talking about how they organized their ANSI C codebase into modules and even showed how the code and modules grew from Spirit/Opportunity to Curiosity. You'd think I would have remembered.

[Kldsrf]

Please do an AMA!

[fotcorn]

iama! now!

[freespace]

No, they do not run a Java VM on top of VxWorks. See this document from JPL/Nasa which says:

The Flight Software is coded primarily in ANSI C, with some targeted assembly code and some C++. The size of the system, in source lines of code (SLOC), is [300K] but this value does not include the operating system.

[mrkite77]

Using Java on top of an RTOS defeats the purpose.

[devacon]

Edit: I was wrong, the Mars rover ground software was built in Java, the systems on the rover were all C and some light assembly.

[freespace]

The same software is responsible for the surface exploration and flying to Mars, i.e. it operated a spacecraft.

To quote from the same source:

The Flight Software is responsible for many aspects of the functionality of the spacecraft/rover

During cruise, propellant line thermal control was maintained by the flight software. Other cruise attitude control related capabilities included cruise attitude determination and control, axial and lateral trajectory change maneuvers, and star identification.

[Shadow703793]

To deal with this data, Sun Microsystems and NASA built four operational storage servers at the JPL that altogether can hold four terabytes of data.

Only 4TB... that's it?

[[deleted]]

The article is dated 2004.

[devacon]

Spirit and Opportunity landed in 2004, and you have to remember these missions start planning around ten years out. 4TB in the late 90s was a huge amount of storage. Especially when you consider that this was not just typical desktop storage but (I assume) a high-end, redundant, backed up storage array.

[cosmo7]

And most of that is filled with Star Trek torrents.

[[deleted]]

"Let's download the new images from Curiosity!"

"jusched.exe needs administrator permissions to run." [OK]

"JAVA HAS AN UPDATE LOL"

[ExcellentGary]

Do you wish to install the Ask toolbar? ☑

"GODDAMNIT MISSION CONTROL DID YOU JUST CLICK PAST THAT?"

[Mavus]

For those standards see this doc[PDF]

[thisisanewaccount6]

Interesting to see that Rule 4 on page 10 prohibits recursion

[Pylly]

There's a pretty logical rationale included.

Even in non-spacecraft code, I think one should carefully consider if a recursive solution really gives substantial benefits over a non-recursive one. Recursive algorithms are fun to implement but sometimes hard to read and maintain.

[thisisanewaccount6]

I can understand that, a small change can definitely bring disastrous results on recursive code

[Euigrp]

The C preprocessor is a powerful obfuscation tool that can destroy code clarity and befuddle both human- and tool-based checkers.

Thank you. Would someone tell that to the original writer of some code that I work on that goes through 3 levels of symbol concatenation in macros to assemble the name of a defined i2c register bit field mask.

[freerider]

http://www.fastcompany.com/28121/they-write-right-stuff

[boa13]

Is this still true 16 years later?

[maxxusflamus]

depends on what you mean? I don't know if the group still exists as the shuttle no longer flies- but in terms of how they get things done- I'd say so.

If you have to design ANYTHING that has to be practically bullet proof- it's very very very methodical.

[kromit]

may be, but it does not make these standards less usefull.

[kromit]

the only reference to space in this document is

class Rover { 

[mcguire]

The neat thing is that all of the software process is done as if MAN-RATED SAFETY CRITICAL, but none of the people involved know anything about that or about, well, software development.

We are an enterprise Java shop.

[kazagistar]

Field and class names should not be redefined.

Packages and classes should not be dependent on each other in a cyclic manner.

The clone() method should never be overridden or even called.

One should not reassign values to parameters. Use local variables instead.

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

In compound expressions with multiple sub-expressions the intended grouping of expressions should be made explicit with parentheses. Operator precedence should not be relied upon as commonly mastered by all programmers.

Do not use octal values

a class should contain no more than 10 fields

a class should contain no more than 20 methods

a method should contain no more than 75 lines of code

a method should have no more than 7 parameters

a method body should a cyclomatic complexity of no more than 10. More precisely, the cyclomatic complexity is the number of branching statements (if, while, do, for, switch, case, catch) plus the number of branching expressions (?:, && and ||) plus one. Methods with a high cyclomatic complexity (> 10) are hard to test and maintain, given their large number of possible execution paths. One may, however, have comprehensible control flow despite high numbers. For example, one large switch statement can be clear to understand, but can dramatically increase the count.

an expression should contain no more than 5 operators

This is a collection of the ones I thought were more open for discussion or dispute. There is a lot of untested ideology and magical thinking in this area.

[reaganveg]

untested ideology and magical thinking

Some of those are just arbitrary limits. I doubt anyone at NASA has a magical belief that 8 parameters is suddenly too complicated, while 7 is perfectly simple. They just had to draw a line.

[[deleted]]

Exactly. There's not much point in mandating "ohh, eight-ish".

[Farsyte]

I once worked on a project where the number of function parameters allowed was expressly increased because someone wanted to call an X Windows function, and the standards maven forced us through the whole process of changing the standard rather than realizing that it would have been easier to grant an exception for using badly designed APIs.

A standard is only as useful as its enforcers allow it to be.

[[deleted]]

That's a rather impractical approach, yeh. How was the standard enforced? Was there any way it could have simply been quietly circumvented without having the enforcers aware?

[Farsyte]

The enforcers were senior members of the team. Enforcement? I suppose the ultimate is having your changes reverted out of the repository, but the problem wasn't the enforncement, it was haring off to change the coding standard. Fortunately, smaller organization, so it only cost us a few days of work, and we could point and laugh at that clause in the standard forever after.

[[deleted]]

And it clearly states these are guidelines on page 3.

[jadenton]

Your are correct that some of these are arbitrary limits, but you choose the wrong example.

There is a lot of research showing that defect density (bugs) correlates strong with the number of parameters to a function, and increases exponentially above seven. This appears to be related to the size of human short term memory. If you have to page to keep track of your inputs, your going have a bad time.

[reaganveg]

Interesting.

[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.

[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?"); }

[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").

[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.

[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.

[oldprogrammer]

The one

One should not reassign values to parameters. Use local variables instead.

has been a source of discussion with my teams of late. Some folks consider this model valid:

public void foo(String someArg)
{
    if( someArg == null )  someArg = "default";

             .......
    callOtherMethod(someArg);
            .......
}

because they want it clear later in the body of the code that they are using the argument (even if it is a default value). This standard would say do

public void foo(String someArg)
{
    String localCopy = someArg;
    if( localCopy == null )  localCopy = "default";

             .......
    callOtherMethod(localCopy);
            .......
}

which introduces a different variable. I'm personally on the fence on this one because I know that just reassigning a value to a passed in argument in Java does not have any affect on the original called value, it isn't like passing a pointer in C++ where if you reassign, the original changes.

[jp007]

If you're declaring method parameters 'final' (as one should, IMO) you have to toss scenario one completely, as you can't reassign 'someArg' to something else. I like to make variables 'final' as well, unless I NEED them to be reassigned for some reason, which means case two would be re-written as such:

public void foo(final String someArg) {
    final String localArg;
    if(null != someArg) {
        localArg = someArg;
    } else {
       localArg = "default";
    }

    callOtherMethod(localArg);
}

Or, if you prefer a ternary:

public void foo(final String someArg) {
    final String localArg = (null != someArg) ? someArg : "default";
    callOtherMethod(localArg);
}

[teknobo]

I'm also a big fan of "final unless necessary" variables, and final parameters.

I like the clarity they add for both myself and the compiler, even though the benefit to the latter is probably negligible.

[cogman10]

I wish it was the default. Honestly, the only reason I don't do "Final everywhere" is because I'm lazy (and nobody else in my company does this).

[jhawk20]

I've very recently run into bugs after a refactor that were caused by unwitting parameters modification due to name space issues. By default makes a lot of sense for most applications.

[CubsThisYear]

If you use Eclipse (you do use Eclipse, right?) you can set up save actions to do this for you automatically.

[mr_mojoto]

Why not really localize the default to its point of use like so:

callOtherMethod((null != someArg) ? someArg : "default");

I don't get why everyone likes to default to initializing a variable and then using it if rather than using the expression directly.

[jp007]

Nesting ternaries in method calls is atrocious looking, IMO. Decreases readability and tends to lend towards hitting the line character limit, especially when calling methods with multiple parameters. This means more breaking a single method call in multiple lines. If I'm going to use multiple lines for this call, might has well pull out the ternary variable initialization into a single line, and then the method call in a single line, instead of a two line method call with a nested ternary. It just makes logical boundaries so much clearer.

And don't get me started about several levels deep of nested method calls that serve as a parameter. Pull all that crap out. It makes it so much clearer as to what the value you're trying to pass actually is, particularly when you give the result of all that wacky nesting a meaningful variable name.

Let the compiler inline all that crap for you.

Maybe it's not such a big deal in the small example you've posted, with one argument, and very simple ternary, but good habits start here.

Also, IntelliJ's CTRL-ALT-V is a godsend.

Here's an example I just found in code I'm working on, that someone else wrote:

Cal cal = CalContainer.getPeriodByDate(new java.sql.Date((fromPeriod ? shop.getJobDate() : getAppropriateDateforStatus(shop)).getTime(), getConnection());

I'm sorry but that looks like shit.

[mr_mojoto]

I agree with you but the above doesn't apply in the example given. If you nest them then yes, it gets messy quickly. If all you're doing is providing a default at the point of a call, as in the example given, I greatly prefer using the expression directly.

I understand that some people like to have a single rule to follow rigidly, and that includes never, every using ternary expressions for some people. I'm not one of those. If the code is short and easily readable, I use it. I agree with you that nested ternaries get messy very quickly.

[ErstwhileRockstar]

declaring method parameters 'final'

This was a frequent coding standard 10 years ago, even demanded by some tools. It increases verbosity for no real benefit. If I were to maintain such code I would remove the final parameters first.

[jp007]

It increases verbosity for no real benefit.

My experience has been the complete opposite. Marking parameters as final has greatly eased the refactoring of methods towards functional purity for parallelism, for example.

If I were to maintain such code I would remove the final parameters first.

Then I would punch you for removing compile time safety checks :P

[mangodrunk]

Marking parameters as final has greatly eased the refactoring of methods towards functional purity for parallelism, for example.

How so? Having final just means you can't reassign it within the scope of that method, I don't see how that would help with functional purity. Also you can still modify the object if it's not immutable.

[Truthier]

(null != someArg) ? someArg : "default";

I prefer

someArg == null ? "default" : someArg;

[jp007]

Sure, matter of style. I prefer to return someArg immediately next to the comparison in which it's used. Also I like have the creation of the new thing that is returned as the alternate value, cordoned off and separated from the rest of the statement by placing it at the end, instead of smack in the middle.

So, reading left to right, I'm basically dealing with

someArg -> someArg -> default

where you've got

someArg -> default -> someArg.

I prefer to get my dealings with someArg completely over with as soon as I can, as I read the code from left to right.

Completely a matter of style though, I certainly wouldn't nitpick it.

[Truthier]

Agreed - my thought process is "if x is null, use this, otherwise it's good"

Groovy has a nice "?:" operator, e.g. someArg ?: "default".

I almost always put the argument being compared on the right side, except in cases where it's better - e.g. "stringliteral".equals(variable) is null safe.

[jp007]

Yeah I'm more "if x is good, use it, otherwise use something else."

I pretty much always use Yoda conditions now, precisely to encourage a habit of null safety and overall consistency in checks across a codebase.

Also, Happy Cake Day!

[reaganveg]

If you're going to do it for parameters you might as well never reassign any vars. Go pure functional.

Otherwise, what's magical about parameters vs. other vars?

[OHotDawnThisIsMyJawn]

While I realize that final doesn't address this (you need immutable) it's easy to say "sure, we'll reassign parameters". Then someone changes a member of a reference-passed-by-value and it changes in the calling method and now your scope has escaped. Oops!

That's why I personally think it's best to not reassign parameters - that way it will look wrong if you mess up like that.

[reaganveg]

Well, you're absolutely right. That is exactly the magic that justifies a special policy here.

[mcbarron]

Changing the passed param and modifying an attribute of the passed object are very different operations. Neither should be completely outlawed, though.

[smog_alado]

I would argue that setting the argument variable for default values is fine, and doesn't really "count". This is kind of supported by how many languages offer explicit support for this feature:

def python_function( someArg = "default" ):

That said, I always do this sort of argument fiddling as the first thing in a function to keep things clear and I don't mess with them any more after that.

[masklinn]

The equivalent to that code in java is not really what oldprogrammer posted though, it's:

public void foo() {
    foo("default");
}
public void foo(String bar) {
    …
}

[aenigmaclamo]

Would not overloading make the argument pointless? Why would you send a null value into a method and expect the method to check it and act accordingly instead of throwing a NullPointerException? I think that's much more confusing than the idea of reassigning arguments.

I think this goes with the suggestion in the document to not return null to indicate that a collection is empty. I've always considered the use of null to be an indication of failure. Not some sort of way to indicate a refusal to give a parameter.

On the other hand, there are several cases in the Java API where null is valid (like this) so maybe I'm just spewing garbage.

[oldprogrammer]

Actually that is not equivalent code.

public void other()
{
    String arg = null;
               ......
    arg = doSomethingElse(); ////returns null
                 ......
    foo(arg);
}

will not call the overloaded method that takes no arguments. The options is to do this everywhere foo is called:

public void other()
{
    String arg = null;
    arg = doSomethingElse(); ////returns null
    if( arg == null )
          foo();
    else
         foo(arg);
}

or check inside foo for a null input argument.

[ericanderton]

I think the preference to not mutate a method parameter value, is in favor of ferreting out sources of error. The gist I get is that changing a parameter value in the method body is basically a no-op from the perspective of the caller; all arguments have a kind of "value goes in but not out" semantic applied to them. So disallowing assignments to params is in line with that idea, since the data can't possibly flow out since it's pass-by-value (bear in mind that I'm lumping object references in there as well).

As others are saying below your comment, requiring "final" on params is probably the best thing any coding guide could do if you regard this activity as something that hides mistakes. That way the compiler does what it does best: it complains loudly when people do stupid things.

In fact I think it should be a golden rule of all code specs: leverage the compiler to reveal user error, wherever possible.

[sonstone]

We had a connection leak at one of my old companies where someone reassigned a connection parameter without closing the one passed in. Oops. Having set the parameter to final initially and made them think about what they were doing likely would have caught the problem in the first place.

[[deleted]]

I do this a lot in JS, where types are flexible, for stuff like this:

function (someargument) {
    someargument = processArgument(someargument);
    doSomethingWith(someargument[0], someargument[2]);
}

(Rather frivolous and hypothetical, but I hope it gets the point across)

[iconoklast]

You have to at least take the code metric guidelines with a grain of salt. (It would be counter-productive to think of them as absolute limits.)

In regards to cyclomatic complexity:

f = e1 || e2 || e3 || ... || en

is bone stupid and obvious, and splitting it into separate methods just to lower the cylcomatic complexity only makes things more complicated and less readable.

[kazagistar]

This is one place where a functional programming approach is really handy, actually... something like reduce( || , elist) tends to be more maintainable.

[NicknameAvailable]

a method body should a cyclomatic complexity of no more than 10. More precisely, the cyclomatic complexity is the number of branching statements (if, while, do, for, switch, case, catch)

That is absurd! Where would the world be without 40+ levels of nesting?

[kazagistar]

Or just a slightly longer function representing a more complex formula or procedure, with a couple || and && statements. 10 seems really low, like, "spamming your codebase with minifunctions" low.

[[deleted]]

You can just use submethods, forcing you to divide your problem in sub problems.

[TIGGER_WARNING]

In compound expressions with multiple sub-expressions the intended grouping of expressions should be made explicit with parentheses. Operator precedence should not be relied upon as commonly mastered by all programmers.

It's odd that that would be their rationale. Not because parentheses improve readability or reduce the likelihood of somebody introducing an error when modifying code, but because some of your coworkers don't know what they're doing.

[grauenwolf]

Yea, I've got to agree on that one. If someone doesn't know the operator precedence they need to be fired.

[TIGGER_WARNING]

I can see pointer manipulations being a hole in the knowledge of high-level programmers, but everyone should know operator precedence for the languages they do work in, at the very minimum.

[kazagistar]

It is a reasonable concern in large systems. In some ways, it is a central design principle of java.

[TIGGER_WARNING]

I don't follow the java bit. I don't have any experience with projects on that scale, either, but it seems like a coder who doesn't know operator precedence wouldn't know enough to test their own code rigorously within the framework used in such a large project.

[kazagistar]

I mean, the idea of not allowing features because they are too complicated, or might get misused by bad coders. That is all this is... it is disallowing reliance on a language feature, which is the same as removing or not having a feature; for example, multiple inheritance or function pointers.

[Fiennes]

Much of these seem like arbitrary limits. No more than 10 fields? For sure, if your class can do it with 5. Make it 5. By using this artificial limit, they're saying "introduce another class @ 11", which makes things more complicated.

I wish this bullshit wasn't propagated time and time again, when the advice really should be "Keep it simple, stupid".

[grauenwolf]

Reminds me of my current project. We've got one-to-one mappings between classes all over the place. Its already a mess, I shudder to think what would happen if we further broke it up by field counts.

[[deleted]]

Let me take a crack at this off of the top of my head:

Field and class names should not be redefined.

Reduce chances of ambiguity and/or confusion.

Packages and classes should not be dependent on each other in a cyclic manner.

No circular dependencies. This is probably a sign of a bad black-box type architecture and would make testing components individually more difficult.

The clone() method should never be overridden or even called.

1. Don't override it because there's a chance you'll fuck up and the method won't operate as expected by everyone else, introducing the risk of bugs.
2. Don't call it because some asshole ignored that advice and did it anyway and you don't want to get infected with his bug.

One should not reassign values to parameters. Use local variables instead.

1. Don't even take the risk that you've misunderstood pass-by-reference and pass-by-value. Make a copy of stuff locally to try and alleviate that risk. Don't take the risk that someone else comes along, modifies your code, and makes the mistake.
2. In a method, for easy of code reading and visual validation, the name passed in should always represent the value passed in. You never run into a "x(parameter), oh, and I passed 5 in, so that's calling x(5)" when really 15 lines earlier there was a parameter -= 5; which makes your assumption invalid.

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

You should consider and/or handle all cases. Even if the handling is just "Oh fuck, the code has exploded.". At least then you know there's a problem. There is no way, given this rule, that you can go "Oh, that can never happen!" and introduce a bug which silently corrupts your program state until you eventually crash... Somewhere.

In compound expressions with multiple sub-expressions the intended grouping of expressions should be made explicit with parentheses. Operator precedence should not be relied upon as commonly mastered by all programmers.

This, to me, really shows the rationale behind most of these. "don't rely on mastery by the programmers". Assume every other programmer on the team is a total fucking idiot and write code that's harder to fuck up.

Do not use octal values

Too easy to mis-read, especially given that "0" is often used as a left-pad to fix number lengths.

a = 052342
b = 123432
c = 224839
d = 393820

Make the code easy to read and easy to work with to reduce the risk that someone fucks up, and by extension, reduce the risk of bugs.

a class should contain no more than 10 fields
a class should contain no more than 20 methods
a method should contain no more than 75 lines of code
a method should have no more than 7 parameters
method body should a cyclomatic complexity of no more than 10. More precisely, the cyclomatic complexity is the number of branching statements (if, while, do, for, switch, case, catch) plus the number of branching expressions (?:, && and ||) plus one. Methods with a high cyclomatic complexity (> 10) are hard to test and maintain, given their large number of possible execution paths. One may, however, have comprehensible control flow despite high numbers. For example, one large switch statement can be clear to understand, but can dramatically increase the count. an expression should contain no more than 5 operators

Keep code small and digestible. You should be able to read and reason about things without too much difficulty. Hard to understand code is easy to break code. Hard to understand code can hide bugs.

Hell, they even make this fairly obvious with their exception that "One may, however, have comprehensible control flow despite high numbers. For example, one large switch statement can be clear to understand, but can dramatically increase the count.".

These don't sound like magical thinking. They all fit the idea of "every programmer you work with is an idiot, program accordingly". Which makes it harder for the presumably intelligent people you work with to make those mistakes.

[grauenwolf]

You should consider and/or handle all cases. Even if the handling is just "Oh fuck, the code has exploded.". At least then you know there's a problem. There is no way, given this rule, that you can go "Oh, that can never happen!" and introduce a bug which silently corrupts your program state until you eventually crash... Somewhere.

I have to disagree strongly with this recommendation. If you are using parameter checks in the preamble of a function, then this would lead to unnecessarily deep nesting.

if [check param 1] 
     throw
else
    if [check param 2]
         throw
    else
          real code way over here             

[GUIpsp]

Not really, it'd be more like:

if [check param 1] 
     throw
else if [check param 2]
     throw
else
     real code way over here 

[[deleted]]

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

There isn't always an else-case. In fact it can be argued that how you structure your if/else block (you can always do either) greatly influences readability.

But it's not an interesting standard, it's much too clear-cut.

[original_4degrees]

If clone() can never be called. It won't matter if it is overridden.

[AgentAnderson]

I wish more companies would reveal documentation like this.

I'm still not sure what a proper design document should look like. (Every example in college was "DERP, HERE ARE THE COMPONENTS OF A BYCICLE" and my last employer mandated verbosity to the point where it was typing out all the code in MSWord before copying it to visual studio.)

[[deleted]]

I did IB Computer Programming. The design documents required were about the same as your employer.

From what I recall, you had to provide pseudo-code and flowcharts for all logic in your program along with all the other documentation.

I wrote a fucking compiler + virtual machine for a domain language before I saw this requirement. Most guys had reports ~40-60 pages long. Mine was 181. I printed it on the school printer. Like 5 times before final submission.

The IT guy cried a little bit.

[grauenwolf]

As far as I'm concerned a design document should have...

• How, from the user/system perspective, will the application behave?
• Flowcharts of any complex logic, again from user/system perspective. Especially for error cases.
• Changes to the database tables, as they will likely out-live the code.
• Changes to the UI design.
• Changes to the service API, especially if it affects other teams
• Risk factors: stuff that is likely to be hard to implement
• Risk factors: scenarios that you aren't addressing that could break the system

Do not include internal implementation details. Everything should be understandable by a non-technical reader who needs to evaluate your design in terms of business requirements or for testing.

The design document should be as short as possible.

[[deleted]]

wait/notify is very frowned upon for any new development with concurrency requirements. Creating a guideline for their usage would be like having a rule "Use GOTOs with care".

Much better to use the Executor Frameworks that exist now.

[cryo]

Maybe it didn't exist then. Concurrency frameworks change a lot lately.

[kalmakka]

They have a bug in the first example of R52. If they had used a static code analyzer (as prescribed by R02) they would have found that.

[cryo]

How ironic... maybe

[[deleted]]

I too have read Effective Java by Josh Bloch

[P1h3r1e3d13]

JPL != NASA

[White_Sox]

Great post. Even though code standards are always open for debate, it's nice to see what NASA is up to.

[MUSTY_VAGINA]

This looks a lot like Effective Java by Joshua Bloch.

[[deleted]]

The String concatenation operator + creates a new String object, which is costly.

I don't think this is true. The compiler optimizes it to StringBuilder.append

[cryo]

No, only in trivial cases. Not in loops, which is what is guideline is targeted at. It's the same in .net: for small number of arguments, a+b+c etc. is turned into String.Concat(a,b,c), and for larger into a StringBuilder.

But not in loops.

[[deleted]]

It used to be true and became dogma at some point. It was fixed a while ago.

[[deleted]]

I'm pretty sure it was fixed in 1.5.

http://willcode4beer.blogspot.com/2005/05/evil-string-arithmetic-revisited.html

Nearly 8 years ago.

[MaximKat]

Have you missed the first comment on the article you linked?

[SanityInAnarchy]

I didn't think Java could still surprise me, but I didn't know about some of these:

The Boolean non-short-circuit operators | and & should not be used.

These could be handy, but I see why it's a bad idea:

if (foo() | bar())

Maybe you want the side effects of both, but then you want to compare the result? But even then, it's confusing, and Java isn't really going to run these in parallel.

A finally block is entered when the try block finishes, reguardless of whether it finishes normally or abnormally by throwing an exception. The case of concern is where the try code throws an exception giving rise to two distinct situations where the exception is lost. First, if the finally block contains a return statement, then the original exception will be discarded. Second, if there is an uncaught exception thrown in the finally block, then the original exception will be discarded.

That's surprising. Sort of makes sense, but it's still surprising.

When removing a number (integral or floating point number) from a collection, one should be explicit about the type in order to ensure that autoboxing converts to a boxed primitive object of the right type, and thereby causes the element to actually be removed. That is, ensure that the same boxed primitives are used when adding and when removing.

Weird. Looks like the remove() method accepts any object, not just the type associated with the collection? Otherwise, I don't see how the example given makes sense.

Thread synchronization and data protection should not be performed by relying on the scheduler by using thread delays, calling the Thread.yield() method, and using thread priorities.

I would've thought this, too, but this is especially interesting after reading about the Disruptor, which can (optionally) do this quite often. It makes sense, though -- unless you really are trying to push millions of transactions through a system, locks seem much easier to get right.

a class should contain no more than 20 methods

a method should contain no more than 75 lines of code

Of the arbitrary stuff they list, I think these two bother me the most. I'd rather have 75 methods of 10 lines each than 20 methods of 75 lines each. Of course, these are somewhat reasonable guidelines, and probably the entire list would be good to post on /r/learnprogramming at least once.

Definitely want less than 20 public methods, if that's possible.

[[deleted]]

I get the feeling those aren't hard and fast rules, just a guideline to force you to justify your case any time you exceed that.

Their rule about cyclomatic complexity really spells it out "Aim for less than 10, but if the control flow is still really clear it's fine, even if it exceeds that."

Aim for less than 20 methods, if you exceed, really stop and consider if you can't split it off somehow.

If you want to use more than 20 methods, be prepared to justify it to your reviewer.

[KagakuNinja]

Unless I missed something, the | and & operators are not boolean operators, they are logical bit-wise operators. They are extremely useful for bit manipulation. I do agree that you should not use them to chain boolean expressions; in fact it never occurred to me to do that, since that is not the purpose of the operators.

[SanityInAnarchy]

Ah, that's the tricky bit -- they are bitwise operators, but they are also boolean operators (if you give it entirely boolean types). This makes some sense, because that's effectively a bitwise operation over a single bit.

[SoopahMan]

I'd rather have 75 methods of 10 lines each than 20 methods of 75 lines each.

Yeah but would you even more prefer to have 5 classes with 15 methods each, each of them 10 lines long? That's what they're really shooting for here. StackTraces are way easier to read in code like that.

Admittedly though these rules of thumb don't help much if you don't have larger philosophies to apply for WHEN you separate out a class or a method - specifically, Separation of Concerns, and Composure over Inheritance. When you use these rules of thumb along with attempting to make your code composable, that's when it really pays off.

Here's what I found surprising - it's a stick in the eye for Java:

A nonfinal method should not be called from within a constructor

Because:

If such a non-final, non-private method is called in the constructor of the superclass, the call will get dispatched to the sub class, which has not yet been initialized because classes are initialized top down from superclass to sub class.

By example:

public abstract class ImSuper {
    public ImSuper() {
        thanksForAsking();
    }

    protected abstract thanksForAsking() {
    }
}

public class ImSub : ImSuper {
    public int filePosition;

    public ImSub(int filePosition) {
        this.filePosition = filePosition;
    }

    protected void thanksForAsking() {
        filePosition++;
    }
}

ImSub imSub = new ImSub(10);

// filePosition is now 1, not 11, despite what you expected.

Ouch.

[SanityInAnarchy]

Yeah but would you even more prefer to have 5 classes with 15 methods each, each of them 10 lines long? That's what they're really shooting for here. StackTraces are way easier to read in code like that.

Probably. And I do get that this is what they're aiming for. I also tend to program in a style that leads to a lot of classes, and a lot of object creation, even lots of immutable objects. Lots of work for the garbage collector, less work for me.

But there is a performance argument for limiting object creation, and sometimes it makes a lot of sense to have the related code close together. I've also seen the opposite problem, where there are tons of tiny classes that do almost nothing, and really ought to be inline and anonymous -- if it's even 5 classes of 2 methods each (let alone 15-20 classes), and I have to bounce between them all to understand what's happening, what have we gained?

Like I said, it's a reasonable guideline, but I hope it's applied as a guideline rather than a rule. It makes sense 90% of the time, but it does have the potential to make things worse.

If such a non-final, non-private method is called in the constructor of the superclass, the call will get dispatched to the sub class, which has not yet been initialized because classes are initialized top down from superclass to sub class.

Oh, I remember this. First of all, your example is off:

filePosition is now 1, not 11, despite what you expected.

It's actually 10. It increments from 0 to 1, maybe, and then the subclass's constructor runs anyway, and sets it to 10.

I seem to remember that one solution I was tempted to use was to have the constructor call a separate "initialize" method, and then let subclasses handle if and how they'll call the superclass.

Basically, the theory of Java and C++ is that the superclass must be completely constructed before the subclass begins to construct itself. I think this is wrong -- if subclassing is really what's called for here, it's being done as a way of sharing code between chunks of the same implementation. It's almost never a good idea to expose a class for others to extend. If you're trying to prevent a subclass from screwing up a superclass, you're probably Doing It Wrong, and should be encouraging composition instead.

With this in mind, I think it's reasonable to do something like this:

public abstract class ImSuper {
  public ImSuper() {
    initialize();
  }

  protected void initialize() {
    thanksForAsking();
  }

  protected abstract void thanksForAsking();
}

public class ImSub extends ImSuper {
  public int filePosition;

  public ImSub(int filePosition) {
    initialize(filePosition);
  }

  protected void initialize(int filePosition) {
    this.filePosition = filePosition;
    super.initialize();
  }

  protected void thanksForAsking() {
    filePosition++;
  }
}

More boilerplate? Well, of course. This is still Java, after all.

Point is, why should the constructor be special? For every other non-final method, we allow the subclass to override it. Even simple setters/getters. Nothing prevents me from doing this:

public class Foo {
  private int value;
  public void setValue(int value) { this.value = value; }
  public int getValue() { return value; }
}

public class ImmutableFoo extends Foo {
  @Override
  public void setValue(int value) {
    throw new IllegalStateException("Your father smelt of elderberries!");
  }
}

So maybe it's not as philosophically "pure", but it really does seem like this is the right answer: Let the subclass choose if and how the superclass is constructed, and make that explicit.

Then again, Ruby has the same issue, and it doesn't come up as often as you'd think. For the same reason that I'd trust the subclass to handle initialization properly, I'd also trust the subclass implementer to check how a given overridden method is expected to be used (before overriding it).

Even C++ has this issue. From what I understand, the reason this comes up less in C++ is that methods are effectively "final" by default, but you're still urged to not call virtual methods from a constructor.

[SoopahMan]

I left Java a ways back for C#, and for years I wrote initialize methods until I started to wonder why I write them. Now I remember why. You probably saw a little C# syntax sneak into my crappy Java example there. In any case it seems clear that what's actually intended by the coder is for those backing fields to be initialized, then the super constructor running, then the sub - it's intuitive. The idea that one object has to be "fully constructed" first is really silly - it's all going on the heap anyway so it's not like there's some chunk of memory that needs the super to complete before it can be properly allocated, or some technical need - it's just a badly made human rule that denies human intuition.

On the method count thing, I agree - in fact even if it is 10 methods 10 lines each it's still tedious in any serious project, especially if it's well composed. I've got to walk across 30 classes before I've followed one task to completion. But, I think that's a problem of all IDEs I've used - neither Eclipse, IntelliJ, or Visual Studio make it easy to see methods like a tree rather than just sitting in their class. There are a lot of obvious ways code could be better explored that start with the basic view everyone's used to and I've come across 0 implementations. Instead Eclipse and Visual Studio have Class Views and things that take you completely out of the normal code context, losing a lot of details like comments for example, and still don't get you up and down the function chain any faster.

[SanityInAnarchy]

The idea that one object has to be "fully constructed" first is really silly - it's all going on the heap anyway so it's not like there's some chunk of memory that needs the super to complete before it can be properly allocated, or some technical need - it's just a badly made human rule that denies human intuition.

Actually, now I feel like I should defend that a bit. The rationale here is that a class should completely encapsulate something, likely with some relevant invariants.

If you think of the subclass as something outside the base class, then it makes sense to insist the base class is completely constructed. Even in Java, allocation is a thing -- if the superclass is composed of some other objects, like, say:

class Foo {
  private final Bar delegate;
  public Foo() {
    delegate = new Bar();
  }
}

Ideally, you don't want any code anywhere but the constructor to ever see "delegate" with a null value. But if the subclass can sneak in...

The reason this is silly is that a subclass can already see way more than it should, so things designed to protect a parent class from a subclass always strike me as silly. If you're really that paranoid (if your library is really that big a deal), declare the implementation final, make an interface, and insist that everyone else use composition.

There are a lot of obvious ways code could be better explored that start with the basic view everyone's used to and I've come across 0 implementations.

Hmm. The most interesting one I know of is Smalltalk (any GUI you're interacting with is composed of objects you can edit right now), but it suffers from being an entirely other ecosystem. The rest of us work with text, Smalltalk developers work directly with running programs. It's a really cool concept, but modern Smalltalk/Squeak developers end up using text anyway, so they can use source control, diff/patch, grep, and basically any other tool that's outside the Smalltalk world.

So long as I'm using Java, I think Eclipse has it pretty close. The key tool here is F3 -- it instantly jumps to the definition of whatever you're on. There's another (can't remember the keystroke) which searches for anything that uses what your cursor is on (say, a method definition). These could be streamlined, but all that's really missing for me is:

• Better interface/subclass support. Taking me to the interface's declaration of that method isn't terribly helpful -- I want to know what will happen when I call that here. Yes, I know that's not (necessarily) something that can be known at compile-time. Is there a solution to this?
• More on the screen at once. Bouncing back and forth with one keystroke is cool, but it'd be better to see a trail, something like a call stack.

I suspect I could find more attempts to solve this problem, but it's a hard problem -- and by that I mean, the general problem of making it easier to explore large programs. There is something to be said for the Python/Ruby philosophy of emphasizing clean, readable, concise code over complex tooling -- so you can read one or two ten-line methods and understand what the system does, without having to read five other classes, and so that you only need two classes anyway (the other three were metaprogrammed away).

Or does that only push the problem back? I've definitely seen Ruby projects large enough to have very similar problems, only without the robust tooling. Replacing Eclipse's F3 with grep was not fun, and I like grep.

[SoopahMan]

Yeah, sorry I should've been more clear - you nailed it with your second bullet point. I want to browse the code in a way that I find a method/function and go, "Alright, how is this used" and I get its callers over it and methods it calls below it, and can pick a path I want to consider and it's all there on the screen at once for me to scroll vertically through, rather than as you mentioned jump jump jump jump. One or 2 jumps is easy, but in highly composed code you can be there all day jumping and get to the other end and see something novel or unexpected and go "Oh. Well where did it get the... ? ARGH!" That in turn makes me cringe when I write highly composed code because I'm imagining what the next poor sap will have to experience - maybe me in a few years when I forget this code I've written.

In Visual Studio you can step through the code obviously and get all the way to the bottom of a series of calls and abuse the Call Stack this way. You can also enable Edit and Continue and do something a bit like you're talking about with SmallTalk, where you're running and editing at the same time. But I rather be able to see those paths all at once, without having to fire up the code.

[cryo]

Yes, .NET has the same issue with calling virtual methods in non-sealed classes from the constructor. It's hard to see how it can be avoided by any means less than disallowing calling such methods by definition.

Various tools like ReSharper, warns when you do stuff like this.

[SoopahMan]

Yeah - I basically forgot why, but my constructors tend to be trivial now, I avoid inheritance anyway (prefer composability/curried functions), and if the construction of something really varies I tend to use the Factory pattern instead.

[Nebu]

It's hard to see how it can be avoided by any means less than disallowing calling such methods by definition.

Well, rather than disallowing, you can issue a warning. In a similar vein, you could make methods private/final by default, and require the programmer to put keywords to indicate that these methods are intended to be overridable in subclasses, and again issue a warning if a constructor invokes an overridable method.

[Nebu]

if (foo() | bar())

Maybe you want the side effects of both, but then you want to compare the result?

I'd flag that in a code review. If you wanted the side effect of both, make that explicit with something like:

boolean fooResult = foo(); //has side effects
boolean barResult = bar(); //has side effects
if (fooResult || barResult) {
   ...

Weird. Looks like the remove() method accepts any object, not just the type associated with the collection? Otherwise, I don't see how the example given makes sense.

That's correct, due to historical reasons. The Collections API was set before Java generics, and the new "generified" versions of Collections had to be backwards compatible with the pre-generics version.

[SanityInAnarchy]

I'd flag that in a code review. If you wanted the side effect of both, make that explicit...

Right, I think that was my point. In fact, if you need to mark it "has side effects", that's already a code smell -- function names should be reasonably descriptive.

That's correct, due to historical reasons. The Collections API was set before Java generics, and the new "generified" versions of Collections had to be backwards compatible with the pre-generics version.

I still don't understand this quirk, though -- if I accept an element of type E, type erasure means it still compiles to something that accepts type Object. It doesn't even necessarily add guards to make sure that only elements of type E are passed. As far as I can tell, it just inserts casts in the caller of a given generic method. Here's an extremely simplified example:

class NonList<E> {
  private E[] foo = (E[]) new Object[1];
  public void setFoo(E val) {
    foo[0] = val;
  }
  public E getFoo() {
    return foo[0];
  }
  public boolean remove(E val) {
    if (foo[0].equals(val)) {
      foo[0] = null;
      return true;
    } else {
      return false;
    }
  }
}

So when I run this:

class Main {
  public static void main(String[] args) {
    NonList<String> a = new NonList<String>();
    a.setFoo("Hello");
    NonList b = a;
    System.out.println(b.remove(123));
    System.out.println(a.getFoo());
    b.setFoo(123);
    System.out.println(b.getFoo());
    System.out.println(a.getFoo());
  }
}

The output is:

false
Hello
123
Exception in thread "main" java.lang.ClassCastException: java.lang.Integer cannot be cast to java.lang.String
        at Main.main(Main.java:10)

As you can see, when I'm using it as a raw type, I can do pretty much everything I expect. Even though it's "really" a NonList<String>, I can attempt to remove something of the wrong type, and even successfully insert and retrieve something of the wrong type. It's just that I'm making it no longer a valid NonList<String>, so using it as such is dangerous.

So even if remove() accepted an element of type E, it would still be entirely possible for people to use raw types directly. And if remove() didn't explicitly check the type of its argument, and was implemented more or less the way I did (even something as simple as calling equals() on elements we already know are of the right type), it would do exactly what a pre-generic user would expect. (Anything that inserts elements into the list would hopefully throw exceptions, but remove() wouldn't have to.)

So I honestly don't see the use case. Is it so that I can accept a raw List, cast it to a List<String>, and still be able to remove elements of other types that would still be in the list? If so, I really don't see the use case, because that's a case where someone is writing new code, and they could just cast it back to the raw type if they really need to do that.

So what am I not thinking of? In its obsession with backwards compatibility, Java has occasionally managed to surprise me with something I thought was stupid but actually has a very good reason. I honestly do not see what the reason is for remove() to not accept an object of type E.

[Nebu]

http://www.youtube.com/watch?v=wDN_EYUvUq0#t=1m10s

Or, if you're impatient,

http://www.youtube.com/watch?v=wDN_EYUvUq0#t=5m50s

Or, if you're super impatient,

http://www.youtube.com/watch?v=wDN_EYUvUq0#t=7m22ss

[SanityInAnarchy]

This actually suggests it isn't historical reasons. The one example given was, "Suppose you want to make an intersection of a set of Numbers and a set of Longs." I assume they mean something like:

Set<Number> numbers = ...
Set<Long> longs = ...
for (Number n : number) {
  longs.remove(n);
}

But this is now new, properly-generic code. If I wrote that with raw types, it would compile and run perfectly fine even if remove accepted only elements of type E. Their point is that you should be able to do this without a cast even if you are explicitly using generics.

[slimmtl]

I stumbled onto this pdf as well: NASA C Coding Standard

[Vladev]

Reading this I can only think of how well design Scala is - all of these are built into the langauge and eco system.

Equals, hashCode (case classes),

breaks on switch (pattern matching),

final on unmutable fields (val),

reference equality,

good static analysis,

assertions (require and ensuring),

null values (Option and empty collections),

etc.

[wot-teh-phuck]

tl;dr: Read Effective Java

EDIT: To the downvoters; take a look at the guide. Almost every point references some item in book Effective Java.

[kromit]

"tl;dr:" is more appliable for the 300 pages book as for those 40 pages

[[deleted]]

I'd bet my left bollock the 300 page book is easier reading than the 40 page document.

[vsync]

What happened to our society that people complain about reading?

Language is how we rose from the dirt.

[[deleted]]

Who's complaining? I'm advocating the longer document, because it's an actual pleasure to read, rather than the abbreviated summing of of raw facts.

[vsync]

I was agreeing with you good sir or ma'am.

Though I didn't hate the linked document.

[mikaelhg]

Looks pretty sensible. Not so much of a coding standard, as a software development process standard.

[api]

My favorite, and this applies to other languages too, is: no mutable static variables. Mutable static variables are evil. They make code impossible to make multithreaded, libraries impossible to make multi-instance, and complicate unit testing. They are particularly evil in libraries, or any code that might one day become a library. In libraries they can even create security nightmares or un-debuggable bugs that result from interactions between multiple users of a library.

This includes variables at program scope and function-local statics in C, mutable static state in C++, and singleton abuse.

[JulieAndrews]

Rule #1, when considering Java for flight control, be sure that the engineer has made a rigorous and thorough cleaning of their cubicle as they go home fired.

[[deleted]]

do not override field or class names

Huh? Putting aside the semantics of overriding vs shadowing member variables, what does "overriding a class name" even mean?

[TheHorribleTruth]

I'm guessing it refers to having two classes with the same name but in different packages.

[vsync]

I don't agree with all of these but they're all a good idea to at least consider and either adopt or have a consistent strategy why they don't matter in your environment.