Style Guide

The LinuxForHealth FHIR Server has been written by many individuals over many years. Formatting has not been strictly enforced, but we'd like to improve it over time, so please consider the following points as you change the code.

This section provides both a Style and a Code Review Guide for FHIR (and Java projects in general). It also includes tips on where to focus attention to find common bugs. Code reviews should not only look for defects present in the current code, but also be aware how the current implementation might be prone to regression in the future.

This guide does not describe the formal review process, nor any official project documentation associated with such a review.

Leave the code better than you found it.

Git

Branch-naming convention:

issue-<#number>

Commit message convention:

short description #<number>

long description

Goals for This Guide

1. Catch defects early

We could use a long list here to describe the various goals of effective code review and consistent styling...but really Catch defects early says it all. Everything else (for example "improve product quality") just comes under this one umbrella.

Quick Reference

1. Follow secure coding best practices.

2. Write tests. Pull Requests should include necessary updates to unit tests (src/test/java of the corresponding project) and integration tests (in the fhir-server-test project).

3. Use comments. Preferably javadoc.

   Comment all types and methods.

   Each member should be preceded by a one line comment.

   It is acceptable to use one comment for a group of related members if their purpose is obvious from their names.

4. Separation of concerns. Apply OO principles of information hiding, loose coupling and high cohesion.

5. Keep the documentation up-to-date. Project documentation exists under the docs directory. We have a Conformance page for documenting conformance to the specification, a User Guide for FHIR Server administrators, and a number of other guides / module READMEs.

6. Indentation: 4 spaces, not tabs. For this we have a checkstyle rule which will fail the build if you're using tabs. We also prefer spaces over tabs in JSON and XML, but its not strictly enforced.

7. All members should be placed before the first method.

8. All constructors should be placed before methods.

9. Members almost always should be final when initialized in the constructor.

10. Members almost always should be private. Use getters and setters.

11. Hide implementation details inside the class.

12. Do not return modifiable collections.

13. Do not use synchronized methods. Keep synchronization internal.

14. Remove unused imports

15. Remove warnings

16. If TODOs are committed, they should be associated with an issue

17. Always use blocks

    // DO:
    if (test) {
        x++;
        doSomething();
    } else {
        doSomethingElse();
    }
    
    // DON'T:
    if (test) x++;
        doSomething();
    

18. Use try-with-resource instead of finally.

    Implement Autocloseable when creating your own resources.

19. Never rely on the finalizer to free resources - it is not guaranteed that the finalizer runs

20. Override toString() to provide single-line descriptions of the class content.

21. Always obfuscate passwords and other sensitive credentials when logging/writing program output.

22. Keep log entries to single-lines where possible

    This benefits downstream log processing tools.

23. Use consistent try-catch formatting

    try {
        doSomething();
    } catch (SpecificException x) {
        cleanUpTheMess();
    } catch (MoreGeneralException x) {
        cleanUpSomeMore();
    } finally {
        logger.exiting("foo");
    }
    

24. Only catch an exception if you can do something about it...or need to

25. Use java.util.logging

26. Always use nanoTime for measuring elapsed times, never currentTimeMillis which is subject to wall clock changes.

    Avoid other logging frameworks if possible. java.util.logging works well, and is supported by Liberty Profile.

27. Quote strings when printing log messages - for easier detection of whitespace issues:

    "Parsing of environment variable '" + KUB_EVENTSTREAMS_BINDING + "' has failed." // do this
    
    "Parsing of environment variable " + KUB_EVENTSTREAMS_BINDING + " has failed.`   // not this
    

    Ideally, this should be made into a utility function.

28. Don't add @author to class

    /**
     * @author myname
     */
     public class MyClass { 
    

29. Don't add @override comments

        /* (non-Javadoc)
         * @see com.ibm.fhir.database.utils.api.IDatabaseTarget#runStatement(java.lang.String)
         */
    

30. Write tests. Pull Requests should include necessary updates to unit tests (src/test/java of the corresponding project) and integration tests (in the fhir-server-test project)

31. Keep the documentation up-to-date. Project documentation exists under the fhir-docs repo and we have a CHANGELOG for tracking user-visible changes, a Conformance page for documenting conformance to the specification, and a User Guide for FHIR Server administrators.

32. Use good indentation.

33. Use spaces (not tabs) in java source. For this we have a checkstyle rule which will fail the build if you're using tabs. We also prefer spaces over tabs in JSON and XML, but its not strictly enforced.

34. Use spaces after control flow keywords (they're not function calls!) if/for/while blocks must always have { }

Code Review

Top Things to Look For

• Unit-tests?
• Do the unit-tests cover boundary conditions?
• Do the unit-tests cover concurrency?
• Is the code commented?
• Are the comments helpful and, most importantly, accurate?
• Loose coupling and high cohesion
• equals and ==
• hashCode and equals
• compareTo
• Always use { ... }
• Resource leakage - try-with-resource
• Logging
• Barriers around debug logging (avoiding expensive String construction)
• Concurrency control - synchronization/lock etc
• Lock leakage - finally unlock?
• Over-synchronization
• Double-checked locking (DCL) errors
• Singleton initialization - does it follow our chosen pattern?
• Unit tests with edge cases
• Algorithms - e.g. linear searches, loop nesting, joining in Java
• Unbounded recursion
• Use of Patterns
• Clone/Deep copy
• Assumptions on order (e.g. iterating over HashMap)
• Use of Vector
• Charset for IO operations
• Reading files into memory
• Streaming compression/JSON processing
• Correct data type usage (int instead of boolean)
• Random vs. SecureRandom
• Exception handling
• Use of StringBuilder instead of StringBuffer
• Multiple loop or routine exits
• Switch statement construction
• Default return values
• Method/block size
• Mixing app-server time with database time.
• Timezone handling (DST transition handling if non-UTC)
• Java SQL date/timestamp conversion
• Avoid SELECT * FROM unless the framework handles column changes
• Instrumentation - timing operations
• Implementation leakage (e.g. propagating SQLException beyond the data access layer

Configuration Files

Code Template

Eclipse Rules

Original Author is the FHIR Team