JMRI Code: Recommended Practices

This page contains miscellaneous info and pointers for JMRI developers.

Class Library Preferences

We use Java Swing for our GUI development. It's a lot more powerful than the original AWT, and the price is right. We have a recommended organization and usage pattern that's documented on another page.
JMRI uses the PureJavaComm libraries to support serial communications on Macintosh, Linux and Windows. Before JMRI 4.7.5, we used the RXTX and SerialIO libraries for this, but these were then removed.
Take a few moments to learn about the different types of Java collections that are available ( List, Deque, HashMap, etc) in the java.util package. Not everything needs to be an array! Only use a Vector when there's a specific reason for it such as compatibility with an existing API; they're slow and memory-intensive compared to e.g. an ArrayList. A Deque can be a good default solution to holding data that won't ever contain 'null'. Consider using a TreeSet for data that's best kept in some sorted order; there's are very few good reasons for sorting data during normal running in JMRI. If you also need random access (e.g. "The 5th element") consider IndexedTreeSet and IndexedTreeMap
JMRI uses Java generics extensively to reduce errors and improve understanding. With lots of people writing and sharing code, using generics instead of casts makes it much easier to understand the code, and allows the compiler to catch many misunderstandings about how things should be used. Most of the important information on them can be found on this page from the Java Tutorial.
If you need to use comma-separated variable (CSV) files, please use the javacsv API if possible. We are already using that in a number of places, and will probably use it in more. If that doesn't provide enough functionality, we might eventually move to the opencsv API, but since we only want to use one, the conversion will be a lot of work.

Collections

Take a few moments to learn about the different types of Java collections that are available ( List, Deque, HashMap, etc) in the java.util package.

Please don't use fixed-size arrays for holding variable sized data in objects. That generally ends up wasting huge amounts of space at runtime, and we try to keep the JMRI memory footprint small when we can.
Only use a Vector when there's a specific reason for it such as compatibility with an existing API; they're slow and memory-intensive compared to e.g. an ArrayList.
A Deque can be a good default solution to holding data that won't ever contain 'null'.
Consider using a TreeSet for data that's best kept in some sorted order; there's are very few good reasons for sorting data during normal running in JMRI. If you also need random access (e.g. "The 5th element") consider IndexedTreeSet and IndexedTreeMap

Code Format

The Java Code Conventions (if that link is broken, try this one from the Internet Archive) for names, formatting, etc are really useful. If you find that you can't read a piece of code, these will help make it better.

Note that we have a few local conventions beyond those in the Java recommendations. You'll find them on other pages in this section, but for example, we recommend that you define the logger reference at the bottom of each file.

Deprecating Code

As development proceeds, sometimes old ways of doing things have to be replaced by new ways. In many cases, you can just change all the using code in our repository, and move forward. For general interfaces that might be used externally to JMRI, such as in scripts and CATS, we prefer to leave the old interface in place for a while, marking it as "deprecated" so that people can discover that it will eventually go away. The sequence is then:

Write the new methods.
Mark the old methods with @deprecated and @Deprecated. The associated comment should tell people where to find the replacement (see below).
Note that if you mark an interface or super-class (i.e. abstract) class or method as deprecated, you should also mark all implementations of it as deprecated. That way, they won't themselves show as deprecations to fix, but code that uses them will.
Start generating warnings for users (especially scripting users) by adding:
```
            jmri.util.Log4JUtil.deprecationWarning(log, "myMethod()");
```
to the start of each method. This will put a warning in the log if that method is used during normal operation.
If you have tests for that method (you should!) you may need to modify the direct tests of the deprecated method; see the instructions on the JUnit page.
As soon as you can, remove all JMRI uses of these deprecated classes and/or methods until the deprecations report is clean. (Until you do this, warnings will be showing up in the logs) You can either compile locally with ci-compile-deprecations to check this, or look at the Jenkins CI report (click the Types tab) to check the JMRI/JMRI master branch. Don't forget to also migrate the Jython sample scripts!
Wait an appropriate length of time; the more visible the class or method, the longer to wait. Things that were in scripts should wait at least two production releases.
Remove the deprecated methods and their tests.

Note that a deprecated interface is meant to still work. Deprecated should only mean that you can't count on the deprecated interface working in the future, so that it would be good to code away from it while it's still working.

There are two forms of marking something as deprecated (Javadoc tag and Annotation), and both allow you to add additional information. A nice discussion of the technicalities is here. We strongly recommend using both of them like this:

/**
 * (Other Javadoc comments)
 * @deprecated As of 2.7.8, use {@ link #foo()} instead
 */
@Deprecated // 2.7.8

where the line contains the version in which the deprecation is applied. That lets you easily know how long ago it was deprecated. (There's a better way to do this in Java 9, but JMRI still has to compile under Java 8 so please don't use that; see the release roadmap for background)

You may want to work with the deprecation checks "on" during compilation. To do that, change this line of build.xml:

<property name="deprecation" value="on" />

This lets you pay attention to new deprecation warnings as you code.

Exceptions

Throwing Exceptions

When checking inputs (i.e. for valid parameter values) and you find a problem, what should you do?

You can throw an unchecked exception. IllegalArgumentException is a great example: Most of the time it doesn't really have to be thought about. It's rare to explicitly try/catch for it. When an IllegalArgumentException (or whatever) is thrown, it works up to some high-level handler, where there will be some default handling (usually just logging) it, and the various libraries will get a chance to clean things up.
You can thrown a checked exception, i.e. a JmriException subclass or some on-target Java checked exception class. This forces all writers of method-using code to think about how to handle problems. They might wrap with a try-catch or just declare it to go upward, but they have to think about it.
Don't use error codes or other "check the return value" approaches. They just make it likely there will be invisible problems that bite people in complicated ways (because you can ignore them, leaving junk behind)

Generally, JMRI developers tend to throw an unchecked exception, i.e. IllegalArgumentException or similar.

Catching Exceptions

SpotBugs will object to code like this:

  try {
     // do something here
  } catch (Exception e) {
  }

with a REC_CATCH_EXCEPTION and/or a DE_MIGHT_IGNORE (less often DE_MIGHT_DROP). This is an example of two problems:

Catching Exception instead of more specific types
Having nothing in the catch block

Let's discuss those separately:

Catching the Exception class

There are two subcases here:

You're catching Exception because a lot of different specific things can be thrown, and it's a pain to write multiple catch blocks for each of those. That's actually obsolete thinking, though, because now there's syntax for combining those:
```
  try {
    // do something here
  } catch (IOException | JDOMException e) {
    // and do something, see below
  }
```
That's a much better way to convey what this code is intended to do.
You want to catch any unchecked exception, such as NullPointerException, that is thrown. Those are all subclasses of RunTimeException, so the clean way to do this is to catch that:
```
  try {
    // do something here
  } catch (RunTimeException e) {
    // and do something, see below
  }
```

Empty catch block

What's an empty catch block trying to say?

Ideally, the code should handle the exception, in the sense of reacting to it so that the right stuff still happens for the user. So do that as a first strategy.
But sometimes, that's complicated, or the error is really something that isn't understood, or perhaps is routine and doesn't matter.
If you can't do anything else, at least consider logging that the code has caught something. Typically, this would be a warning:
```
log.warn("can't do anything useful with this:", e);
```
Adding the Exception object e to the log logs all its content, including the traceback. That way, if something weird happens later on, at least there's a clue in the log.
If it's really routine, log at debug or even trace level. That way if it turns out later to not actually be routine, turning on additional logging will help track it down.
Consider not catching the error, so that it propagates up. (The methods for targeting the catch block more narrowly in the prior section can help here) It will eventually be caught, and will contain a more useful stach trace. This might require adding the exception to the signature of the current method so you can throw it upwards.
Or, if absolutely can't change the signature, consider re-characterizing it, e.g.:
```
  try {
     // do something here
  } catch (IOException e) {
     // do something to clean upp
     ...
     // But still need to signal that this failed.
     // Let's blame the arguments we are processing:
     throw new IllegalArgumentException("couldn't process because ...", e);
  }
```
If it really is "ignore this, it's going to be OK", you should add an annotation so that others know why it's OK and don't have to worry about it in the future:
```
@edu.umd.cs.findbugs.annotations.SuppressFBWarnings(value={"DE_MIGHT_DROP", "DE_MIGHT_IGNORE"}, 
    justification = "(Something about why this is OK)")
```