Gridarta Development Documentation: Code Conventions

This document describes the Code Conventions used for Gridarta.

Code Conventions for the Java Programming Language

Additionally, the following set of rules applies:

Small Rules

("small" in the sense how long it takes to describe them, not in their importance!)

• Imports are sorted alphabetically by package, within a package alphabetically by class.
• The *-form of import is not allowed.
• Use inline comments rarely. If you have to use inline comments to explain what code does, something is wrong with the code.
• Annotate variables and method types with @Nullable or @NotNull.
• Annotate action methods with @ActionMethod.

Field Initialization

Field initialization with the automatic default value is only allowed if the automatic default value carries semantics that are used later. In all other circumstances, which means in most cases, fields must not be initialized with the automatic default value. (The automatic default value is null, zero (any mutations of 0 or 0.0) or false.)

Stream I/O Handling

The following example code shows how streams should be opened / closed:

/*
 * Gridarta MMORPG map editor for Crossfire, Daimonin and similar games.
 * Copyright (C) 2000-2023 The Gridarta Developers.
 *
 * This program is free software; you can redistribute it and/or modify
 * it under the terms of the GNU General Public License as published by
 * the Free Software Foundation; either version 2 of the License, or
 * (at your option) any later version.
 *
 * This program is distributed in the hope that it will be useful,
 * but WITHOUT ANY WARRANTY; without even the implied warranty of
 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 * GNU General Public License for more details.
 *
 * You should have received a copy of the GNU General Public License along
 * with this program; if not, write to the Free Software Foundation, Inc.,
 * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
 */

import java.io.FileInputStream;
import java.io.FileOutputStream;
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;

public class SafeCopy {

    public static void main(final String... args) throws IOException {
        try (InputStream in = new FileInputStream(args[0]);
             OutputStream out = new FileOutputStream(args[1])) {
            final byte[] buf = new byte[4096];
            while (true) {
                final int bytesRead = in.read(buf);
                if (bytesRead == -1) {
                    break;
                }
                out.write(buf, 0, bytesRead);
            }
        }
    }

}

Use try-with-resources. Rationale:

• The streams are closed automatically, in reverse order of opening, whether the work completes normally or throws an exception.
• The stream references can't be null and are implicitly final.
• The stream references needn't be nulled because they lose scope as soon as the try block ends.
• Several streams need no nested tries; they are listed in one resource specification.
• If the work and closing a stream both throw an exception, the work exception is thrown and the close exception is attached to it as a suppressed exception (Throwable.getSuppressed()), so neither is lost.

Checkstyle

The enforced rule set is Checkstyle: ./gradlew check runs Checkstyle 8.33 with config/checkstyle/checkstyle.xml on all Java sources, and the build fails on violations. Run it before submitting a patch. The main rules are:

• No tab characters. Indent with 4 spaces instead.
• No trailing whitespace, and every file ends with a newline.
• No * imports, and no unused, redundant or illegal (sun.*) imports.
• Method and constructor parameters are declared final.
• Modifiers are in the order the JLS suggests, without redundant modifiers.
• Braces around all if, else, for, while and do bodies, with the usual placement of { and }.
• Whitespace around operators and keywords and after commas, and no padding inside parentheses.
• Standard Java naming for types, methods, fields, parameters and local variables.
• equals() and hashCode() are overridden together, boolean expressions and returns are not needlessly complicated, long literals use an upper case L, and arrays are declared Java style (String[] args).
• Property files that belong together contain the same keys.

The Rules in Detail

In addition to Checkstyle, the Gridarta developers use IntelliJ IDEA inspections. The inspection profile is not stored in the repository and is not run by the build. This description describes the inspection profile "No errors allowed here". As the name already suggests, this inspection profile must always run without errors. The purpose of this inspection profile is to prevent a regression of code quality. Currently it is quite small. New rules will be added whenever a certain level of cleanness and style is reached.

The following list explains the current rules, listed in a way that you can easily identify them in IntelliJ IDEA. It serves two purposes. One is to explain the rationale of the configured rules. The other is to enable developers that do not use IntelliJ IDEA / InspectionGadgets to know what the rules are to be able to perform the same verifications.

• Inspections
  • Abstraction issues

    'instanceof' check for 'this'

    This most likely is a failure to understand object oriented programming. Such constructs must be replaced by proper polymorphism.

  • Assignment issues

    Assignment replaceable with operator assignment

    Operator assignments should be used wherever applicable because they are easier to read: It's faster to see and understand that the new variable value is based on a modification of the original value, not a completely new value.
    Conditional operators are currently ignored because foo |= bar(); is not the same as foo = foo || bar(); because the behavior regarding side-effects in bar() is changed.

    Assignment to catch block parameter

    This reports things like catch (FooException e) { e = ...; } as they are either errors or, if not, at least very confusing.

  • Bitwise operation issues

    Incompatible bitwise mask operation

    Bitwise mask expressions which are guaranteed to always evaluate to true or false most likely are logical errors. Example: (var & constant1) == constant2.

    Pointless bitwise expression

    Pointless expressions that and with zero, or with zero or shift with zero most likely are logical errors.

    Shift operation by inappropriate constant

    Shifts with shift values out of range (0..31 for int, 0..63 for long) most likely are logical errors.

  • Class structure

    'final' method in 'final' class

    This is unnecessary and may be confusing. Methods in final classes are always implicitly final. A method should only be explicitly declared final if the context is "the class might be subclassed, but this method is not designed to be overridden". Thus if a method in a final class is explicitly declared final, it may lead to the conclusion that the class was declared final by mistake.

    Missing @Deprecated annotation

    Makes sure that deprecated members that are documented as deprecated in Javadoc (@deprecated Javadoc tag) are also annotated as @Deprecated in the source code.
    Background: The annotation is the form of deprecation that is visible through reflection and to annotation processors; a @deprecated Javadoc tag only documents it. javac warns about a Javadoc tag without the annotation (lint category dep-ann, enabled in the Gridarta build by -Xlint:all).

    Missing @Override annotation

    Makes sure that members that override members from a class (not implement from an interface) are annotated as @Override in the source code.
    Background: A missing @Override isn't an error by itself. But the opposite situation, an @Override when nothing is overridden is an error. That is useful for finding typos when wanting to override a method. E.g. if you override toString() using @Override public String tostring() (note the typo), the compiler will be able to report this as an error.

    'private' method declared 'final'

    Private methods are implicitly final. Explicitly declaring them final looks like the method should be public or protected instead.

    'protected' member in 'final' class

    Final classes cannot be sub-classed. Protected members are explicitly visible for subclasses. Because of that, protected members in final classes are an oxymoron that indicates an error.

    'public' constructor in non-public class

    If the class is not visible, the constructor isn't either. Declaring the constructor of higher accessibility than its class is pointless and most likely an error.

  • Code style issues

    Missorted modifiers

    Modifier order does not match JLS suggestion

    Annotations should be sorted alphabetically, annotations MUST be sorted before keywords, keywords must be sorted according to JLS.

  • Finalization issues

    'finalize()' called explicitly

    finalize() must only be called by the garbage collector, but not application software.

    'finalize()' does not call 'super.finalize()'

    This always is an error as this prevents the superclass from performing its own finalization code.

    'finalize()' not declared 'protected'

    This is an error because 'finalize()' must not be public because it never needs to be called directly.

  • General

    Declaration has Javadoc problems

    Makes sure that a Javadoc comment that is present also has a certain level of technical quality. Currently the following omissions are treated as errors: @author for top level classes, periods in briefs, @return for methods, @param and @throws or @exception for methods and constructors. Unknown Javadoc tags are also reported (@note and similar extension tags are known to this inspection and won't cause false positives).
    Though it would be possible to ignore deprecated elements, they're not. Even if an element is deprecated, it must still be documented properly and without errors.

    Declaration has problems in Javadoc references

    Makes sure that references in Javadoc comments ({@link ...} and possibly @see ...) can be resolved.

    equals() and hashCode() not paired

    Due to their contract, equals() and hashCode() must always be paired. If one of them is overridden, so must be the other.

    Redundant suppression

    Reports usages of warning suppression when they suppress something that isn't there.

    Variable is assigned to itself

    Stuff like a = a; most likely is an error.

    Wrong package statement

    Reports when the package statement doesn't correspond to the project directory structure.

  • Imports

    * import

    * imports are forbidden in Gridarta.

    Import from same package

    java.lang import

    Redundant import

    Redundant or pointless imports are forbidden in Gridarta.

    Static import

    Our code style doesn't allow static import.

    Unused import

    Import statements must not list types they don't use.

  • Inheritance issues

    Abstract method with missing implementations

    The compiler would of course report this in the subclasses that miss the implementation. This inspection supports you while editing or if you're too lazy to perform a build prior to a commit.