10 common broken rules of clean code

Introduction

From time to time I am asked to do code review of an application. I like doing this, because it is always learning experience. I can see how others program, what problems they have and how they solve them. Except that I can see code written in various conditions and by people with different skills and experience. I prepared a list of 10 popular “clean code” broken rules which I often encounter during this code reviews and I would like to share this list with you.

Note: If you don’t know what clean code means, I recommend to read Robert C. Martin Clean Code book.

1. Bad and inconsistent naming

Naming is very often neglected (especially by less experienced developers) but it shouldn’t. Good naming sometimes is difficult but it is needed for a good level of code redability and maintainability.

2. Too much comments

Comments from line 1, 4, 7, 18 are not needed, becasue code speaks for itself. Comment block from lines 14-16 are not needed too, this code should be removed – our source control will remember everything:).

Comments are needed only in two cases:
– commenting public API.
– when programmer uses some kind of hack and there is no other way to explain this.

In other cases comments are unnecessary and should be avoided. You should use refactor technique called Extract Method instead.

3. Duplication

The same code occurring more than once. If it is not intended, it should be refactored by moving that code to separate class and/or method. See Don’t Repeat Yourself (DRY) principle.

4. Sloppy formatting

Similar to bad naming. It doesn’t affect execution of our code but it affects readability. Some people will say that is minor issue but I will say it is not. What do you think about book which is not formatted good? What do you think about author and editor of this book? In programming we should have the same rules because we are professionals, aren’t we?

5. Too big classes/methods.

Classes and methods should be as small as possible. They should have “only one reason to change” as Single Responsibility Principle says. Common code smells are:
– too much lines of codes
– too many dependencies (constructors/methods parameters)
– code in class concerning different entities, use cases.

6. Bad exceptions handling.

We should always catch the most specific exception ( DivideByZeroException for example above). Moreover, we should be careful when we try rethrow given exception, because we can lost whole stack trace information. Below corrected code:

7. No encapsulation

This is most frequently broken rule of clean code and object oriented design paradigm. I see this almost in every codebase. Every class is public with public properties and public methods. This is not object oriented programming. Our duty is to create fully encapsulated objects and hide theirs internals. This is topic for another post why to do this but for know I beg you – stop revealing your classes internals only because your IDE add public keyword by default.

8. Too many conditional statements

Sometimes, when our bussiness logic is complicated, I see a lot nested if/else statements. This is difficult to read and understand.

Often we can get rid part of this statements applying some design pattern and principle (see The Open/Closed Principle and Strategy Pattern for example). If we can’t, we should decompose these conditionals.

9. Unused code

Unused code is created in 2 cases. First case is when code was changed (due to refactoring or requirements change) and some part of that code is not valid. Second case is when programmer wanted to add some additional functionality which is not required at the time of writing but he thinks it may be needed later.

In both cases unused code should not exist. It decreases redadabilty of code, design, adds unnecessary complexity to them – it should be avoided. There is even programming principle for second case – YAGNI.

10. Magic strings and numbers.

Programming is enough difficult even without magic. Code with meaningless strings and numbers like Option == 1, order.Type = 'A' is difficult to read, maintain and refactor. Instead of this we should use enums and consts and enjoy good readability and compile-time checks.

Summary

I tried to list all most common issues which I often encounter. I think it is important to remember at least two things when we write the code. Firstly, code is more often read than written. Secondly, working code which even fulfills business requirements is not sufficient if it is written badly. Remembering this and knowing rules of writing good quality code will lead us to software craftsmanship faster.

How to publish and handle Domain Events

Introduction

Domain Event is one of the building blocks of Domain Driven Design. It is something that happened in particular domain and it captures memory of it. We create Domain Events to notify other parts of the same domain that something interesting happened and these other parts potentially can react to.

Domain Event is usually immutable data-container class named in the past tense. For example:

Three ways of publishing domain events

I have seen mainly three ways of publishing domain events.

1. Using static DomainEvents class

This approach was presented by Udi Dahan in his Domain Events Salvation post. In short, there is a static class named DomainEvents with method Raise and it is invoked immediately when something interesting during aggregate method processing occurred. Word immediately is worth emphasizing because all domain event handlers start processing immediately too (even aggregate method did not finish processing).

2. Raise event returned from aggregate method

This is approach when aggregate method returns Domain Event directly to ApplicationService. ApplicationService decides when and how to raise event. You can become familiar with this way of raising events reading Jan Kronquist Don’t publish Domain Events, return them! post.

3. Add event to Events Entity collection.

In this way on every entity, which creates domain events, exists Events collection. Every Domain Event instance is added to this collection during aggregate method execution. After execution, ApplicationService (or other component) reads all Eventscollections from all entities and publishes them. This approach is well described in Jimmy Bogard post A better domain events pattern.

Handling domain events

The way of handling of domain events depends indirectly on publishing method. If you use DomainEvents static class, you have to handle event immediately. In other two cases you control when events are published as well handlers execution – in or outside existing transaction.

In my opinion it is good approach to always handle domain events in existing transaction and treat aggregate method execution and handlers processing as atomic operation. This is good because if you have a lot of events and handlers you do not have to think about initializing connections, transactions and what should be treat in “all-or-nothing” way and what not.

Sometimes, however, it is necessary to communicate with 3rd party service (for example e-mail or web service) based on Domain Event. As we know, communication with 3rd party services is not usually transactional so we need some additional generic mechanism to handle these types of scenarios. So I created Domain Events Notifications.

Domain Events Notifications

There is no such thing as domain events notifications in DDD terms. I gave that name because I think it fits best – it is notification that domain event was published.

Mechanism is pretty simple. If I want to inform my application that domain event was published I create notification class for it and as many handlers for this notification as I want. I always publish my notifications after transaction is committed. The complete process looks like this:

1. Create database transaction.
2. Get aggregate(s).
3. Invoke aggregate method.
4. Add domain events to Events collections.
5. Publish domain events and handle them.
6. Save changes to DB and commit transaction.
7. Publish domain events notifications and handle them.

How do I know that particular domain event was published?

First of all, I have to define notification for domain event using generics:

All notifications are registered in IoC container:

In EventsPublisher we resolve defined notifications using IoC container and after our unit of work is completed, all notifications are published:

This is how whole process looks like presented on UML sequence diagram:

You can think that there is a lot of things to remember and you are right!:) But as you can see whole process is pretty straightforward and we can simplify this solution using IoC interceptors which I will try to describe in another post.

Summary

1. Domain event is information about something which happened in the past in modeled domain and it is important part of DDD approach.
2. There are many ways of publishing and handling domain events – by static class, returning them, exposing by collections.
2. Domain events should be handled within existing transaction (my recommendation).
3. For non-trasactional operations Domain Events Notifications were introduced.

Processing commands with Hangfire and MediatR

In previous post about processing multiple instance aggregates of the same type I suggested to consider using eventual consistency approach. In this post I would like to present one way to do this.

Setup

In the beginning let me introduce stack of technologies/patterns:
1. Command pattern – I am using commands but they do not look like theses described in GoF book. They just simple classes with data and they implement IRequest  marker interface of MediatR.

2. Mediator pattern. I am using this pattern because i want to decouple my clients classes (commands invokers) from commands handlers. Simple but great library created by Jimmy Bogard named MediatR implements this pattern very well. Here is simple usage.

and handler:

3. Hangfire. Great open-source library for processing and scheduling background jobs even with GUI monitoring interface. This is where my commands are scheduled, executed and retried if error occured.

Problem

For some of my uses cases, I would like to schedule processing my commands, execute them parallel with retry option and monitor them. Hangfire gives me all these kind of features but I have to have public method which I have to pass to Hangifre method (for example BackgroundJob.Enqueue). This is a problem – with mediator pattern I cannot (and I do not want) pass public method of handler because I have decoupled it from invoker. So I need special way to integrate MediatR with Hangfire without affecting basic assumptions.

Solution

My solution is to have three additional classes:
1. CommandsScheduler – serializes commands and sends them to Hangfire.

2. CommandsExecutor – responods to Hangfire jobs execution, deserializes commands and sends them to handlers using MediatR.

3. MediatorSerializedObject – wrapper class for serialized/deserialized commands with additional properties – command type and additional description.

Finally with this implementation we can change our client clasess to use CommandsScheduler:

and our commands are scheduled, invoked and monitored by Hangfire. I sketched sequence diagram which shows this interaction:

Processing commands with MediatR and Hanfire

Additionally, we can introduce interface for CommandsSchedulerICommandsScheduler. Second implementation will not use Hangfire at all and only will execute MediatR requests directly – for example in development process when we do not want start Hangfire Server.

Summary

I presented the way of processing commands asynchronously using MediatR and Hangfire. With this approach we have:
1. Decoupled invokers and handlers of commands.
2. Scheduling commands mechanism.
3. Invoker and handler of command may be other processes.
4. Commands execution monitoring.
5. Commands execution retries mechanism.

These benefits are very important during development using eventual consistency approach. We have more control over commands processing and we can react quickly if problem will appear.

Processing multiple aggregates – transactional vs eventual consistency

When we use Domain Driven Design approach in our application, sometimes we have to invoke some method on multiple instances of aggregate of the same type.

For example, in our domain we have customers and when big Black Friday campaign starts we have to recalculate theirs discounts. So in domain model exists Customer aggregate with RecalculateDiscount method and in Application Layer we have DiscountAppService which is responsible for this use case.

There are 2 ways to implement this and similar scenarios:

1. Using transactional consistency

This is the simplest solution, we get all customers aggregates and on every instance the RecalculateDiscount method is invoked. We surrounded our processing with TransactionScope so after that we can be certain that every customer have recalculated discount or none of them. This is transactional consistency – it provides us ACID and sometimes is enough solution, but in many cases (especially while processing multiple aggregates in DDD terms) this solution is very bad approach.

First of all, customers are loaded to memory and we can have performance issue. Of course we can change implementation a little, get only customers identifiers and in foreach loop load customers one by one. But we have worse problem – our transaction holds locks on our aggregates until end of processing and other processes have to wait. For the record – default transaction scope isolation level is Serializable. We can change isolation level but we can’t get rid of locks. In this case application becomes less responsive, we can have timeouts and deadlocks – things we should avoid how we can.

2. Using eventual consistency

In this approach we do not use big transaction. Instead of this, we process every customer aggregate separately. Eventual consistency means that in specified time our system wile be in inconsistent state, but after given time will be consistent. In our example there is a time, that some of customers have discounts recalculated and some of them not. Let’s see the code:

In this case on the beginning we got only customers identifiers and we process customer aggregates one by one asynchronously (and parallel if applicable). We removed problem of locking our aggregates for a long time. The simplest solution is usage of Task.Run() , but using this approach we totally losing control of processing. Better solution is to use some 3rd party library like Hangfire, Quartz.NET or messaging system.

Eventual consistency is a big topic used in distributed computing, encountered together with CQRS. In this article I would like to show only another way of executing batch processing using this approach and its benefits. Sometimes this approach is not a good choice – it can have impact on GUI and users may see stale data for some time. That is why it is important to talk with domain experts because often it is fine for user to wait for update of data but sometimes it is unacceptable.

Summary

Transactional consistency – whole processing is executed in one transaction. It is “all or nothing” approach and sometimes can lead to decrease performance, scalability and availability of our application.

Eventual consistency – processing is divided and not executed in one big transaction. In some time application will be in inconsistent state. It leads to better scalability and availability of application. On the other hand can cause problems with GUI (stale data) and it requires supporting mechanisms which enable parallel processing, retries and sometimes process monitors as well.