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

matrix_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:

cccs-top-dependencies

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

alien

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

stargate

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.

12 thoughts on “Screaming Architect

  1. Nice article, I had similar thoughts after looking at that source code.
    “the help of a dependency analyzer” – is it some tool or IDE plugin?

    Like

  2. Thank you for posting. I have a few comments that can make you happier about the example code.

    1, Entities
    1.1. About the number of abstract classes in the entity package: I think entity classes don’t have to be abstract, if the dependency is from low-level to high-level. An abstract class, interface and polymorphism can be used to inverse the dependency, so high-level package can call the low-level implementation without depending on them. In this case, the package entity is far from user IO, thus it’s high-level. Depending on them makes sense. But I agree with you on that the class Entity may be declared abstract, but it’s not a big deal, because Entity class isn’t used for polymorphism anyway.

    1.2. About GatewayUtilities: I don’t think having ID field means that they “know” about persistence. At least, the business rule doesn’t know them by depending on it. Entity classes having ID means that they are identifiable by ID. Those assumptions are in the Entity test code. It tests isSame method, and when IDs are same, they are the same entities. Entity doesn’t know if those entities will be stored in-memory (a logical memory address, index of an array, tree-map, hash-map, etc), database, file system, network, etc. Maybe it’s not going to be stored at all.

    In this case, the test code (low-level) knows that entities have IDs, but entities doesn’t know anything about the user (caller) of entity classes. Maybe how you understand the word “know” in the context is different from how I understand.

    1.3. I agree with you on that those Anemic Objects in entities package are hard to understand. I don’t think entities should be anemic at all. In this example case, maybe they didn’t have any logic to put there.

    2. Gateway
    2.1. Interface can depend on concrete classes. I don’t see that’s a problem at all. Concrete classes depends on interfaces when the concrete classes are implementing the interface, unless the language supports duck typing. Entities package does not implement this interface in the gateway.

    2.2. In this case, gateway is closer to user IO than entities, so it’s natural for the gateway to depend on entities.

    3. Usecases
    3.1. Mostly agree on your points. I think those singletons should have been passed instead.

    4. Maintenability
    4.1. I think that could be done the opposite way. What we want is to show the description on certain web pages. We don’t “want” the entity to have description field. For example, if the description can be generated on the fly (with AI or by Googling, etc), and we may not need to change entities.

    So, instead of starting from the entities, we can start from the place closest to the user IO. In this case, we would want to add description, for instance, on a certain web page. And then we may easily find where we need to change, thanks to the compiler complaints. Entities will be modified at last only when we really need to change it. This way, we don’t need to find references using the IDE, the compiler will find it.

    What you’ve done was hard because it was done the opposite way.

    4.2. Don’t you feel the power of the Clean Architecture? We have so many chances to “stop” keep modifying the higher level packages in this architecture. Let’s think about a case that we want the old-fashioned web page background for codecasts that were published a long time ago. Do we start by adding a boolean or URL field for old-fashioned background in the entity classes? No. We’ll start modifying from the lowest level (closest to IO) and stop modifying somewhere. The entity is least likely to change.

    4.3. It’s true that this architecture makes it harder to change entities, because it’s in a deep dependency graph. What’s the purpose of modification? Changing the entities is not the purpose of change. No client would ask “please change the entity”, but they would say “please change the view (or whatever close to them)”. With this architecture, we less likely need to change the entities.

    Like

    1. Thanks for commenting. I could find myself agreeing with your first points (it all depends on the circumstances), I think it all comes down to the last two. Maintainability is where a good design gets separated from a bad one.

      I really don’t “feel the power” of the Clean Architecture, indeed I positively think it’s a bad idea for most circumstances (maybe not all). What is *possible* doesn’t really interest me as much as what is *probable*.

      What is the probability of *any* change in this architecture to stay in a single class/package/module? Since everything is separated on technical grounds, basically only technical details stay localized. There is one exception, the UseCases have business-logic, but if any change would need some new data, or some other composition of data it can’t stay localized.

      How often do business people request a technical detail to change? Not very often.

      Taking your example: If a business person wants some change in the presentation, how often is it a detail that doesn’t require new data at all? I would say rarely. Even if that data doesn’t need to be in the Entity, you have a pretty simple change-request and you already have to touch multiple classes, multiple packages, even multiple layers which many projects seem to have in different modules even.

      Like

      1. I don’t understand where this is going when you say “What is the probability of *any* change in this architecture to stay in a single class/package/module?”
        I mean, I can’t imagine any software architecture that ensures that when a new requirement needs new data and those data should be displayed out to an output interface (a web page or whatever you want), the modification will be enclosed to a single class, package or module.
        We can say that our architecture isn’t good if we need to modify many different or unrelated logical parts of our app even if the change requested involve just one of them.
        For example, suppose we want to add the last modification date to all our entities and suppose that the date should not be displayed (we add the date just for security purpose). In this case, I can say that the architecture is very good if this modification requires just changing the entities, and not so bad if I need to change entities and DB persistence logic (is clearly quite better if the DB layer is totally unaware of entity shape, but if not is not a big deal, as these two concepts could be quite coupled).
        Instead, if I need to change entities, DB Layer and some UI part (imagine for example an app where the UI display directly the entities and do some sort of reflection to show all field into a web page) I can say that is a BAD architecture.
        But if the request is “add this data to the entity and show it into the web page”, how this modification can (and should) stay in a single class, package or module?

        Like

        1. Objects need to know how to display themselves.

          This is the only way you can localize changes to one business concept. I don’t blame you if you’ve never seen such a design, because every popular design/architecture pattern out there goes in the opposite direction. It is however possible, and it results in a much more maintainable architecture for average business projects.

          Like

          1. Yeah, I know that mantra, but I don’t agree 100%. What do you mean for “display themselves”? So if you are designing a web app, yo code inside the business entities a method “display” that convert that entity to HTML? And what about if later someone asks you to create an API for your application and your entity now have to be sent in JSON format?
            Ok, you can argue that you can use JSON for both webpage and API. And then if I ask you a console interface that displays the entity as a String?

            I mean, I don’t think business entities should know how to display themselves. Instead, in my opinion in a good architecture business entities are aware of display logic, and many presenters should be responsible for “translate” the object to the “displayable” form. Or better, to the output form.

            Am I not right?

            Like

            1. Yes, that’s the way I design my web-apps, and android apps too. Things display themselves. Obviously not directly to HTML for example, but using some abstraction, like returning an abstract Wicket Component. Those in turn can display themselves into HTML + JS + CSS + whatever is needed.

              If someone later asks me to add a JSON API or somethings else, I will think about how that could be incorporated into the design on case-by-case basis. The notion that the “business code” should not have to deal with the UI is completely unrealistic, even though I realize it’s a popular thing.

              I saw this tried multiple times. UIs are not some afterthought. UIs differ and you have to have different models and behavior to accommodate them. Some UIs are interactive, some aren’t. Some can process arbitrary amount of data, some can’t. Some need lazy loading, asynchronous loading, or paged results, etc. Some can search interactively, with suggestions, etc., others can’t. The list goes on. These problems completely disappear if you treat the UI as a proper requirement, just like you would the “business logic” to add two amounts.

              Like

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out /  Change )

Twitter picture

You are commenting using your Twitter account. Log Out /  Change )

Facebook photo

You are commenting using your Facebook account. Log Out /  Change )

Connecting to %s