WG - Document best practices for code quality tools

[gsmet]

In https://groups.google.com/g/quarkus-dev/c/Rb-WAzCNuVw, we discussed about how we could document Quarkus best practices and ultimately transform as a set of Sonar rules (obviously, they could be integrated in all sorts of tooling).

The goal of this working group is to come up with this set of best practices.

I suggest the following process:

• we start a thread here for each best practice we want to discuss
• we make sure all the discussion takes place in this thread
• once approved, we document the best practice in the description of this discussion
• finally, we will extract these best practices to a document (ADR, Markdown file...)

Approved best practices

...WIP...

[geoand]

we make sure all the discussion takes place in this thread

Yes please, let's try and consolidate the various opinions that have been thrown around on disparate channels :)

finally, we will extract these best practices to a document (ADR, Markdown file...)

Definitely +1 for an ADR - this will allow us to easily refer to the rules in the future

[edeandrea]

Should we separate rules/practices for extensions vs applications? For example, some rules may only make sense if the code is for an extension instead of within an application.

[geoand]

I think initially we should focus on applications as the number of extension authors is rather limited.

[edeandrea]

I've captured what was in https://quarkusio.zulipchat.com/#narrow/stream/187038-dev/topic/Quarkus.20Anti-Patterns into this discussion. Feel free to add additional context to anything, or to add new proposals.

[edeandrea]

Proposal: Prefer @ApplicationScoped over @Singleton

Unless there is a good reason not to.

Our guides recommend this as well. @Singleton's are cheaper with respect to memory usage and do not require default constructors.

Additionally, you can not mock @Singletons, so you will run into issues if you do mocking in your tests.

[ueberfuhr]

... and they are not created lazily, and interceptors do not work for them (I guess). This is all because @Singleton is not a CDI scope, and so the object is not proxied.

[Ladicek]

Interceptors of course do work for @Singleton beans. Interception in Quarkus is implemented by subclassing, which is a different thing to the client proxy that normal scoped beans have.

[edeandrea]

Proposal: Prefer constructor injection over field injection

Field-level injection

@Inject
MyBean myBean

Forces your beans to have package-private scope within a class. Additionally, its difficult when mocking beans that use field injection to know/understand what the dependencies of a bean are.

Using constructor injection

private final MyBean myBean;

public MyClass(MyBean myBean) {
   this.myBean = myBean;
}

is a little more verbose, but it allows proper scoping/encapsulation of dependencies within a class, plus it is explicit as to what is required in order to instantiate/use a class.

[Chealer]

For sure the extra verbosity is sad, but I do not have enough Java expertise to opine.
I would simply propose that no matter what the result is, Quarkus document its conclusion. In other words, if each approach has its advantages, it would still be useful to document the dilemma so that developers reading Quarkus documentation realize they are making a choice.

[mkouba]

So in my opinion, the constructor injection is usually the best choice but field/setter (CDI initializer methods) injection is fine too. I mean, I wrote a lot of CDI beans, tests, integration code, etc. And I don't think we should enforce any kind of injection.

A recommendation is fine but those annoying warnings (IntelliJ, Sonar) are IMO a little bit too much.

NOTE: I was a CDI TCK developer. I developed and let the Weld project. I wrote a lot of CDI integration code (WildFly, Thorntail, SmallRye). And I "shaped" and developed Quarkus ArC. So I might be a little bit biased...

[manovotn]

I am from the same (possibly biased) background as Martin and I need to second his opinion here - I don't see field injection as anyhow inherently bad. It is simply an option - you can use constructor, field or initializer method to inject your values and it is mostly a matter of preference.

CDI handles the object lifecycle so injecting a bean means a user will get their hands on fully initialized object regardless of how they performed the injection.
Constructors also require some extra code, not to mention the need for an extra no-arg constructor for any beans that need client proxies as well (Quarkus can circumvent this to some extent, though not always).

So I am also leaning towards a recommendation but not a warning.

[Chealer]

Thank you very much @mkouba and @manovotn
Regarding extra code for constructors, my colleague O. Bilal suggested using Project Lombok's annotations like RequiredArgsConstructor, as documented on Baeldung.

[edeandrea]

FWIW, and this is just my opinion (but I know its shared by many others)....Lombok is evil...

[edeandrea]

Proposal: Minimize the usage of Lambdas when writing build-time initialization code

@gastaldi Can fill in details (https://quarkusio.zulipchat.com/#narrow/stream/187038-dev/topic/Quarkus.20Anti-Patterns/near/466380624)

@franz1981 feel free to add more context here as well.

They are also difficult to debug when things go wrong.

[edeandrea]

Personally, I feel that using functional APIs with lambdas are cleaner and much easier to read/maintain. I can understand the debugging issue, but I don't quite understand the performance issues.

So I guess I'm not sure if I agree with this proposal or not.

[gsmet]

So the rule here is actually to avoid lambdas for runtime code when you can... in Quarkus proper.

That's something we do in Quarkus itself. But I'm not sure that's something we should generalize to Quarkus projects as it seems a bit weird to raise a warn for lambdas as they are part of modern Java programming.

[geoand]

+1 to what @gsmet says

[mkouba]

This one definitely deserves some dicsussion.

That's something we do in Quarkus itself. But I'm not sure that's something we should generalize to Quarkus projects as it seems a bit weird to raise a warn for lambdas as they are part of modern Java programming.

I think that it does not make sense at all. Especially in reactive/Mutiny-based apps.

First of all, AFAIK we don't have any benchmark that would show the difference between the anonymous classes VS lambdas. And I believe that one of the reasons why we don't have it is that it's not easy to create one. Because there are so many combinations/configurations to test.

Recently, I tried to explain in a zulip discussion that the sentence that was added recently in CONTRIBUTING.MD is not necessarily fully and unconditionally valid.

[Ladicek]

One thing I find particularly funny: the reactive core of Quarkus, Vert.x, contains a ton of lambdas. If they had to use anonymous classes, they would probably be in a lunatic asylum by now. Preferring anonymous classes over lambdas is madness, even if there's some impact on memory. (I was able to measure that with JDK 11, a year or two ago. I didn't measure on newer JDKs, it's possible/likely that cost has shrinked.)

[edeandrea]

Proposal: Never use LinkedList. Use ArrayList instead

[mkouba]

This is IMO an invalid simplification. There are definitely use cases where LinkedList is a valid choice.

If we have some strong opinions here we should explain where exactly should be LinkedList avoided.

[edeandrea]

Proposal: If you mostly have non-reactive endpoints which do most of the work, consider using a single event loop vertx instance, or at least leave half of the cores available to the blocking side

This is especially valid if the load is not very high; it can easily reduce the power consumption by more than 30% and the CPU usage by 15% or more.

From @franz1981: https://quarkusio.zulipchat.com/#narrow/stream/187038-dev/topic/Quarkus.20Anti-Patterns/near/466519026

[maxandersen]

is that even possible to capture/analyze in a static tool analysis?

[franz1981]

We already check how to perform the dispatch at build time - so I suppose it is, based on the return types of the declared end point.

@maxandersen this is what we have talked about in the call ;)

[Serkan80]

Can this be a bit more elaborated ? Does this mean that if you have a service running with low load, that you have to reduce the amount of eventloops to 1 ? If yes, how do you do that ?

[Serkan80]

And what abour resource/memory consumption? Does that also decrease?

[maxandersen]

@franz1981 yes i recognize what it is. Just asking how it would be detected quality checking tools like sonar cube or similar.

[edeandrea]

Proposal: Logging: Use the static Log class and its methods rather than a private static final Logger LOGGER

See https://quarkus.io/guides/logging#simplified-logging

[turing85]

What exactly is the ratio here? I really do not like the fact that Log.<level>(...) does not support the {} notation. And adding lombok's @Slf4j or @Log4j2 is just one depenency away.

[cescoffier]

We should add a rule: Avoid Lombok at all cost :-)

[ueberfuhr]

I don't like that static method. Complicates mocking.

[turing85]

We should add a rule: Avoid Lombok at all cost :-)

Any specific reason for this @cescoffier ?

[geoand]

There are a lot of reasons. This thread by @graemerocher contains a ton of excellent information

[edeandrea]

Proposal: Logging: Don't use log guarding

Something like

if (Log.isDebugEnabled()) {
  Log.debug("Debug something");
}

isn't needed, as the Log.debug method will check isDebugEnabled.

If you need to include something which is expensive to compute, use the variant log methods that take parameters:

Log.debugf("Debug and output something thats expensive to compute: %s", someMethodThatComputesSomethingToOutput());

[gastaldi]

@edeandrea ah my mistake, I confused it with java.util.logging.Logger#log(java.util.logging.Level, java.lang.Throwable, java.util.function.Supplier<java.lang.String>)

[edeandrea]

Unless there is a variable with some kind of Functional Interface, it's going to compute

Intuition and reading the code would imply it would, but I thought I remembered something from a while back where the compiler would optimize and defer the call to the method until the value it produces was required.

Of course I could be totally wrong.

[gsmet]

It's recommended to use the String format approach to avoid concatenating a string if you don't need to. But if your method call is a parameter of the method, it will be called for sure.

That being said, in most cases you're using variables.

So you can drop the isDebugEnabled if you don't use string format or if the parameters are all variables.

[edeandrea]

But even if the parameters are variables, how were they created? If they are simple objects, sure, but if they involve using resources, then you should probably guard the creation of the variables?

[mkouba]

If they are simple objects, sure, but if they involve using resources, then you should probably guard the creation of the variables?

That's right. When the creation of the log message is expensive (no matter what the reason is) then it's better to guard the log method. The problem is that you need to keep those two methods (isDebugEnabled() and debug) synchronized in the sense of log level.

[edeandrea]

Proposal: Logging: Don't use String concatenation in log messages. Use proper format methods.

Don't do:

Log.info("value: " + someValue);

Instead, do:

Log.infof("value: %s", someValue);

[turing85]

Do we need this rule? IIRC, sonarqube already has this. Not sure if it works for Log.<level>(...) though...

[geoand]

Another benefit of doing this work, is that we can point IDE devs to the source of truth and they can update their software accordingly.

[ueberfuhr]

Question: How about test logging?

Should I mock the logger (verification is against special methods / white box) or should I configure the logger to log in-memory and make my assertion against the in-memory data?

[Serkan80]

Not a fan of it. Especially if you are going to validate the log message, which can break easily many times by each change of the message.

And to be honest, most of the time I write IT tests over unit tests with Quarkus.

[edeandrea]

If you want to go down the road of writing tests based on what gets logged, I would recommend using an in-memory logger. Quarkus already has something that can help you with this - the InMemoryLogHandler.

I have an example of this in https://github.com/edeandrea/quarkus-todo-apps/blob/main/quarkus-todo-listener/src/test/java/org/acme/TodoCompletionListenerTests.java

[Serkan80]

What about RoutingContext vs Context/UriInfo ?

With Quarkus 3.x the http layer is mapped through Vertx and JAX-RS stuff is not used anymore if I’m not wrong. So RoutingContext is always filled and available. I had occasions when injecting Context gave me null.

Is it a good practice to inject RoutingContext by default then ?

[geoand]

It's possible, it's just not safe. You would need to use Instance<Whatever> instance and make a check.

Even so, using RoutingContext is super risky. In a JAX-RS/Jakarta REST environment you can use UriInfo for such things.

[Serkan80]

@geoand Can we make it clear then when to use each of these objects ?

• in jaxrs resources => UriInfo, Context
• rest client (filters, providers) => RoutingContext ?
• in cdi beans => ???

[geoand]

The rule is very simple actually:

Anything that is only active when an incoming HTTP request is being served, can only be used for the duration of that request.

So types like UriInfo and RoutingContext can only be safely injected into Jakarta REST / JAX-RS classes.
Injecting these types into other CDI beans MUST be done via Instance<Whatever> instance and the instance checked before it is used.

[ueberfuhr]

Not sure if injecting JAX-RS resources into CDI beans would be a bad idea for layered architectures in general.

[geoand]

It's a very bad idea indeed :)