Screaming Architect

Having nothing better to do on a rainy Sunday afternoon, I though I would catch up with the CleanCode CaseStudy Project, see how it’s structured and what its connection to Clean Architecture and other concepts from Uncle Bob is.

Here is what I found…

Let’s see the Code

After cloning the project from github, the source code is easy enough to find in the directory src. It’s not a standard Maven layout, which one would perhaps expect with a pom.xml included, but not a big deal.

The first application specific packages are under the root package “cleancoderscom“, which is again, a deviation from standard Java package naming. Although somewhat irritating, not really an issue.

So the first packages are:

• entities
• gateways
• http
• socketserver
• usecases
• view

Let’s stop right here.

According to Screaming Architecture, I expected to find some clues here about the nature of the application. Some business terms what this application is about, what its “theme” or purpose is. A good top-level design should tell the reader something about the business and not technical details.

Let’s see: “entities”, “gateways”, “usecases”, “view” are completely generic terms, so let’s ignore those for the moment. The “http” and “socketserver” packages sound less like part of the business and more like supporting infrastructure stuff. So this thing is maybe a web application?

But I still don’t know what it is about.

Top Architecture

To get a new perspective on how these top level packages interact, let’s enlist the help of a dependency analyzer:

There are no circular dependencies, which is a good thing. Curiously however, the root package, which has two classes in it, is not on the top of the dependency graph.

Java packages should normally only depend on more abstract packages “above”, or at most the same level as themselves. Packages should never depend on a sub-package, because that implies that the more abstract package is depending on some specific implementation.

Anyway, that means that the root package is not the most abstract package, “entities” is. Maybe we should start there.

Entities

The entities package contains the following classes:

• Codecast
• Entity
• License
• User

Ok, now we are getting somewhere. This application is about Codecasts, Licenses and Users. A quick glance at the code reveals that these are actually not interfaces or abstract classes, which is curious, since the rest of the application seems to depend on them. In general, the more things depend on a class, the more abstract the class needs to be according to Uncle Bob’s own package metrics.

I assume that the class “Entity” is generic, because it has a generic name. Let’s continue there and figure out why it’s here:

package cleancoderscom.entities;
import java.util.Objects;

public class Entity implements Cloneable {
   private String id;

   public boolean isSame(Entity entity) {
      return id != null && Objects.equals(id, entity.id);
   }

   public void setId(String id) {
      this.id = id;
   }

   public String getId() {
      return id;
   }

   public Object clone() throws CloneNotSupportedException {
      return super.clone();
   }
}

Well, it’s not declared abstract, so maybe it’s a thing of it’s own, or somebody just forgot to mark it abstract. It’s apparently something that may have an id. It obviously does not always have an id, because it can be constructed without.

I do not yet know what part of the business domain this class represents, or what id means, because there is nothing here that would tell me. This class does not want to speak to me, so let’s try finding references to this class and look for clues there.

To my absolute surprise, there are no references to this class or its methods anywhere in the code other than the three other entities which inherit from it. It is either a useless abstraction, or somebody accidentally left it in after a refactoring.

But wait, there is a reference to it in the test code, in a class called GatewayUtilities, which looks suspiciously like an in-memory storage, with methods like: “save()“, “delete()“, and “getEntities()“. Because it’s just test code, let’s not dwell on the naming of this class, but let’s look inside and see how the Entities are used, for example in the save() method:

public T save(T entity) {
   if(entity.getId() == null)
      entity.setId(UUID.randomUUID().toString());
   String id = entity.getId();
   saveCloneInMap(id, entity);
   return entity;
}

Hmm, that looks like the id is managed here, which means it doesn’t have any “business” relevant meaning, it exists just for the sole purpose of helping with persistence. Hey, that’s cheating! Aren’t the “business” objects supposed to be completely decoupled from persistence according to clean architecture? All the entities have this id, which means they definitely do know about persistence and quite specifically what kind of information the persistence code would need from them. This is not a clean separation at all.

The other entities are all Anemic Objects, meaning they have no logic in them, just public data. This makes them impossible to understand, since I don’t know what they are for, what part of the “business” they represent, what they supposed to do.

There is no logic in this package whatsoever, so let’s continue with the package “gateway”, that depends on the classes here.

Gateways

There are three classes here:

• CodecastGateway
• LicenseGateway
• UserGateway

One “Gateway”, whatever that is, for each entity. Let’s look at CodecastGateway first:

package cleancoderscom.gateways;

import cleancoderscom.entities.Codecast;
import java.util.List;

public interface CodecastGateway {
   List findAllCodecastsSortedChronologically();

   void delete(Codecast codecast);

   Codecast save(Codecast codecast);

   Codecast findCodecastByTitle(String codecastTitle);

   Codecast findCodecastByPermalink(String permalink);
}

That looks suspiciously like a persistence or storage class. I don’t know why it’s called “Gateway”, but I will make a mental note that Gateways equal Storage.

The other ones look the same, with a “save()” and “find*()” methods. It is slightly curious that these are interfaces, but depend on the entities, which are concrete classes. It seems completely backwards, as normally concrete implementations should depend on interfaces, not the other way around.

Unfortunately this package also does not contain anything relevant to the business domain, in fact it seems purely technical. There is no other place to hide the business logic now, but the “usecases” package, so let’s have a look at that.

Usecases

There are two packages here:

• codecastDetails
• codecastSummaries

This looks like some business-related naming. It seems that “Codecasts” are the central concept which this application is about and more specifically it is concerned with “Summaries” and “Details” of these. I could imagine that Users and Licenses are a way to control access to these summaries or details. This anwsers some basic questions about the application, but let’s continue and see how it works.

The “details” depend on the “summaries”, so let’s look at the latter:

• CodecastSummariesController.java
• CodecastSummariesInputBoundary.java
• CodecastSummariesOutputBoundary.java
• CodecastSummariesPresenter.java
• CodecastSummariesResponseModel.java
• CodecastSummariesUseCase.java
• CodecastSummariesViewImpl.java
• CodecastSummariesView.java
• CodecastSummariesViewModel.java
• CodecastSummary.java

It seems we are back to technical classes. This does not seem like anything the business would know or care about. Even if you are not a fan of Domain-Driven Design and don’t want to have a “Ubiquitous Language”, I would still expect classes to have at least something, even if just tangentially to do with the business and its language.

Of these classes 3 are interfaces and 7 are concrete classes. Let’s have a look at each of the concrete classes:

• CodecastSummary: This is a structure. I mean it does not even try to pretend to be an object (like a Bean), it just has public fields. No logic.
• CodecastSummariesViewModel: Mix of structure and a bean. No logic whatsoever.
• CodecastSummariesViewImpl: This is the actual business logic of the application I think. Displaying the summary of a Codecast. It is however only one method, the bulk of which is in a static method and the object has no state. It is a procedure. Something that could be a static utility method somewhere and it would make no difference at all to the design.
• CodecastSummariesUseCase: A procedure (one method with no object state) to get all the Codecasts out of a singleton (!) “Gateway” (Storage) and load it into a “presenter”.
• CodecastSummariesResponseModel: A bean. No logic.
• CodecastSummariesPresenter: A purely technical object that shovels data out of the summary and into a “view model”. It has two methods, “getViewModel()” and “present(model)“, of which “present(model)” has to be called first otherwise “getViewModel()” returns null. So there is a temporal coupling, which is generally considered a code smell.
• CodecastSummariesController: A technical object with one method that triggers the “use case” and “view” objects above.

So this means out of 10 classes, only 2 actually contain something that has anything to do with the “business”, the rest are just boilerplate technical classes. Even in the 2 classes that do something, the logic is basically in a procedure, a thing that just gets data from a static source (a singleton, or other static objects), does something with it, and pushes it somewhere else.

This design is as far from object-orientation as anything can be while still using a (semi-)object-oriented language. The structures and procedures in there could have been easily written in C (a non-object-oriented language) and it would have made no difference at all to the design.

That in itself does not mean however that it is useless. A good architecture is measured by its maintainability, not blind adherence to some theory, so let’s try to excerise this design. Let’s pick a change that is more than just adjusting the color or format of something, but still would be expected to be easy to do in an architecture adhering to the principle of Separation of Concerns and the Single Responsibility Principle.

Maintainability

Let’s say the next business requirement is to add a “Description” to the “Codecast”. We want to tell the users more about the Codecast in the Summary to move them to buy it. What would this change involve?

The obvious place to start with the implementation is the Codecast entity, in which the “description” needs to be added:

public class Codecast extends Entity {
   private String title;
   private String description; // This is the new field
   private Date publicationDate = new Date();
   private String permalink;

    ...getters/setters...
}

Unfortunately all the code compiles after this change, so I have to use the “find references” function of my IDE to get any idea where this field might be of interest. I look for “getTitle()“, I assume everywhere that is used, the description has to be used as well.

I find only CodecastSummariesUseCase, so I add the one line to copy the description:

private CodecastSummary summarizeCodecast(Codecast codecast, User user) {
   CodecastSummary summary = new CodecastSummary();
   summary.title = codecast.getTitle();
   summary.description = codecast.getDescription(); // I added this line
   summary.publicationDate = codecast.getPublicationDate();
   summary.permalink = codecast.getPermalink();
   summary.isViewable = isLicensedFor(VIEWING, user, codecast);
   summary.isDownloadable = isLicensedFor(DOWNLOADING, user, codecast);
   return summary;
}

That does not compile, CodecastSummary has to be adjusted:

public class CodecastSummary {
   public String title;
   public String description; // I added this line
   public String permalink;
   public Date publicationDate;
   public boolean isViewable;
   public boolean isDownloadable;
}

The code compiles again. The data from the Codecast was shoveled into the Summary structure, so let’s find usages of that.

There is one the CodecastSummariesPresenter in which I have to copy the description field again:

private ViewableCodecastSummary makeViewable(CodecastSummary codecastSummary) {
   final ViewableCodecastSummary summary = new ViewableCodecastSummary();
   summary.title = codecastSummary.title;
   summary.description = codecastSummary.description; // Added
   summary.permalink = codecastSummary.permalink;
   summary.publicationDate = dateFormat.format(codecastSummary.publicationDate);
   summary.isDownloadable = codecastSummary.isDownloadable;
   summary.isViewable = codecastSummary.isViewable;
   return summary;
}

The code does not compile of course, because I have to add the “description” to the ViewableCodecastSummary too. That is actually a static inner class of CodecastSummariesViewModel:

public static class ViewableCodecastSummary {
   public String title;
   public String description; // Added
   public String permalink;
   public String publicationDate;
   public boolean isDownloadable;
   public boolean isViewable;
}

It all compiles again. I guess I have to look for usages of this structure now. It is in CodecastSummariesViewImpl. This is where the HTML page is generated from the Codecast data:

...
   ViewTemplate codecastTemplate = ViewTemplate.create("html/codecast.html");
   codecastTemplate.replace("title",
      viewableCodecastSummary.title);
   codecastTemplate.replace("description",
      viewableCodecastSummary.description); // Added
   ...
...

That’s it! Other than modifying the entity itself, I had to change 5 classes out of the total 10 for the summary “use case”. So assuming a real application with many “use cases”, the number of classes one would have to touch for any changes on a single “business model” entity is:

1 + (dependant usecases) * 5

And that is assuming you can track down all of the uses and consolidate them to be somewhat consistent. After all, there might be “use cases” that use data differently, interpret boolean flags in the model differently, etc.

All in all, not a very maintainable setup.

Summary

This analysis of the Clean Code Case Study project uncovered some surprising results:

• The architecture is not “Screaming” at all. It could be argued it is the opposite, hiding behind a mostly technical organization of packages and classes.
• Dependencies between packages somewhat jumbled, not adhering to best practices.
• Using a strict separation between data and logic, which means the code is not object-oriented.
• Persistence is not actually separated from the “domain” even though it is claimed this architecture does that.
• Uses static methods, singletons and other anti-patterns.
• Uses a lot of unnecessary boilerplate.
• Technical aspects dominate the codebase instead of business-related concepts.
• Is not maintainable if the domain changes.

I am not sure whether this project is a fair representation of Uncle Bob’s style of programming, Screaming Architecture, Clean Architecture, Separation of Concerns, SOLID, and others. Maybe it’s just a tool to demonstrate separate specific concepts and not meant to be taken as some reference architecture for any of the above principles.

It is however highly visible on GitHub and therefore might be taken literally regardless of real intent. It is therefore important to point out that it has deep structural flaws (listed above) and should not be taken as a blueprint for any real projects, especially not for Object-Oriented ones.