Don’t stop writing code comments

TL;DR: The older I get, the more diverse projects I have seen, the more often I refrain from giving someone an absolute advice. In almost any cases, there are many aspects to consider. In times when people hardly read documentation and often only headlines, an advice like “Stop Writing Code Comments” guarantees some clicks, but isn’t absolutely for the good nor appropriate in all scenarios.

At the beginning of this week, @java tweeted this:

Pointing to this blog post Stop Writing Code Comments. Brian seems to be a big apologist of the clean code movement. I can understand this to some extends, but moved away from “keep methods as small as possible” and similar quiet some extend. Sometimes it’s just necessary and as long as the logic in a bigger method stays testable, I don’t see an issue. And apart from that, what’s dirty code anyway?

What and how to comment

But I want to address some of the methods in the Medium post above. The name of the method is indeed good. But imagine the following:

/**
 * Finds all the employees with a given status. The method will return all employees with a given status, regardless
 * when they joined the company. The returned list will be immutable, changes to the employee objects will not be tracked.
 * 
 * @param status The employment status of the employees to find. Must not be null.
 * @return A list of employees with the given status or an empty list of no employee has the given status
 * @throws IllegalArgumentException when the status is literal null
 */
List<Employee> getEmployeesByStatus(Status status) {
  ...
}

As a user of that method I know now exactly what to pass to the method and what I can expect to return. I have an idea that the list may be empty, but is never null.

The employee class is badly named, that’s nothing to fix with a comment indeed. But chose a better naming and add some meaningful comment and things look bright:

/**
 * A person that is currently employed at the company. Instances of this class are not attached to the database
 * and represents a current employee purley from the business domain and can be savey passed around services.
 *
 * For other types of employees, see {@link RitiredEmployee} or {@link FiredEmployee}
 */
public class CurrentEmployee {
}

I do actually like this part:

public void sendPromotionEmailToUsers() {
  calculatePrices();
  compareCalculatedPricesWithSalesPromotions();
  checkIfCalculatedPricesAreValid();
  sendPromotionEmail();
}

There’s nothing much to add here. But let’s focus on sendPromotionEmail. It’s intentions are pretty clear, but I think it can be improved:

/**
 * Sends an email to all users eligible for a promotion. It does not include any retry mechanism, an e-mail is considered
 * sent when the configured SMTP server acknowledged the e-mail
 * An exception is thrown when the SMTP server is not reachable.
 *
 * When no users are eligible for promotion, no emails are sent
 * @see #getEmployeesByStatus
 * @throws RuntimeException when the SMTP Server could not be contacted
 */
void sendPromotionEmail() {
}

By coincidence I found this paper on The Use Of Sub-Routines In Programmes by David Wheeler. My friend Lukas quoted it in his talk 10 Reasons Why we Love Some APIS and Why we Hate Some Others. The paper – being from 1952 – summarises things neatly:

[…] sub-routines are very useful – although not absolutely necessary – and that the prime objectives to be born in mind when constructing them are simplicity of use, correctness of codes and accuracy of description. […]

Neither small and grokable methods nor comments alone help to create an understandable software system. One needs both.

On the obsolesce of comments

Brian speaks a lot about comments that becomes invalid and outdated. In my opinion, comments just needs to be treated as deliverable artefact as well and needs to be refactored with the code being documented. It’s easy as that.

Useless comments

I wholeheartedly agree with large chunks of commented code: Remove it, please! That’s what a version control system is for.

Regarding TODO comments: I use them occasionally to remind me of things I need to come back to in the future, which would block me from more important things at the very moment.
However, one alternative solution to that is to write failing tests: Those reminds you much better of things to fix.

Title photo by Marc Schäfer on Unsplash

Filed in English posts | Comments (2) »

04-Jun-19

Separation of concerns in Spring applications

TL;DR: Don’t surprise your fellow developers.

Yesterday I learned – or realized again, don’t know – that one can add @CompenenScan to all Spring components and not only to @Configuration components:

It’s also ok to annotated methods of standard components with @Bean and turn them into weird factory components (Those being processed in “light” though, as Marten reminded us.)

I see several issues here, the dilution of concern’s being my primary. I can get a quick overview of all services of an application with my IDE looking for @Service or whatever. The same with configuration. That helps me a lot not to have to think about things.

I spent a considerable amount of time yesterday helping a friend debugging a weird @SpringBootTest. He wanted to exclude some services from being started and only test some. The first impression was: He tried this by specifying them on the classes attribute of @SpringBootTest. That’s not what that attribute does. That attribute is used to point the test context to configuration classes.

To include or exclude components from being started, one uses @ContextConfiguration, also from the test package. It has include and exclude.

So we added this and removed the classes from @SpringBootTest attribute. And: Nothing worked anymore. Wtf? After some digging it turned out, that those services mentioned on that attribute – while clearly named and annotated service – also configured more component scanning and the module did not have a @SpringBootApplication, so the test context couldn’t figure out about the components without being explicitly told to.

Given the fact that the project in question was just a bit older than a year, I think it’s already legacy and not in a good sense: A new developer joining that team would be have a super hard time figuring out what’s happening and why? Things are being named in one way and do something else or something in addition.

Go figure what happens next: It’s always Springs fault, of course.

Well, this time I’d say it’s quite dangerous indeed that all components contribute to the application context and not only @Configuration classes.

Imagine a library, containing some cool services. When you add this library via a custom starter, you would be hopefully aware, that the starter can add many things to your context. You could however be explicit and write down your own @Configuration and return new AwesomeLibraryService() from within a @Bean method. The AwesomeLibraryService could be annotated with an arbitrary @ComponentScan and scan the kitchen sink, the JDK and all the rest, including a blockchain mining library. Probably not what you want.

Anyway, we all are responsible developers, everyone does DDD and writes nice, clean micro services.

My impression of real world projects, especially in big cooperates, is a another. I always stumble upon the same issues:

  • Issues with dependencies
  • Incompatibile versions
  • Cargo cult: A configuration hacked together from various sources is passed on all the time

People are gonna reuse the stuff the find somehow working in projects, whether it’s good or bad.

It’s a seniors developers responsibility to make sure that things have well defined concerns. I am sure that it is impossible to prevent cargo cult, but one can make it a bit safe.

Title photo by Franck V. on Unsplash.

Filed in English posts | Comments (1) »

17-May-19

GraalVM on macOS

I have been doing some research with GraalVM and I noticed that RC14 brings an Info.plist inside it’s contents folder. That’s a nice thing to have, as it automatically registers it if you unzip it into /Library/Java/JavaVirtualMachines as a JDK 8 implementation, but a nasty surprise when using /usr/libexec/java_home -v1.8 to configure your JAVA_HOME, which now point to the GraalVM. Probably it’s not much of a problem, but I stumbled upon it as one of my favourite tools, jQAssistant suddenly stopped working.

The fix is easy and I described it already for JDK 9 ea years ago:

Just delete or rename the info.plist file:

export GRAALVM_HOME=/Library/Java/JavaVirtualMachines/graalvm-ce-1.0.0-rc14/Contents/Home
sudo mv $GRAALVM_HOME/../Info.plist $GRAALVM_HOME/../Info.plist.bak

This way you can have a standard JDK 8 having around that is selected via a standard java_home call and explicitly using GraalVM.

Filed in Apple, Java | Comments (0) »

27-Mar-19

Using Thymeleaf inline JavaScript with D3.js

I gave this talk several times now and every time I got asked how I created this visualization, clustering the genres I’m listening to by decade and country:

I just did a super spontaneous video for that. Key take away: Just use server side rendered HTML through Thymeleaf, obviously running on Spring Boot, render the content model as inlined Javascript and be good to go. Want an API later on?

Structure your Spring MVC controller and your service or repository layer in a way that makes it actually reusable.

Anyway, here’s the short clip and below the relevant code snippets:

Spring MVC controller, GenresController.java, either returning a String, denoting the template to be rendered, or the Model as just the response body:

@GetMapping(value = { "", "/" }, produces = MediaType.TEXT_HTML_VALUE)
public ModelAndView genres() {
 
	var model = genresModel();
	return new ModelAndView("genres", model);
}
 
@GetMapping(value = { "", "/" }, produces = MediaType.APPLICATION_JSON_UTF8_VALUE)
@ResponseBody
public Map<String, Object> genresModel() {
 
	var genres = this.genreRepository.findAll(Sort.by("name").ascending());
	var allSubgrenes = genreRepository.findAllSubgrenes();
	var top10Subgenres = allSubgrenes.stream().sorted(comparingLong(Subgenre::getFrequency).reversed()).limit(10).collect(toList());
	return Map.of(
			"genres", genres,
			"subgenres", allSubgrenes,
			"top10Subgenres", top10Subgenres
	);
}

The Thymeleaf template genres.html. The standard controller above renders it when / is requested, accepting text/html.

<!DOCTYPE html>
<html xmlns="http://www.w3.org/1999/xhtml" xmlns:layout="http://www.ultraq.net.nz/thymeleaf/layout"
      xmlns:th="http://www.thymeleaf.org"
      layout:decorate="~{layout}">
<head>
    <title th:text="#{genres}">Placeholder</title>
    <script th:inline="javascript">
        var subgenres = /*[[${subgenres}]]*/ {};
    </script>
</head>
<body>
<!-- Some more content -->
</body>
</html>

Calling this thing application.js is way too much. It’s a simple function, reading in the global subgenres variable and displays it as a nice D3.js chart.

Remember: This is also no silver bullet and I absolutely don’t have anything agains todays Javascript development. I just feel more at home near a database.

Filed in Uncategorized | Comments (11) »

31-Jan-19