High Quality Code

The anatomy of high quality code that supports longevity, cross-team usage, and correctness


Some limitations might apply

  • This training is not proven science
  • Everything is highly subjective
  • There is no real truth, but there is also no real untruth
  • The only fact is that we need good code
  • There are many ways to achieve the same goal
  • You will probably not agree
  • That is ok...
  • ...hopefully you still take away something
  • This is a motivation to consider cleaner coding practices and turn them into law

What is good code?

What do you think is good code?

  • ???

There are two ways of constructing a software design: One way is to make it so simple that there are obviously no deficiencies and the other way is to make it so complicated that there are no obvious deficiencies.

C.A.R. Hoare, The 1980 ACM Turing Award Lecture

Why do we need good code?

Let's continue the last ideas

  • It is maintainable... by many
  • It is extensible
  • It is reusable
  • It is correct
  • It is robust
  • It is predictable
  • It is efficient but still understandable

Any fool can write code that a computer can understand. Good programmers write code that humans can understand.

Martin Fowler

If you are proud on the fact, that nobody gets your code despite the code being correct, you are working for the wrong company.

René Schwietzke

How long does code live?

Just data from four popular projects



How long does code live?

Just data from four popular projects



The 8 and a half Basic Rules

Just to Get into the Mood

Simplicity and Readability


  • Be concise
  • Less code, less problems
  • Does not mean short or obfuscated

Simplicity is prerequisite for reliability.

Edsger W. Dijkstra


  • Others can read your code
  • Comments, conventions, and documentation

Modularity and Layering


  • Build larger functionality on smaller
  • Make things reusable


  • Have APIs
  • Clearly separate concerns
  • Think of the TCP/IP model or OSI

I invented the term ‘Object-Oriented’, and I can tell you I did not have C++ in mind.

Alan Kay

Design and Efficiency


  • Plan before you build
  • Thoughts are cheaper than debugging
  • Functional spec vs. internal blueprint


  • Efficient and quick
  • Plan for performance high level
  • Optimize low-level later if needed
  • Yes, there is even green code

Elegance and Clarity


  • Simplicity and efficiency at the same time
  • Brilliant code that is not hard to understand


  • Understand code
  • Make code understandable by others
  • Have a clear goal
  • If you cannot explain it to others, you don't know what your are doing

... and a half

The final touch - Balance

  • You cannot have all 8 at the same time perfectly
  • Balance it out
  • Constantly decide what is the best

Writing Code

Let's talk about writing code



  • Yes, really.
  • Formatting is what makes a newspaper readable and recognizable
  • Imagining books not being consistently formatted?
  • Code has to be readable easily
  • Code has to be mergeable easily
  • Yes, formatting is a war
  • You can only end it by dictating something

Formatting Suggestions

Just some ideas for Java code based on personal preferences

  • Allman or BSD style: Braces on a new line to make blocks clear
  • One line blocks get braces as well
  • Indent by four spaces
  • Sensible code width of 120 to 150 characters
  • Everything should be final if possible
  • Camel-case naming
  • No underscores except in CONSTANTS
  • Constants in uppercase
  • No upfront variable declaration
  • Empty blocks possible but with explanation
 * The Intershop and Demandware Style
 * Lookup the key in the map, returns the value if found, null otherwise.
 * Will throw an IllegalArgumentException when null is passed as key.
 * @param k the key to look up in the map
 * @return the value found or null otherwise
public V get(final K key) throws IllegalArgumentException
    // prevent null from being used
    if (key == null)
        throw new IllegalArgumentException("Get doesn't support null keys");

    // where is our entry located
    final int pos = calculatePosition(key);

    // get the entry
    final Entry<K, V> entry = getEntry(key, pos);
    if (entry == null)
        // does not exist
        return null;
        // we know it, return value
        return entry.value;


Good code doesn't need comments, it explains itself...?!

  • That is so much §%$&%%"§!!!
  • Half-life of knowledge about code is short
  • Theories say 3 to 6 weeks and you reverse-engineer your own code again
  • Not every line of code speaks, not every method name can speak
  • Comments have several purposes:
    • They guide the eye
    • They document the interface
    • They document your ideas and decisions
  • Forget documentation outside the code, it dies quickly
  • It is not your code! It is our code!
  • I don't need comments.
  • It was hard work for me, why should it be easier for others?
  • Smart people can read code without comments.

Always code as if the guy who ends up maintaining your code will be a violent psychopath who knows where you live.

Rick Osborne


Best practices

  • Revolves around grouping of your code
  • See JDK as good example, most documentation is in code and packages
  • Make clear what the API is for and what consumption you permit
    • Public
    • Internal
    • Component only
    • No API at all
 * Hash table based implementation of the <tt>Map</tt> interface.  This
 * implementation provides all of the optional map operations, and permits
 * <tt>null</tt> values and the <tt>null</tt> key.  (The <tt>HashMap</tt>
 * class is roughly equivalent to <tt>Hashtable</tt>, except that it is
 * unsynchronized and permits nulls.)  This class makes no guarantees as to
 * the order of the map; in particular, it does not guarantee that the order
 * will remain constant over time.
 * <p>This implementation provides constant-time performance for the basic
 * operations (<tt>get</tt> and <tt>put</tt>), assuming the hash function
 * disperses the elements properly among the buckets.  Iteration over
 * collection views requires time proportional to the "capacity" of the
 * <tt>HashMap</tt> instance (the number of buckets) plus its size (the number
 * of key-value mappings).  Thus, it's very important not to set the initial
 * capacity too high (or the load factor too low) if iteration performance is
 * important.
 * <p>An instance of <tt>HashMap</tt> has two parameters that affect its
 * performance: <i>initial capacity</i> and <i>load factor</i>.  The
 * <i>capacity</i> is the number of buckets in the hash table, and the initial
 * capacity is simply the capacity at the time the hash table is created.  The
 * <i>load factor</i> is a measure of how full the hash table is allowed to
 * get before its capacity is automatically increased.  When the number of
 * entries in the hash table exceeds the product of the load factor and the
 * current capacity, the hash table is <i>rehashed</i> (that is, internal data
 * structures are rebuilt) so that the hash table has approximately twice the
 * number of buckets.

 * <p>As a general rule, the default load factor (.75) offers a good
 * tradeoff between time and space costs.  Higher values decrease the
 * space overhead but increase the lookup cost (reflected in most of
 * the operations of the <tt>HashMap</tt> class, including
 * <tt>get</tt> and <tt>put</tt>).  The expected number of entries in
 * the map and its load factor should be taken into account when
 * setting its initial capacity, so as to minimize the number of
 * rehash operations.  If the initial capacity is greater than the
 * maximum number of entries divided by the load factor, no rehash
 * operations will ever occur.
 * <p>If many mappings are to be stored in a <tt>HashMap</tt>
 * instance, creating it with a sufficiently large capacity will allow
 * the mappings to be stored more efficiently than letting it perform
 * automatic rehashing as needed to grow the table.  Note that using
 * many keys with the same {@code hashCode()} is a sure way to slow
 * down performance of any hash table. To ameliorate impact, when keys
 * are {@link Comparable}, this class may use comparison order among
 * keys to help break ties. ...

More Examples

 * Implementation notes.
 * This map usually acts as a binned (bucketed) hash table, but
 * when bins get too large, they are transformed into bins of
 * TreeNodes, each structured similarly to those in
 * java.util.TreeMap. Most methods try to use normal bins, but
 * relay to TreeNode methods when applicable (simply by checking
 * instanceof a node).  Bins of TreeNodes may be traversed and
 * used like any others, but additionally support faster lookup
 * when overpopulated. However, since the vast majority of bins in
 * normal use are not overpopulated, checking for existence of
 * tree bins may be delayed in the course of table methods.
 * Tree bins (i.e., bins whose elements are all TreeNodes) are
 * ordered primarily by hashCode, but in the case of ties, if two
 * elements are of the same "class C implements Comparable",
 * type then their compareTo method is used for ordering. (We
 * conservatively check generic types via reflection to validate
 * this -- see method comparableClassFor).  The added complexity
 * of tree bins is worthwhile in providing worst-case O(log n)
 * operations when keys either have distinct hashes or are
 * orderable, Thus, performance degrades gracefully under
 * accidental or malicious usages in which hashCode() methods
 * return values that are poorly distributed, as well as those in
 * which many keys share a hashCode, so long as they are also
 * Comparable. (If neither of these apply, we may waste about a
 * factor of two in time and space compared to taking no
 * precautions. But the only known cases stem from poor user
 * programming practices that are already so slow that this makes
 * little difference.)
 * Because TreeNodes are about twice the size of regular nodes, we
 * use them only when bins contain enough nodes to warrant use
 * (see TREEIFY_THRESHOLD). And when they become too small (due to
 * removal or resizing) they are converted back to plain bins.  In
 * usages with well-distributed user hashCodes, tree bins are
 * rarely used.  Ideally, under random hashCodes, the frequency of
 * nodes in bins follows a Poisson distribution
 * (http://en.wikipedia.org/wiki/Poisson_distribution) with a
 * parameter of about 0.5 on average for the default resizing
 * threshold of 0.75, although with a large variance because of
 * resizing granularity. Ignoring variance, the expected
 * occurrences of list size k are (exp(-0.5) * pow(0.5, k) /
 * factorial(k)). The first values are:
 * 0:    0.60653066
 * 1:    0.30326533
 * 2:    0.07581633
 * 3:    0.01263606
 * 4:    0.00157952
 * 5:    0.00015795
 * 6:    0.00001316
 * 7:    0.00000094
 * 8:    0.00000006
 * more: less than 1 in ten million

And a bad one...

 * Implements Map.remove and related methods
 * @param hash hash for key
 * @param key the key
 * @param value the value to match if matchValue, else ignored
 * @param matchValue if true only remove if value is equal
 * @param movable if false do not move other nodes while removing
 * @return the node, or null if none
final Node<K,V> removeNode(int hash, Object key, Object value,
                           boolean matchValue, boolean movable) {
    Node<K,V>[] tab; Node<K,V> p; int n, index;
    if ((tab = table) != null && (n = tab.length) > 0 &&
        (p = tab[index = (n - 1) & hash]) != null) {
        Node<K,V> node = null, e; K k; V v;
        if (p.hash == hash &&
            ((k = p.key) == key || (key != null && key.equals(k))))
            node = p;
        else if ((e = p.next) != null) {
            if (p instanceof TreeNode)
                node = ((TreeNode<K,V>)p).getTreeNode(hash, key);
            else {
                do {
                    if (e.hash == hash &&
                        ((k = e.key) == key ||
                         (key != null && key.equals(k)))) {
                        node = e;
                    p = e;
                } while ((e = e.next) != null);
        if (node != null && (!matchValue || (v = node.value) == value ||
                             (value != null && value.equals(v)))) {
            if (node instanceof TreeNode)
                ((TreeNode<K,V>)node).removeTreeNode(this, tab, movable);
            else if (node == p)
                tab[index] = node.next;
                p.next = node.next;
            return node;


An example

  • Proper method comment
  • All parameters documented
  • Comment guidance in the code
  • Decisions documented (StringBuilder size)
 * Processes an uri and replace an encoded space (+) by %20.
 * There is no error handling defined, make sure you don't pass
 * null. 
 * This method is thread-safe and stateless.
 * @param uri
 *            the uri as String to process, null is not permitted
 * @return a new uri as String with all spaces being encoded as %20 
 *         instead of +
public static String fixUrlSpaceEncoding(final String uri)
    // determine ?
    final int pos = uri.indexOf("?");
    if (pos < 0)
        // nothing to do
        return uri;

    // get the data before and after the ? pos, keep the ?
    final String path = uri.substring(0, pos);
    final String query = uri.substring(pos);

    // we assume that some replacements have to be made and hence ask 
    // for a little more space (+16) already to avoid that the buffer grows
    // that is ok for our cases right now, because we mostly have only 
    // 1-4 spaces in it, if you encounter more, you can start with more overhead
    // to keep it overall efficient
    final StringBuilder sb = new StringBuilder(uri.length() + 16).append(path);
    for (int i = 0; i < query.length(); i++)
        final char c = query.charAt(i);
        // replace + with %20
        if (c == '+')

    return sb.toString();

Boilerplate OO

Don't write what you don't need


  • An interface is a statement
  • It indicates that several implementations can exist
  • It indicates that this is an API
  • If you have to cast to get to details to use it, something is wrong

OO Forests

  • Not everything is object-oriented
  • Some stuff can be implemented very simply
  • Inheritance can make the code hard to read
  • Inheritance might not lead to less code

Object-oriented programming is an exceptionally bad idea which could only have originated in California.

Edsger W. Dijkstra


You owe it to yourself

  • Documented: Well, that is obvious and it should include usage examples
  • Stable and Consistent: Naming is the same everywhere, breaking API changes are versioned or a new API
  • State the state: Make clear if it is stateful or stateless or if the consumer is required to manage the state
  • Secure: Care about it, no universal rule applies here
  • Easy to Adopt: Make first time usage simple, make migration to it simple
  • Tested: It is fully tested and the state of the API is frozen by tests
  • Small: Make it as compact as possible
  • Less Dependent: Should not depend on other APIs
  • Fast: Performance has to be awesome otherwise state that it is not designed for X

Obscuring Logic

Logic should be readable easily

  • Sort out unwanted conditions first aka guards first
  • IFs should be short, if you have to think about it later again, it is too complex
  • Order matters
  • Don't nest too many or and and
  • Negating with ! can be overlooked, consider foo == false instead
  • Keep ternary operators compact and readable, prefer if-else otherwise
public boolean doIt(String s)
    // empty s cannot be processed, sort things out early
    if (s == null || s.length() == 0)
        return false;
    // That is BAD code here
    if (s.length() == 3 && 
            !"foo".equals(s) && 
            !("bar".equals(s) || "rab".equals(s)))
        return false;
    // final result (too obscure)
    return s.split(",").length() > 3 ? 
                return true : 
                s.split(";").length() < 3; 

Negated Switches

Be clear, don't negate what you want to express

  • People think positive
  • Asking for on or running is more natural
  • Don't say enable this to disable that or set it to true to disable it
  • Feature is on or off, don't say not on
  • Do the same for variables
# Uncomment to disable the processing, it is enabled by default
# myapplication.processing.disable = true
 * Horrible example
public void test(final boolean disabled)
    if (!disabled)
        // do stuff here
    if (isEnabled())


By consistent and compare to what others do

  • Use naming consistently
  • Try not to invent a new scheme
  • Look at the JDK for naming help (when programming Java)
  • Write correctly in camel casing, abbreviations as words
  • Parameters need good names too
  • Name local variables equally well
  • You can shortcut a single char, int for loops or other simple things
 * Remove all entries from the list
 * But why purge, all of Java says clear()?!
public void purge() 
    final boolean globalState = getGlobalState();
    final String organisationName = getName();
    for (final char c : organisationName.toCharArray())
        // more here

// parameters and overloading
public void removeCarsFromStreet(final Street street)
public void removeCarsFromStreet(
                final Street street, final Car carType)
public void removeCarsFromStreetByType(
                final Street street, final Car carType)
public V getFromMap(final K key); // JDK says get()
public void put(List<V> list); // JDK says putAll or addAll!

public final static int NUMBER = 42; // I am a constant

public final static String DEFAULT_NAME = "Maria"; // So do I

Commented Code

Retire or remove?

  • When try out code or disable code, commenting it is fine
  • Explain what you did and why and perhaps when this can go
  • Feel free to attach case numbers
  • For everything else there is GIT
 * Demo
public boolean demo(final String s)
    // ...
    // Comment and replaced with enhanced for, because it is 
    // more compact code (FOO-987654)
    // for (int i = 0; i < s.length(); i++)
    // {
    //    final char c = s.charAt(i);
    //    if (c != ' ')
    //    {
    //        result.add(c);
    //    }
    // }
    for (final char c : s.toCharArray())
        if (c != ' ')

    // ...


Get the rare cases right

  • Never catch all, catch only what applies
  • Never catch Errors
    • JDK: An Error is a subclass of Throwable that indicates serious problems that a reasonable application should not try to catch. Most such errors are abnormal conditions.
  • Don't catch common runtime exceptions, they indicate programming errors
  • Exceptions are rare conditions, don't use them for communicating the normal
  • Exception cost time and might force the JIT to throw away code
public void test()
    try(InputStream is = new FileInputStream(new File("f.txt")))
        int i = is.read();
    catch (final FileNotFoundException e)
        // do your stuff here
        // try to avoid other exceptions
    catch (final IOException e)
        // do your stuff here
        // try to avoid other exceptions
        // in case something is left to do after all that
        // trouble

Empty Blocks

Explain why is something not there

  • Nothing can be a powerful message
  • Make clear it is on purpose
  • Explain why the second case doesn't have to be covered
 * Fill the battery if needed
public void fillUpOfNeeded()
    if (getGlobal() < 42)
        // we don't need to recharge, we still have
        // enough juice

Small is Nice

Small methods are good methods

  • Small methods are easier to test and read
  • Decompose larger logic into smaller chunks
  • Compose logic out of smaller methods
  • Large methods cannot be effectively inlined
  • Too large methods might not be JIT-compiled at all


Arguments are implicit documentation

  • Make arguments speak
  • Avoid argument confusion when you have multiple signatures per method
  • Keep the order consistent
  • Java and JavaScript signatures don't consider the names only type and order
  • Long argument lists should be avoided
public void test()
public void test(boolean a, String b)
public void test(boolean a, String b, String c)
public void test(String b, String c, String d, boolean a)

public void split(
                String line, 
                char delimiter, 
                boolean quotesPermitted)


Contain data to the required scope

  • Limit visibility scope as much as possible
  • Don't pre-declare variables without assignment
  • Prefer final local variables if possible
public void limitedScope(final String s)
    // dont't do that
    char c; 
    // make things final if possible
    final StringBuilder sb = new StringBuilder();
    for (int i = 0; i < s.length(); i++)
        c = s.charAt(i);


State is hard to control and error-prone

  • State is needed but need to be controlled
  • Try immutable objects instead
  • Immutable is not always efficient
public class DiceCup
    // list of dice to use
    private final List<Dice> dice = new ArrayList<>();
     * Constructor, takes a list of dice to be used
    public DiceCup(final List<Dice> dice)
     * Rolls all dice
    public List<Dice> roll()
        for (final Dice d : dice)
        // keep a copy for returning
        final List<Dice> result = new ArrayList<>(dice);
        // empty out cup
        // return result
        return result;


Everything that has to be returned

  • Proper resource handling is key to reliable applications
  • File handles, open input or output streams, databases (PalDB), network, connections
  • Use try-with-resource preferably to delegate management to the language
  • Acquire and return on the same level and preferably method
  • Keep usage short
  • Don't leak resources and let others close them
public void test()
        InputStream  is = new FileInputStream(new File("foo.txt"));
        OutputStream os = new OutputStream(new File("bar.txt"));
        // your I/O code here
    catch (final FileNotFoundException e)
        // do special stuff for the exception case
    // This is the virtual finally block.
    // Java will auto-close "os" first
    // "is" last

Error Handling

Always error handle completely

  • Document all error situations and the outcome
  • Consider three levels
    • Runtime: Don't handle that. It tells you, you screwed up
    • Error: Serious problem, no recovery possible
    • Standard: You communicate a rare case explicitly and the case can be handled without breaking anything
  • Consider reuse of exceptions when it is API consistent before starting your own new set of exceptions
  • If you can fallback for error handling, prefer that over communicating it

Functional Code

Make the right move at the right time

  • Functional code is great
  • Functional code can be difficult to read or maintain
  • Always check the trade-off
  • Functional is great for less redundancy
public void test()
    final List<Person> persons = persons.stream().filter(p -> {
        if (p.getGender() == Gender.MALE) 
            return true;
        LocalDate now = LocalDate.now();
        Duration age = Duration.between(p.getBirthDate(), now);
        Duration adult = Duration.of(18, ChronoUnit.YEARS);
        if (age.compareTo(adult) > 0) 
            return true;
        return false;
    }).map(p -> p.getFirstName() + " " + p.getLastName())


Log properly and carefully

  • Log any unexpected error condition with enough details
  • Make sure you prepare for the case where logging is needed to debug
  • Keep log content meaningful because logs are large and lines might be not adjacent to other
  • Make sure the log levels make sense (DEBUG, INFO, WARN, ERROR)
  • If you don't care about WARN messages, it is not warn
  • A stable happy system should not write anything


State and plan

  • Tell others that your code is fit for concurrency
  • Tell others why your code uses concurrency and how
  • Explain the concept and its limitations
  • Measure that concurrency actually helps
 * This method sorts incoming data and is using up the max 
 * number of available CPU cores to be quick. The threads will
 * be automatically started and stopped.
 * Tests have shown that is scales well up to 10 cpus and any 
 * further CPU core up to 20 contribute about 80% more of the 
 * theoretical limit. Beyond that, more cpus don't contribute 
 * anything. 
 * No tests with more than 40 cpus have been performed.
public SomeStream sort(SomeStream inputData)


Nothing can irritate you

  • Know the data you are getting
  • Explain what data you expect
  • Explain what you don't expect
  • If there is anything you can't know yet, explain how you handle the worst case
  • No runtime errors due to proper testing
  • Errors are logged and explained
  • Known error states are handled correctly and not logged


Good code is efficient code

  • Think about algorithms first
  • Think about data dimensions
  • Limit what you cannot handle
  • Write good code first, measure, decide about tuning
  • Don't start micro-tuning before it is necessary
  • Be aware what resources are sparse: CPU, memory, disk...
  • Measure under production condition and not on a single notebook

Testing Code

Let's talk about testing code

Testing Basics

Just a quick rinse

  • Bottom line: No tests, no code
  • Black box first, documentation first
  • Forget the code and forget what your ideas behind it
  • Never make assumptions about the later usage
  • If an API (aka shared code), make sure the behavior is frozen by tests
  • Don't forget state
  • Vary your test data
  • White box next
  • Check for assumptions made
  • Check for hardcoded values
  • Check for behavior that is not documented
  • Check for all potential error conditions
  • Check for state information
  • Check for OS dependencies
  • Check for multi-threading requirements

Behavior Captured

Make sure your test is complete

  • Good tests capture the full behavior
  • Important for APIs
  • Every possible behavior has to be covered
  • Every error situation
  • Every input
  • Every possible return value
  • Documentation states that behavior, tests prove it


Make sure your test is documented

  • Explain what your are testing
  • You might want to explain why
  • Not every unit test is mapping to a single class, explain the scope
  • Name your methods clearly
  • You might want to consider classes per methods when the test cases are rich
 * Tests for the alternative implementation of ParseLong 
 * that is supposed to faster but does not handle all edge-case
 * instead falls back to JDK based parsing.
 * This class is holding all tests for com.foo.bar.ParseNumbers
public class ParseNumbersTest
     * Verify constructior and that all constructors are
     * private only.
    public void constructorsArePrivate();

     * Verify some standard good cases that are covered by 
     * this implementation. We are not measuring the performance
     * difference, just verifiying correctness.
    public void parseLongs_StandardGoodCases();
     * Verify the fallback to JDK long parsing for 
     * negative numbers, because this is not covered 
     * by this alternative implementation, because we don't
     * expect this in the input data.
    public void parseLongs_Fallback();

     * Everything that is not parseable also not as fallback
     * should report a NumberFormatExcpeption
    @Test(expected = NumberFormatException.class)
    public void parseLongs_ExceptionForUnparseable_NoData();

    @Test(expected = NumberFormatException.class)
    public void parseLongs_ExceptionForUnparseable_Null();

    @Test(expected = NumberFormatException.class)
    public void parseLongs_ExceptionForUnparseable_Letters();

Reviewing Code

Let's review what someone has written


What is a code review?

Code review is systematic examination of computer source code.


Review Your Code

Why and how


  • To understand it
  • To learn from it
  • To verify common style
  • To ensure consistency
  • To find things not visible from the outside (white box)
  • To investigate the untestable: concurrency and security
  • To find untested branches
  • To find incorrect assumptions made


  • Automated: Let tools review code, mostly not practical and does only cover a small range
  • Manual, use a checklist
    • Naming
    • Styleguides
    • Documentation
    • Tests
    • Efficiency
    • Security

Review Your Tests

Quick reminder what to check

  • Are all cases covered?
  • Not only happy-path but really all including edge-cases, especially when the API is public or pseudo-public
  • Are the tests documented?
  • Are the tests consistent?
  • Are the tests stable?
  • If we touched the area, we should almost never have less tests, almost more
  • Check changes of existing tests, because it might have broken the freeze-promise

Questions and Answers