`__
is the right way to do this.
In addition to being "nicer" to read because it's clearer, much more importantly
than, a multi-catch does not also accidentally catch RuntimeException, as catch
(Exception e) would.
Catching RuntimeException such as NullPointerException & Co. is typically not
what the developer who used "catch (Exception e)" as shorthand intended.
If a catch (Exception e) is used after a try { } block which does not call any
methods declaring that they may throw checked exceptions with their throws
clause (perhaps not anymore, after code was changed), then that catch may really
have been intended to catch any possible RuntimeException instead?
In that case, if there exceptionally really is a particular reason to want to
"do something and recover from anything that could possibly go wrong, incl.
NullPointerException, IndexOutOfBoundsException, IllegalArgumentException,
IllegalStateException & Co.", then it is clearer to just catch
(RuntimeException e) instead of catch (Exception e).
Before doing this, double check that this truly is the intention of that code,
by having a closer look at code called within the try,
and see if that called code couldn't simply be made more robust.
Be particularly careful with methods declaring checked exceptions in their
“throws” clause:
if any matching exception is thrown inside the “try” block, changing
“catch (Exception e)” to “catch (RuntimeException e)” could change the method
behavior since the exception will exit the method instead of being processed by
the “catch” block.
Proliferation of catch (Exception or RuntimeException e)
{ LOG.error("It failed", e); } in regular "functional" code is a symptom of a
missing abstraction in framework code; e.g. an Abstract interface implementation
helper with correct default error handling, so that functional code does
not have to repeat this over and over again.
For example:
#. For DataBroker related transaction management, check out the (at the time of
writing still in review) new utilities from
`c/63372 `__ & Co.
#. For RPC related catch, see
`c/63413 `__
#. Instead of ``catch(Exception)`` after a ``try { close(anAutoCloseable) }``
just use ``AutoCloseables.closeOrWarn(anAutoCloseable)`` introduced in
https://git.opendaylight.org/gerrit/#/c/44145/
Sometimes developers also simply don't see that an existing framework API
intends implementations to simply propagate their errors up to them.
For example, for Exception handling in:
#. OsgiCommandSupport's ``doExecute()``, the right thing to do is... nothing.
The parent ``doExecute()`` method declaration throws Exception,
and that is intentional by the Good People of Karaf.
Therefore, ``catch(Exception)`` in a OsgiCommandSupport's ``doExecute`` is not required
: in this case it's perfectly OK to just let any error "propagate" upwards
automatically.
If ``doExecute()`` calls other private methods of an OsgiCommandSupport
implementation, then it is perfectly OK to make those methods declare
``"throws SomeException"`` too, and not "handle" them yourself.
#. Callable's ``call()`` passed to a ``DataStoreJobCoordinator enqueueJob()``,
the right thing to do is... nothing.
Do not catch ``(Exception)`` but let it propagate.
If it's useful to "augment" the exception message with more custom details
which are available inside Callable's ``call()``, then the right thing to do is
to ``catch (Exception e)
{ throw new YourProjectApiException("Failed to ... for {}", aDetail, e); }``
and, exceptionally, use ``@SuppressWarnings("checkstyle:IllegalCatch")``.
#. ``org.opendaylight.infrautils.inject.AbstractLifecycle``'s start() and stop()
methods, again the right thing to do is... nothing.
Do not catch any Exception but let it propagate.
Here are some cases where ``catch(Exception)`` is almost always wrong, and a
respective ``@SuppressWarnings`` almost never acceptable:
#. In Tests code you typically just ``@Test testSomething() throws
(Some)Exception`` instead of catch,
or uses ``@Test(expected = ReadFailedException.class)``.
One rare case we have seen where it's justified is a
``@Test(expected = ReadFailedException.class)``
with ``catch (Exception e) throw e.getCause()``.
#. In one time "setup" (initialization) kind of code.
For example, catch for a ``DataBroker registerDataChangeListener`` makes little
sense - it's typically much better to let a failure to register a data change
listener "bubble up" then continue, even if logged, and have users wonder why
the listener isn't working much later.
Only in lower-level "Framework" kind of code, catch (Exception e) is sometimes
justified / required,
and thus ``@SuppressWarnings("checkstyle:IllegalCatch")`` acceptable.
Catching ``Throwable`` in particular is considered an absolute No No
(see e.g. discussion in https://git.opendaylight.org/gerrit/#/c/60855/)
in almost all cases.
You may got confused any meant to catch Exception (see above)
or RuntimeException?
Carefully consider whether you mean to catch and set some flag and/or
log, and then rethrow, or intended to "swallow" the exception.
``System.out``
^^^^^^^^^^^^^^
The ``RegexpSingleLineJava`` "Line contains console output" and "Line contains
``printStackTrace``" should NEVER be suppressed.
In ``src/main`` code, ``System.out.println`` has no place, ever (it should probably be
a ``LOG.info``; and ``System.err`` probably a ``LOG.error``).
In Java code producing Karaf console output, we should only use ``System.out``, and
add the corresponding ``@SuppressWarnings``. ``System.out`` handles pipes and remote
sessions correctly.
In ``src/test`` code, there should be no need to write things out - the point of a
test is to assert something.
During development of a test it is sometimes handy to print things to the console
to see what is going on instead of using the debugger, but this should be removed
in final code, for clarity, and non-verbose test execution.
If you must, do you a Logger even in a test - just like in ``src/main`` code.
This also makes it easier to later move code such as helper methods from test to
production code.
Javadoc Paragraph: < p > tag should be preceded with an empty line
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Checkstyle (rightfully) flags this kind of JavaDoc up as not ideal for
readability:
.. code:: java
/**
* Utilities for...
* This...
and you can address this either like this:
.. code:: java
/**
* Utilities for...
*
*
This...
or like this:
.. code:: java
/**
* Utilities for...
*
* This...
Different ODL developers `agree to disagree `__
on which of the above is more readable.
Additional Resources
--------------------
- ``Checkstyle`` http://checkstyle.sourceforge.net/
- ``Maven``:
https://github.com/checkstyle/checkstyle/blob/master/src/main/resources/google_checks.xml
- ``Eclipse``:
https://github.com/google/styleguide/blob/gh-pages/eclipse-java-google-style.xml
- ``IntelliJ``:
https://github.com/google/styleguide/blob/gh-pages/intellij-java-google-style.xml
- `How to set Checkstyle up in IntelliJ old wiki page `__
Classes methods / fields ordering
=================================
Ordering based on modifiers. This is based on visibility and mutability:
| public static final fields
| static final fields
| private static final fields
| private final fields
| private non-final fields
| private volatile fields
| private constructors
| public constructors
| static factory methods
| public methods
| static methods
| private methods
| The first group should be very strict, with the exception of
FieldUpdaters, which should be private static final, but defined just
above the volatile field they access. The reason for that is they are
tied via a string literal name.
The second group is less clear-cut and depends on how instances are created --
there are times when juggling the order makes it easier to understand what is
going on (e.g. co-locating a private static method with static factory method
which uses it).
The third group is pretty much free-for-all.
The goal is to group things so that people do not have scroll around to
understand the code flow.
Public methods are obviously entry-points,
hence are mostly glanced over by users.
Overall this has worked really well so far because;
- the first group gives a 10000-foot overview of what is going on in the class,
its footprint and references to other classes
- the second group gives instantiation entry-points, useful for examining who
creates the objects and how
- the third group is implementation details -- for when you really need to dive
into the details.
Note this list does not include non-private fields.
The reason is that public fields should be forbidden, as should be any mutable
non-private fields.
The reason for that is they are a nightmare to navigate when attempting to
reason about object life-cycle.
Same reasoning applies to inner class, keep them close to the methods which use
them so that the class is easy to read.
If the inner class needs to be understood before the methods that operate on it,
place it before them, otherwise (especially if it's an implementation detail)
after them.
That's when an inner class is appropriate of course.
error-prone
===========
The infrautils projects has adopted the `error-prone code quality tool `__
(by Google), with suitable customized configuration.
You can use it by using ``org.opendaylight.infrautils:parent`` instead of
``org.opendaylight.odlparent:bundle-parent``.
Note that ``@SuppressWarnings("InconsistentOverloads")`` needs to be placed on the
class, not method; see
https://github.com/google/error-prone/pull/870 and
https://github.com/google/error-prone/issues/869.
SpotBugs
========
SpotBugs is the successor project to FindBugs (which is dead).
SpotBugs is enforced by default across all artifacts since Magnesium.
For artifacts that do not pass SpotBugs, either fix them or disable enforcement
by defining the following property in the pom.xml:
.. code-block:: none
false
All notes re. FindBugs listed below do still apply to SpotBugs as well
(it's compatible).
FindBugs
========
Note that starting with odlparent 3.0.0 when we say "FindBugs" we now actually
mean "SpotBugs", see above.
``@FBSuppressWarnings``
-----------------------
If really needed, projects can to override individual FindBugs rules on
a case-by-case basis by using a ``@SuppressFBWarnings`` annotation:
.. code:: java
@SuppressFBWarnings("RCN_REDUNDANT_NULLCHECK_OF_NONNULL_VALUE")
public V get(K key) {
Unchecked/unconfirmed cast from ``com.google.common.truth.Subject`` to ``com.google.common.truth.BooleanSubject`` etc.
----------------------------------------------------------------------------------------------------------------------
FindBugs seems to be too dumb to deal with perfectly valid Google Truth test
code (which does not use any explicit cast...) so it's OK to:
.. code:: java
@SuppressFBWarnings("BC_UNCONFIRMED_CAST")
an entire JUnit \*Test class because of that.
null analysis errors, incl. FindBugs' NP_NONNULL_FIELD_NOT_INITIALIZED_IN_CONSTRUCTOR
-------------------------------------------------------------------------------------
see the general null analysis next chapter.
``nonNullAndOptional``
^^^^^^^^^^^^^^^^^^^^^^
Some of the content from this chapter may be moved to
http://www.lastnpe.org later...
Everything ``@NonNullByDefault``
--------------------------------
Do not use null anywhere, assume all method arguments and return values are
``NonNullByDefault``.
null annotations from ``org.eclipse.jdt.annotation`` instead of ``javax.annotation``
------------------------------------------------------------------------------------
We prefer using the null annotations from the package ``org.eclipse.jdt.annotation``
, instead of the ones from ``javax.annotation``
(or those from ``org.jetbrains:annotations``, or ... ``Android``/``Lombok's``/some of the
other ones out there).
This is because the org.eclipse one are modern generics enabled @Target
TYPE_USE annotations, whereas the other ones are not.
Obviously we do NOT "depend on Eclipse" in any way, or "need Eclipse at
run-time" just because of 4 annotations from an org.eclipse package,
which are available in a very small JAR at build-time; e.g.
``org.eclipse.jdt.annotation.NonNull`` is absolutely no different from
``javax.annotation.Nullable``, in that regard.
BTW: The ``javax.annotation NonNull`` & Co. are not more or less "official"
than others; Prof. FindBugs Bill Pugh pushed those to Maven central, but
his "dormant" JSR 305 was never officially finalized and approved;
he's since moved on, and no longer maintains FindBugs.
The Eclipse annotations are also supported by SpotBugs/FindBugs (`says
this issue `__) as well
as NullAway.
null analysis by FindBugs VS. Eclipse JDT
-----------------------------------------
FindBugs' null analysis is inferior to Eclipse JDT's, because:
- richer null analysis
- generics enabled (see above)
- works with existing external libraries, through external annotations,
see http://www.lastnpe.org
- much better in-IDE support, at least for Eclipse users
*WIP: We are aiming at, eventualy, running headless Eclipse JDT-based null
analysis during the build, not just for users of the Eclipse IDE UI;
watch*\ `issue ODLPARENT-116 `__
\ *, and*\ http://www.lastnpe.org\ *.*
BTW: FindBugs is dead anyway, long live SpotBugs! \_TODO Gerrit & more
documentation to clarify this...\_
PS: An alternative interesting null checker tool is the `Checker
Framework `__.
Runtime null checks
-------------------
In addition to static null analysis during development, you can check null
safety at run-time.
Please use either `JDK's Object's requireNonNull `__
\ () or `Guava's
Preconditions `__
``checkNotNull()`` utility, instead of if (something == null).
Please also use the variant of ``requireNonNull()`` or ``checkNotNull()`` with the
String message to indicate what argument is checked.
For example:
.. code:: java
public doSomething(Something something) {
this.something = Objects.requireNonNull(something, "something");
}
We recommend use of JDK's Object's ``requireNonNull()`` instead of Guava's
Preconditions ``checkNotNull()`` just because the String message of the former can
prevent the problem you can have with the latter if you confuse the order of the
arguments.
We accept and think its OK that ``checkNotNull()`` throws a NullPointerException and
not an IllegalArgumentException (even though code otherwise should never
manually throw new NullPointerException),
because in this particular case a NullPointerException would have happened
anyway later, so it's just an earlier NullPointerException, with added
information of what is null.
We NEVER catch (NullPointerException e) anywhere, because they are programming
errors which should "bubble up", to be fixed, not suppressed.
``nullable`` errors for fields related to dependency injection
--------------------------------------------------------------
null analysis frameworks, such as Eclipse's or FindBugs or other, will not like
this kind of code in a ``@NonNullByDefault`` package:
.. code:: java
class Example {
@Inject
Service s;
}
the recommended solution is to always use constructor instead of field
injection instead, like this:
.. code:: java
class Example {
final Service s;
@Inject
Example(Service s) {
this.s = s;
}
}
When this exceptionally is not possible, like in a JUnit component test,
then it's acceptable to suppress warnings:
.. code:: java
@SuppressFBWarnings("NP_NONNULL_FIELD_NOT_INITIALIZED_IN_CONSTRUCTOR")
class SomeTest {
public @Rule GuiceRule guice = new GuiceRule(TestModule.class);
private @Inject Service service;
}
Optional
--------
You do not have to use Optional, because real null analysis can give us the same
benefit.
If cleaning up code for null safety, then do not mix introducing Optional with
other null related clean up changes; it's clearer for reviews if you FIRST fix
missing null checks and add null related annotations, and then THEN (optionally)
switch some return types to Optional.
You can use Optional for return types, but not method arguments.
Never use ``Optional>`` (obviously incl. ``Optional>``
or ``Optional>`` AND ``Optional>`` etc.),
just return a respective empty Collection instead.
Note that instead of if ``(anOptional.isPresent()) { return anOptional.get(); }
else { return null; }``
you can and for readability should just use return ``anOptional.orNull()``.
However ideally any such code should not return null but an Optional of
something itself.
Note that instead of ``if (aNullable == null) ? Optional.absent() :
Optional.of(aNullable)a``
;you can and for readability should just use ``Optional.fromNullable(aNullable)``.
To transform an Optional into something else if it present, use the transform()
method instead of an if () check;.
for example:
.. code:: java
List vrfEntries = MDSALUtil.read(broker, LogicalDatastoreType.CONFIGURATION, vpnVrfTables).transform(VrfTables::getVrfEntry).or(new ArrayList<>());
**Take care** with ``Optional.transform()`` though: if the transformation can return
null, ``Optional.transform()`` will fail with a NullPointerException
(this is the case of most YANG-generated methods).
You can use Java 8 ``Optional.map()`` instead; when it encounters null, it returns
``Optional.empty()``.
The above example would become
.. code:: java
List vrfEntries = MDSALUtil.read(broker, LogicalDatastoreType.CONFIGURATION, vpnVrfTables).toJavaUtil().map(VrfTables::getVrfEntry).orElse(new ArrayList<>());
Prefer the newer Java 8 ``java.util.Optional`` (JUO) over the older Google Guava
``com.google.common.base.Optional`` (GGO), because it offers a better functional
style API for use with Java 8 lambdas, which leads to much more naturally
readable code.
Until we fully migrate all ODL APIs (which is planned), in glue code calling
existing APIs returning GGO (such as the DataBroker API) but itself then wanting
to return a function of that as JUO, please just use the ``toJavaUtil()`` method
available in Guava Optional.
Further Reading & Watching
^^^^^^^^^^^^^^^^^^^^^^^^^^
- https://github.com/google/guava/wiki/UsingAndAvoidingNullExplained
- https://stackoverflow.com/questions/26327957/should-java-8-getters-return-optional-type
Streaming and lambdas
=====================
Lambdas are very useful when encapsulating varying behavior.
For example, you can use them instead of boolean behavior selection parameters:
.. code:: java
public void someMethodA(SomeObject A) {
commonMethod(A, false);
}
public void someMethodB(SomeObject B) {
commonMethod(B, true);
}
private void commonMethod(SomeObject C, boolean behaviour) {
// common code
if (behaviour) {
doA();
} else {
doB();
}
// common code
}
can be replaced with
.. code:: java
public void someMethodA(SomeObject A) {
commonMethod(A, this::doA);
}
public void someMethodB(SomeObjectB) {
commonMethod(B, this::doB);
}
private void commonMethod(SomeObject C, Function behaviour) {
// common code
behaviour.apply();
// common code
}
They are also useful for replacing small anonymous inner classes which follow
the functional pattern (implementing an interface with a single non-default
method).
Your IDE will typically suggest such transformations.
Lambdas should be avoided when the resulting code is more complex and/or longer
than the non-functional form.
This can happen particularly with streaming.
Streaming also has its place, especially when combined with Optional (see above)
or when processing collections.
It should however be avoided when many objects are created in the resulting
lambda expressions, especially if DTOs end up being necessary to convey
information from one lambda to the next where a single variable could do the
trick in a more traditional form. (TODO: provide examples.)