- Coding Conventions
- Naming
- Proper IDE configuration
- Obsolete APIs
- Code-Documentation
- Clean Code
- Logging
- Catching and handling Exceptions
- Prefer general API
- Prefer primitive types
- Constants
- Refactorings
- Optionals
- Avoid catching NPE
- Consider extractig local variable for multiple method calls
- Encoding
- BLOBs
- Stateless Programming
- Closing Resources
- Enclose blocks with curly braces
- Field Initializion
- Lambdas and Streams
The code should follow general conventions for Java (see Oracle Naming Conventions, Google Java Style, etc.). We consider this as common sense instead of repeating this here. The following sections give us additional conventions that we consider additionally.
We follow these additional naming rules:
-
Always use short but speaking names (for types, methods, fields, parameters, variables, constants, etc.).
-
Avoid using existing type names from JDK (from
java.lang.
,java.util.
, etc.) - so e.g. never name your own Java typeList
,Error
, etc. -
Strictly avoid special characters in technical names (for files, types, fields, methods, properties, variables, database tables, columns, constraints, etc.). In other words only use Latin alpahnumeric ASCII characters with the common allowed technical separators for the accordign context (e.g. underscore) for technical names (even excluding whitespaces).
-
For package segments and type names prefer singular forms (
CustomerEntity
instead ofCustomersEntity
). Only use plural forms when there is no singular or it is really semantically required (e.g. for a container that contains multiple of such objects). -
Avoid having duplicate type names. The name of a class, interface, enum or annotation should be unique within your project unless this is intentionally desired in a special and reasonable situation.
-
Avoid artificial naming constructs such as prefixes (
I*
) or suffixes (*IF
) for interfaces. -
Use CamelCase even for abbreviations (
XmlUtil
instead ofXMLUtil
) -
Avoid property/field names where the second character is upper-case at all (e.g. 'aBc'). See #1095 for details.
-
Names of Generics should be easy to understand. Where suitable follow the common rule
E=Element
,T=Type
,K=Key
,V=Value
but feel free to use longer names for more specific cases such asID
,DTO
orENTITY
. The capitalized naming helps to distinguish a generic type from a regular class. -
For
boolean
getter methods useis
prefix instead ofget
but forboolean
variable names avoid theis
prefix (boolean force = …
instead ofboolean isForce = …
) unless the name is a reserved keyword (e.g.boolean abstract = true
will result in a compile error so consider usingboolean isAbstract = true
instead).
Ensure your IDE (IntelliJ, Eclipse, VSCode, …) is properly configured. This is what IDEasy is all about. A reasonable configuration of formatter, save actions, etc. will ensure:
-
no start imports are used (
import java.util.*
) -
no diff-wars in git (if every developer in the team uses the same formatter settings the diffs in git will only show what really changed)
-
import statements are properly grouped and sorted
-
code has properly indentation and formatting (e.g. newlines after opening curly braces)
Please avoid using the following APIs:
-
java.util.Date
- use according type fromjava.time.*
such asLocalDate
,LocalDateTime
, orInstant
-
java.util.Calendar
- same as above -
java.sql.Date
- useLocalDate
-
java.sql.Time
- useLocalTime
-
java.sql.Timestamp
- useInstant
orLocalDateTime
-
java.io.File
- usejava.nio.file.Path
-
java.nio.file.Paths.get(…)
- usejava.nio.file.Path.of(…)
-
java.util.Vector
- useList
andArrayList
orLinkedList
-
java.lang.StringBuffer
- usejava.lang.StringBuilder
-
java.lang.Runtime.exec(…) - use `ProcessBuilder
(we useProcessContext
on top) -
com.google.common.base.Objects
- usejava.util.Objects
As a general goal, the code should be easy to read and understand. Besides, clear naming the documentation is important. We follow these rules:
-
APIs (especially component interfaces) are properly documented with JavaDoc.
-
JavaDoc shall provide actual value - we do not write JavaDoc to satisfy tools such as checkstyle but to express information not already available in the signature.
-
We make use of
{@link}
tags in JavaDoc to make it more expressive. -
JavaDoc of APIs describes how to use the type or method and not how the implementation internally works.
-
To document implementation details, we use code comments (e.g.
// we have to flush explicitly to ensure version is up-to-date
). This is only needed for complex logic. -
Avoid the pointless
{@inheritDoc}
as since Java 1.5 there is the@Override
annotation for overridden methods and your JavaDoc is inherited automatically even without any JavaDoc comment at all.
The book Clean Code gives a collection of helpful rules and guidelines for coding. A summary can be found here.
Most important explicit aspects are:
Properly use logging to give feedback for various purposes.
In general Java code, SLF4J is the way to go.
However, in our IDEasy project we typically have access to IdeLogger
via context
.
Here is a generic example showing logging in action:
public class MyClass {
private static final Logger LOG = LoggerFactory.getLogger(MyClass.class);
private boolean doSomething(Data data) {
LOG.trace("Trying to do something on {}", data);
boolean success = false;
try {
something();
LOG.info("Successfully did something");
success = true;
} catch (Exception e) {
LOG.error("Failed to do something: {}", e.getMessage(), e);
}
return success;
}
// ...
}
With SLF4J we have to create the logger via LoggerFactory
.
In IDEasy for commandlets, etc. we do not need to define LOG
and can simply use this.context
instead of LOG
.
Level | Type | Meaning |
---|---|---|
|
Standard |
Only for real errors that should raise the end-users attention. If an error is logged something went wrong and action needs to be taken and usually the operation failed. |
|
Standard |
For warnings when something is not correct and the end-user should have a look. E.g. if something is misconfigured. Unlike error the process can continue and may hopefully success. |
|
Proprietary |
For interaction with the end-user. Typically for questions the end-user needs to answer (use dedidcated |
|
Proprietary |
For steps of advanced processing. Allows to divide some processing into logical steps (use |
|
Standard |
Only used for debugging. Disabled by default to avoid "spammming" the end-user. Can be enabled with |
|
Standard |
Only used for very fine grained details. Disabled by default to avoid "spammming" the end-user. Can be enabled with |
The Log-Levels with type Proprietary
only exist in IdeLogger
for allowing different syntax coloring for these specific use-cases.
When logging messages (especially on debug
or trace
) you should always use {}
syntax for dynamic values:
LOG.trace("Trying to do something on {}", data); // good
/*
LOG.trace("Trying to do something on " + data); // bad
LOG.trace(String.format("Trying to do something on %s", data)); // bad
*/
When catching exceptions always ensure the following:
-
Never call
printStackTrace()
method on an exception -
Either log or wrap and re-throw the entire catched exception. Be aware that the cause(s) of an exception is very valuable information. If you loose such information by improper exception-handling you may be unable to properly analyse production problems what can cause severe issues.
-
If you wrap and re-throw an exception ensure that the catched exception is passed as cause to the newly created and thrown exception.
-
If you log an exception ensure that the entire exception is passed as argument to the logger (and not only the result of
getMessage()
ortoString()
on the exception).
-
try {
doSomething();
} catch (Exception e) {
// bad
throw new IllegalStateException("Something failed");
}
This will result in a stacktrace like this:
Exception in thread "main" java.lang.IllegalStateException: Something failed
at com.devonfw.tools.ide.ExceptionHandling.main(ExceptionHandling.java:14)
As you can see we have no information and clue what the catched Exception
was and what really went wrong in doSomething()
.
Instead always rethrow with the original exception:
try {
doSomething();
} catch (Exception e) {
// fine
throw new IllegalStateExeception("Something failed", e);
}
Now our stacktrace will look similar to this:
Exception in thread "main" java.lang.IllegalStateException: Something failed
at com.devonfw.tools.ide.ExceptionHandling.main(ExceptionHandling.java:14)
Caused by: java.lang.IllegalArgumentException: Very important information
at com.devonfw.tools.ide.ExceptionHandling.doSomething(ExceptionHandling.java:23)
at com.devonfw.tools.ide.ExceptionHandling.main(ExceptionHandling.java:12)
Never do this severe mistake to lose this original exception cause!
The same applies when logging the exception:
try {
doSomething();
} catch (Exception e) {
// bad
LOG.error("Something failed: " + e.getMessage());
}
Instead include the full exception and use your logger properly:
try {
doSomething();
} catch (Exception e) {
// fine
LOG.error("Something failed: {}", e.getMessage(), e);
}
Also please add contextual information to the message for the logger or the new exception. So instead of just saying "Something failed" a really good example could look like this:
LOG.error("An unexpected error occurred whilst downloading the tool {} with edition {} and version {} from URL {}.", tool, edition, version, url, e);
Avoid unnecessary strong bindings:
-
Do not bind your code to implementations such as
Vector
orArrayList
instead ofList
-
In APIs for input (=parameters) always consider to make little assumptions:
-
prefer
Collection
overList
orSet
where the difference does not matter (e.g. only useSet
when you require uniqueness or highly efficientcontains
) -
consider preferring
Collection<? extends Foo>
overCollection<Foo>
whenFoo
is an interface or super-class
-
In general prefer primitive types (boolean
, int
, long
, …) instead of corresponding boxed object types (Boolean
, Integer
, Long
, …).
Only use boxed object types, if you explicitly want to allow null
as a value.
Typically you never want to use Boolean
but instead use boolean
.
// bad
public Boolean isEmpty {
return size() == 0;
}
Instead always use the primitive boolean
type:
// fine
public boolean isEmpty {
return size() == 0;
}
Literals and values used in multiple places that do not change, shall be defined as constants.
A constant in a Java class is a type variable declared with the modifiers static final
.
In an interface, public static final
can and should be omitted since it is there by default.
public class MavenDownloader {
// bad
public String url = "https://repo1.maven.org/maven2/"
public void download(Dependency dependency) {
String downloadUrl = url + dependency.getGroupId() + "/" + dependency.getArtifactId() + "/" dependency.getVersion() + "/" + dependency.getArtifactId() + "-" + dependency.getVersion() + ".jar";
download(downloadUrl);
}
public void download(String url) { ... }
}
Here url
is used as a constant however it is not declared as such.
Other classes could modify the value (MavenDownloader.url = "you have been hacked";
).
Instead we should better do this:
public class MavenDownloader {
// fine
/** The base URL of the central maven repository. */
public static final String REPOSITORY_URL = "https://repo1.maven.org/maven2/"
public void download(Dependency dependency) {
String artifactId = dependency.getArtifactId();
String version = dependency.getVersion();
String downloadUrl = REPOSITORY_URL + dependency.getGroupId().replace(".", "/") + "/" + artifactId + "/" + version + "/" + artifactId + "-" + version + ".jar";
download(downloadUrl);
}
public void download(String url) { ... }
}
As stated above in case of an interface simply omit the modifiers:
public interface MavenDownloader {
// fine
/** The base URL of the central maven repository. */
String REPOSITORY_URL = "https://repo1.maven.org/maven2/"
void download(Dependency dependency);
void download(String url);
}
So we conclude:
-
we want to use constants to define and reuse common immutable values.
-
by giving the constant a reasonable name, we make our code readable
-
following Java best-practices constants are named in
UPPER_CASE_WITH_UNDERSCORES
syntax -
by adding JavaDoc to the constant we give additional details what this value is about and good for.
-
In classes we declare the constant with the visibility followed by the keywords
static final
. -
In interfaces, we omit all modifiers as they always default to
public static final
for type variables.
Do refactorings with care and follow these best-practices:
-
use
git mv «old» «new»
to move or rename things in git. Otherwise your diff may show that a file has been deleted somewhere and another file has been added but you cannot see that this file was moved/renamed and what changed inside the file. -
do not change Java signatures like in a text editor but use refactoring capabilities of your IDE. So e.g. when changing a method name, adding or removing a parameter, always use refactoring as otherwise you easily break references (and JavaDoc references will not give you compile errors so you break things without noticing).
-
when adding paramaters to methods, please always consider to keep the existing signature and just create a new variant of the method with an additional parameter.
Lets assume we have this method:
public void doSomething() {
// ...
}
Now, assuming this method is called from multiple places, this change is bad:
// bad
public void doSomething(boolean newFlag) {
// ...
}
The reason is that it is most likely causing a lot of merge conflicts for feature-branches and PRs of other developers, currently working with code calling doSomething()
that will not work after the change.
Instead keep the existing signature and add a new one:
// fine
public void doSomething() {
doSomething(false);
}
public void doSomething(boolean newFlag) {
// ...
}
Typically, you should design flags such that false
is a reasonable default.
That is why we are passing false
in the example from the existing method to the new one.
With Optional
you can wrap values to avoid a NullPointerException
(NPE).
However, it is not a good code-style to use Optional
for every parameter or result to express that it may be null.
For such case use JavaDoc (or consider @Nullable
or even better instead annotate @NotNull
where null
is not acceptable).
However, Optional
can be used to prevent NPEs in fluent calls (due to the lack of the elvis operator):
Long id;
id = fooCto.getBar().getBar().getId(); // may cause NPE
id = Optional.ofNullable(fooCto).map(FooCto::getBar).map(BarCto::getBar).map(BarEto::getId).orElse(null); // null-safe
Please avoid catching NullPointerException
:
// bad
try {
variable.getFoo().doSomething();
} catch (NullPointerException e) {
LOG.warning("foo was null");
}
Better explicitly check for null
:
// fine
Foo foo = null;
if (variable != null) {
foo = variable.getFoo();
}
if (foo == null) {
LOG.warning("foo was null");
} else {
foo.doSomething();
}
Please note that the term Exception
is used for something exceptional.
Further creating an instance of an Exception
or Throable
in Java is expensive as the entire Strack has to be collected and copied into arrays, etc. causing significant overhead.
This should always be avoided in situations we can easily avoid with a simple if
check.
Calling the same method (cascades) multiple times is redundant and reduces readability and performance:
// bad
Candidate candidate;
if (variable.getFoo().getFirst().getSize() > variable.getFoo().getSecond().getSize()) {
candidate = variable.getFoo().getFirst();
} else {
candidate = variable.getFoo().getSecond();
}
The method getFoo()
is used in 4 places and called 3 times.
Maybe the method call is expensive?
// fine
Candidate candidate;
Foo foo = variable.getFoo();
Candidate first = foo.getFirst();
Candidate second = foo.getSecond();
if (first.getSize() > second.getSize()) {
candidate = first;
} else {
candidate = second;
}
Please note that your IDE can automatically refactor your code extracting all occurrences of the same method call within the method body to a local variable.
Encoding (esp. Unicode with combining characters and surrogates) is a complex topic. Please study this topic if you have to deal with encodings and processing of special characters. For the basics follow these recommendations:
-
Whenever possible prefer unicode (UTF-8 or better) as encoding.
-
Do not cast from
byte
tochar
(unicode characters can be composed of multiple bytes, such cast may only work for ASCII characters) -
Never convert the case of a String using the default locale. E.g. if you do
"HI".toLowerCase()
and your system locale is Turkish, then the output will be "hı" instead of "hi", which can lead to wrong assumptions and serious problems. If you want to do a "universal" case conversion always explicitly use an according western locale (e.g.toLowerCase(Locale.US)
). Consider using a helper class (see e.g. CaseHelper) or create your own little static utility for that in your project. -
Write your code independent from the default encoding (system property
file.encoding
) - this will most likely differ in JUnit from production environment-
Always provide an encoding when you create a
String
frombyte[]
:new String(bytes, encoding)
-
Always provide an encoding when you create a
Reader
orWriter
:new InputStreamReader(inStream, encoding)
-
Avoid using byte[]
for BLOBs as this will load them entirely into your memory.
This will cause performance issues or out of memory errors.
Instead, use streams when dealing with BLOBs (InputStream
, OutputStream
, Reader
, Writer
).
When implementing logic as components or beans, we strongly encourage stateless programming.
This is not about data objects (e.g. JavaBeans) that are stateful by design.
Instead this applies to things like IdeContext
and all its related child-objects.
Such classes shall never be modified after initialization.
Methods called at runtime (after initialization) do not assign fields (member variables of your class) or mutate the object stored in a field.
This allows your component or bean to be stateless and thread-safe.
Therefore it can be initialized as a singleton so only one instance is created and shared accross all threads of the application.
Ideally all fields are declared final
otherwise be careful not to change them dynamically (except for lazy-initializations).
Here is an example:
public class GitHelperImpl implements GitHelper {
// bad
private boolean force;
@Overide
public void gitPullOrClone(boolean force, Path target, String gitUrl) {
this.force = force;
if (Files.isDirectory(target.resolve(".git"))) {
gitPull(target);
} else {
gitClone(target, gitUrl);
}
}
private void gitClone(Path target, String gitUrl) { ... }
private void gitPull(Path target) { ... }
}
As you can see in the bad
code fields of the class are assigned at runtime.
Since IDEasy is not implementing a concurremt multi-user application this is not really critical.
However, it is best-practice to avoid this pattern and generally follow thread-safe programming as best-practice:
public class GitHelperImpl implements GitHelper {
// fine
@Overide
public void gitPullOrClone(boolean force, Path target, String gitUrl) {
if (Files.isDirectory(target.resolve(".git"))) {
gitPull(force, target);
} else {
gitClone(force, target, gitUrl);
}
}
private void gitClone(boolean force, Path target, String gitUrl) { ... }
private void gitPull(boolean force, Path target) { ... }
}
Resources such as streams (InputStream
, OutputStream
, Reader
, Writer
) or generally speaking implementations of AutoClosable
need to be handled properly.
Therefore, it is important to follow these rules:
-
Each resource has to be closed properly, otherwise you will get out of file handles, TX sessions, memory leaks or the like.
-
Where possible avoid to deal with such resources manually.
-
In case you have to deal with resources manually (e.g. binary streams) ensure to close them properly via
try-with-resource
pattern. See the example below for details.
Closing streams and other such resources is error prone. Have a look at the following example:
// bad
try {
InputStream in = new FileInputStream(file);
readData(in);
in.close();
} catch (IOException e) {
throw new IllegalStateException("Failed to read data.", e);
}
The code above is wrong as in case of an IOException
the InputStream
is not properly closed.
In a server application such mistakes can cause severe errors that typically will only occur in production.
As such resources implement the AutoCloseable
interface you can use the try-with-resource
syntax to write correct code.
The following code shows a correct version of the example:
// fine
try (InputStream in = new FileInputStream(file)) {
readData(in);
} catch (IOException e) {
throw new IllegalStateException("Failed to read data.", e);
}
In Java curly braces for blocks can be omitted if there is only a single statement:
// bad
if (condition())
doThis();
else
doThat();
While this is not really wrong it can lead to problems e.g. when adding a statement:
// bad
if (condition())
doThis();
else
doThat();
System.err.println("that");
Now, it gets hard to see that the last statement is always executed independent of the condition. Maybe that should actually go only to the else block as we can guess from the indentation. If you always use curly braces this cannot happen and the code is easier to read:
// fine
if (condition()) {
doThis();
} else {
doThat();
System.err.println("that");
}
//System.err.println("that");
Non-static fields should never been initialized outside of the constructor call.
First of all even Java developers with many years of experience will not see anything wrong with such code:
public class MyClass {
private String name = null; // bad
}
However, when inheritance comes into play you can easily get tricked by Java internals that average developers are not aware of. To understand the problem and why such assignments are bad code-style you need to understand what the Java compiler makes out of such code and when it gets executed. So let us look at the following example code (and lets not discuss if this example code makes much sense or not):
public class MyClass {
private String message;
public MyClass(String name) {
this.message = computeMessage(name);
}
protected String computeMessage(String name) {
return "Hi " + name;
}
@Override
public String toString() {
return this.message;
}
public static class MySubClass extends MyClass {
private String name = null;
private String firstName = null;
public MySubClass(String firstName, String lastName) {
super(firstName + " " + lastName);
this.firstName = firstName;
}
@Override
protected String computeMessage(String name) {
this.name = name;
return super.computeMessage(name);
}
public String getName() {
return this.name;
}
}
public static void main(String[] args) {
MySubClass subClass = new MySubClass("John", "Doe");
System.out.println(subClass.getName());
System.out.println(subClass.toString());
}
}
Now a average Java developer would expect this code to print the following output:
John Doe
Hi John Doe
One could assume this since during the constructor call the overridden computeMessage
method is invoked that assignes the name
variable to John Doe
.
However, the output of this program is actually this:
null
Hi John Doe
So why is this happening?
The reason is that the Java compiler takes all field assignment statements and puts them in your constructor after the super
call and before any other following constructor statements.
So your constructor will actually look like this:
public MySubClass(String firstName, String lastName) {
super(firstName + " " + lastName);
this.name = null; // automatically inserted here by javac
this.firstName = null; // automatically inserted here by javac
this.firstName = firstName;
}
So if you would have written the code yourself in this way instead of assigning the fields directly when declared, you would more easily understand what is going wrong.
Since the name
field is properly assigned within the super
call the assignment of name
to the value null
overrides this resulting in the actual program output.
However, you can imagine how easily you can be tricked by such behavior and waste hours debugging your code until you find such problem.
To avoid this, we recommend to never initialize non-static fields outside the constructor.
If you further want to be more strict, then also avoid calling non-final methods from constructor calls.
With Java8 you have cool new features like lambdas and monads like (Stream
, CompletableFuture
, Optional
, etc.).
However, these new features can also be misused or led to code that is hard to read or debug.
To avoid pain, we give you the following best practices:
-
Learn how to use the new features properly before using. Developers are often keen on using cool new features. When you do your first experiments in your project code you will cause deep pain and might be ashamed afterwards. Please study the features properly. Even Java8 experts still write for loops to iterate over collections, so only use these features where it really makes sense.
-
Streams shall only be used in fluent API calls as a Stream can not be forked or reused.
-
Each stream has to have exactly one terminal operation.
-
Do not write multiple statements into lambda code:
// bad collection.stream().map(x -> { Foo foo = doSomething(x); ... return foo; }).collect(Collectors.toList());
This style makes the code hard to read and debug. Never do that! Instead, extract the lambda body to a private method with a meaningful name:
// fine collection.stream().map(this::convertToFoo).collect(Collectors.toList());
-
Do not use
parallelStream()
in general code (that will run on server side) unless you know exactly what you are doing and what is going on under the hood. Some developers might think that using parallel streams is a good idea as it will make the code faster. However, if you want to do performance optimizations talk to your technical lead (architect). Many features such as security and transactions will rely on contextual information that is associated with the current thread. Hence, using parallel streams will most probably cause serious bugs. Only use them for standalone (CLI) applications or for code that is just processing large amounts of data. -
Do not perform operations on a sub-stream inside a lambda:
set.stream().flatMap(x -> x.getChildren().stream().filter(this::isSpecial)).collect(Collectors.toList()); // bad set.stream().flatMap(x -> x.getChildren().stream()).filter(this::isSpecial).collect(Collectors.toList()); // fine
-
Only use
collect
at the end of the stream:set.stream().collect(Collectors.toList()).forEach(...) // bad set.stream().peek(...).collect(Collectors.toList()) // fine
-
Lambda parameters with Types inference
(String a, Float b, Byte[] c) -> a.toString() + Float.toString(b) + Arrays.toString(c) // bad (a,b,c) -> a.toString() + Float.toString(b) + Arrays.toString(c) // fine Collections.sort(personList, (Person p1, Person p2) -> p1.getSurName().compareTo(p2.getSurName())); // bad Collections.sort(personList, (p1, p2) -> p1.getSurName().compareTo(p2.getSurName())); // fine
-
Avoid Return Braces and Statement
a -> { return a.toString(); } // bad a -> a.toString(); // fine
-
Avoid Parentheses with Single Parameter
(a) -> a.toString(); // bad a -> a.toString(); // fine
-
Avoid if/else inside foreach method. Use Filter method & comprehension
// bad static public Iterator<String> TwitterHandles(Iterator<Author> authors, string company) { final List result = new ArrayList<String> (); foreach (Author a : authors) { if (a.Company.equals(company)) { String handle = a.TwitterHandle; if (handle != null) result.Add(handle); } } return result; }
// fine public List<String> twitterHandles(List<Author> authors, String company) { return authors.stream() .filter(a -> null != a && a.getCompany().equals(company)) .map(a -> a.getTwitterHandle()) .collect(toList()); }