A couple of months ago, when I was looking for best practices and clean code guidelines, I discovered a strange term coming with a bunch of concepts: object calisthenics. After having a quick glance at it, I decided to give it a shot.
It was absolutely stunning, and I'm really convinced that those rules helped me to write a much more readable, cleaner and overall better code.
That's why I wanted to share those rules and explain them by example, to whom it may interest !
Object calisthe-what ?
Object calisthenics are a set of 9 simple rules focused on maintainability, readability, testability, and comprehensibility of your code.
If you already write a code respecting this principle, object calisthenics may enforce it; if you don't, this is a great starting point !
The advantages of having a small set of rules is that you are free to apply only those you find relevant or only one at the time, to ease your learning curve.
The rules
Enough teasing, here are the rules !
- Only one level of indentation per method
- Don’t use the
elsekeyword - Wrap all primitives and strings
- First class collections
- One dot per line
- Don't abbreviate
- Keep all entities small
- No classes with more than two instance variables
- No getters / setters / properties
This may be a bit abstract for the moment, so let's dive into coding !
Examples
Talking is cheap show me the code !
Rule by rule, let me show you how object calisthenics can help you in practice.
Some keyword and practices only bound to C# will sometimes not be used in order to only illustrate the rules and not C# features
Only one level of indentation per method
Indentation is helpful in most case to remember the nesting of all statements in our logic. However, too much indentation will make the code much harder to read, since you will have to mentally add an additional layer of context in your head for the code you are reading.
For example, here is a small Board object that exposes a method to print all of its cells:
class Board
{
private const int Dimension = 15;
private int[,] cells = new int[Dimension, Dimension];
public string Stringify()
{
var buffer = new StringBuilder();
// 0
for (var i = 0; i < Dimension; ++i)
{
// 1
for (var j = 0; j < Dimension; ++j)
{
// 2
buffer.Append(cells[i, j]);
}
buffer.Append("\n");
}
return buffer.ToString();
}
}
Here, there is up to 2 levels of indentation, which makes the code much less readable for future maintainers.
Let's break it down by using the extract method refactoring technique for each one of those levels:
class Board
{
private const int Dimension = 15;
private int[,] cells = new int[Dimension, Dimension];
public string Stringify()
{
// 0
var buffer = new StringBuilder();
StringifyRows(buffer);
return buffer.ToString();
}
private void StringifyRows(StringBuilder buffer)
{
// 0
for (var i = 0; i < Dimension; ++i)
{
// 1
StringifyRow(buffer, i);
}
}
private void StringifyRow(StringBuilder buffer, int row)
{
// 0
for (var i = 0; i < Dimension; ++i)
{
// 1
buffer.Append(cells[row, i]);
}
buffer.Append("\n");
}
}
Sure, you will not reduce the number of lines, but greatly improve readability.
In some case, this can also help you to improve the testability of your code, by having more atomic methods.
Don’t use the else keyword
Conditionals are among the most common tools in almost every programming language. However, they often weighted the code a lot, not to mention when they are nested.
Let's apply this to a simple login function
public void Login(string username, string password)
{
if (! userRepository.Exists(username))
{
AddError("Unknown user");
Redirect("Login");
}
else if (! userRepository.IsValid(username, password))
{
AddError("Invalid credentials");
Redirect("Login");
}
else
{
Redirect("Profile");
}
}
Keeping all the previously invalidated statements in mind when in the else block is pretty simple in this short example. However, try to transpose it into a bigger and growing code base: this can easily become a huge mess.
Keep also in mind that testing this method will not be easy either: you will have to remember all the path that your code may or may not take.
To improve it, we can apply the early return pattern. The name is pretty explicit: when you met a condition that is decisive, return the appropriate value.
You can either be optimistic (happy path first) or defensive (failing paths first). Let's see how it can be applied to our method !
public void Login(string username, string password)
{
if (! userRepository.Exists(username))
{
AddError("Unknown user");
return Redirect("Login");
}
if (! userRepository.IsValid(username, password))
{
AddError("Invalid credentials");
return Redirect("Login");
}
return Redirect("Profile");
}
If you have to manipulate the result, you can differ the return, statement at the end of your method, and introduce a variable to hold the value to be returned.
Wrap all primitives and strings
This one is pretty easy. It aims to reduce the primitive obsession code smell. The goal is to use meaningful types instead of raw primitives.
The good question to ask yourself here is "is my type holding more information than juste its value ?". As an example, when you have to handle a vehicle registration plate, you may do the following for a function:
public bool IsValidNumberPlate(string numberplate)
{
// Code goes here
}
However, you will lose a bunch of information and context. Worst, you do not have any control on how your numberplate is structured. A better approach would be by wrapping this value into a dedicated type:
class NumberPlate
{
private char[] part1 = new char[2];
private int[] part2 = new int[3];
private char[] part3 = new char[2];
public NumberPlate(/* ...*/ ) { /* ... */ }
public override string ToString()
=> string.Join(string.Empty, part1) + " "
+ string.Join(string.Empty, part2) + " "
+ string.Join(string.Empty, part3);
}
You can now access extra information for your method, and use a more explicit signature:
public bool IsValidNumberPlate(NumberPlate numberplate)
{
// Code goes here
}
Of course, using a property or a class method would be a better approach to expose the validation of our object
Using a dedicated type can also help you to apply tests on your type to detect flows in its usage. Business rules also became more straightforward and easier to read / understand.
First class collections
This rule is very straightforward:
Any class that contains a collection should contain no other member variables.
Let's break it down !
When having at least one collection in you object, most of the time it means that you are dealing with a custom collection.
The problem is that when using your inner-collection along with other member variables, you will probably mix both the "collection logic" and the object logic.
Once again, let's have a small look at an example.
In this case, we would like to manage our candy shop. To do so, we have a CandyCollection but with specific rules. As we are still a child, our parent is looking for the stock and if we have too much candy, they will throw some. A naive
way of implementing this may be the following:
For the sakes of this example, I am not testing against any edge case
public class CandyShop
{
public const int Price = 1;
private const int ParentThreshold = 5;
private readonly List<string> _candies = new List<string>();
private int _moneyCount = 0;
public void SellCandy(int amount)
{
_candies.RemoveRange(0, amount);
_moneyCount += amount * Price;
}
public void AddCandy(string candyName)
{
if (_candies.Count == ParentThreshold)
{
return;
}
_candies.Add(candyName);
}
}
As you may have noticed, mixing the collection and my business logic here is forcing me to handle the logic of both in each method. As an example, in AddCandy, I have to write and maintain a logic that does not belong to a candy shop at all, but rather to my stock.
Now, let's apply the rule, and refactor everything related to the collection in a dedicated class:
public class CandyShop
{
public const int Price = 1;
private int _moneyCount = 0;
private readonly CandyStock _stock = new CandyStock();
public void SellCandy(int amount)
{
_stock.RemoveCandies(amount);
_moneyCount += amount * Price;
}
public void AddCandy(string candyName)
=> _stock.AddCandy(candyName);
}
public class CandyStock
{
private const int ParentThreshold = 5;
private readonly List<string> _candies = new List<string>();
public void RemoveCandies(int count)
=> _candies.RemoveRange(0, count);
public void AddCandy(string candyName)
{
if (_candies.Count == ParentThreshold)
{
return;
}
_candies.Add(candyName);
}
}
Now each class is only caring about what it is really responsible of: the shop of its money, and the stock of its rules and its collection.
As a result, you may enforce the Single Responsibility Principle but also improve the reusability of your logic. Moreover, this become pretty handy in OOP to apply the appropriate visibility modifiers (you business logic class may be public, but its inner collection private, to hide its logic from the consumer)
One dot per line
This is more like "keep in mind the law of Demeter": Only talk to your immediate friends.
Let me show you:
public class Location
{
public Piece Current;
}
public class Piece
{
public string Representation;
}
public class Board
{
private readonly Location _location;
// Some more methods
public string ShowCurrentPieceInLower()
=> Console.WriteLine(_location.Current.Representation.ToLower());
}
This line in ShowCurrentPieceInLower looks awful.
Not only we are exposing too much information from our classes (it looks like some of those should be private), but we are also violating the Single Responsibility Principle by manipulating a field from Piece and operating on it from a class that has no direct link with it.
A better way to handle this would be reworking the visibility of our class members, implementing and exposing the appropriate methods in the appropriate classes.
Something like:
public class Location
{
private Piece Current;
public void ShowCurrentInLower()
=> Console.WriteLine(Current.GetLowerRepresentation())
}
public class Piece
{
private string Representation;
public string GetLowerRepresentation()
=> Representation.ToLower();
}
public class Board
{
private readonly Location _location;
// Some more methods
public string ShowCurrentPieceInLower()
=> Console.WriteLine(_location.ShowCurrentInLower());
}
Don't abbreviate
Abbreviations might seems handy when having a function or a class that has its name repeating itself all over again. It may also be useful when it's too long.
In my example, I do not want a huge name when writing those:
namespace MyApp
{
public abstract class MyClass
{
abstract void EvaluateSumAndPrintResultInConsole(int[] numbers);
abstract void CreatePersonFromName(Person person, string name);
abstract void CreatePersonFromLocation(Person p, Location location);
}
}
but rather
namespace MyApp
{
public abstract class MyClass
{
abstract void Sum2Console(int[] numbers);
abstract void CreatePFromName(Person person, string name);
abstract void CreatePFromLocation(Person p, Location location);
}
}
Wrong !
If you ever happen to be in this situation, it is most likely another underlying problem and you should avoid it.
In my example, naming my methods this way is showing me two major problems:
- I'm having a method that violates the Single Responsibility Principle (doing the calculation and the logging)
- I'm having code duplication and I should rework my architecture
In fact, for EvaluateSumAndPrintResultInConsole, I should break this in (at least) two major components: one dedicated to operation on my collection (however, remember: first class collections !) and one dedicated to the display. Patterns or not, this logic should not be mixed in a single method.
For CreatePersonFromName and CreatePersonFromLocation I'm most likely in a scenario of code duplication, and the creation of the Person object should be delegated to its base class. Moreover, operations on it should also be part of the class logic.
Those are two specific use cases, however, keep in mind that having abbreviation is almost always a red flag and may show some flaws in your application's design.
Business abbreviation are awful too, you sure do not want to make your codebase a little bit more cryptic along with your business rules, think about an intern that may come to the project in a year of two !
Keep all entities small
I can't provide an example for this one unless making this article completely unreadable, however, let's have an imaginary example.
You have just being hired in a new company, and come to work on your very first project. It has quite some legacy already and many many classes and files. The business logic is not very easy to understand either, so, you decide to open the code and try to figure it out.
You're browsing the project tree, and open a file that seems to be pretty easy as an entrypoint: CustomerService.cs.
You then see the scrolling bar shrinking, and your editor displaying you more than 1K lines of code. As you are seing this, a coworker comes to see you and ask you to add a small feature in the order processing logic.
What a mess !
The idea behind this rule is that the longer are your file, the less they will be easy to read, understand and maintain.
I also find huge class appealing for developers because they usually already have everything included, which make any addition very easy in it.
If you are not careful enough, you may skip the core OOP principles and slowly deteriorate your code base.
A good goal is to keep every entity, and all files in general, under a certain limit. This could be 50 lines for a scripting language, 150 for a more verbose one, more with comments, whatever suit you the most !
No classes with more than two instance variables
This arbitrary number of two is for you to be aware of keeping a high cohesion and a better encapsulation of you objects.
The idea behind it is that they are two type of classes:
- The ones that maintain the state of an instance variable
- The ones that coordinate other variables
As an example for the first one, you could think of a counter
public class Counter
{
public int count { get; private set; } = 0;
public void AddCount()
=> ++count;
}
As described, this counter is minimal and to expand its logic, you will have to create a higher component with a more specific logic and with an instance of this counter.
For the coordination of two members, you may think about any class that would not fit in the previous category.
No getters / setters / properties
In many cases, getters and setters may wreck the encapsulation of your logic. Some are even saying that getter and setters are evil.
Let's take our candy shop again to see why:
public class CandyShop
{
public int Money { get; set; } = 0;
}
Well, if designed this way, I am free to set my money count from any class that has access to an instance of CandyShop. As a competitor, I could easily withdraw money and as the manager, easily have as much money as I want to (which completely violate the Open-Close principle).
So, how do we fix it ? It's pretty simple and can be rephrased as: Tell, don't ask.
Your class should expose ways of interacting with it, but maintain its logic. Trivially applied to our candy shop, we may modify it as so:
public class CandyShop
{
public int Money { get; private set; } = 0;
public void BuyLollipop(int count)
=> Money -= Constants.LollipopBuyingPrice * count;
public void SellLollipop(int count)
=> Money += Constants.LollipopSellingPrice * count;
}
This way, no one can extend and tweak our business logic (of course, you may want to check for negative numbers, and other edge cases)
There is also one behavior that you should be aware of: any decision in another class depending solely of a publicly exposed value of another class is a bad choice. If this value is so much important, then the associated logic should belong to the class exposing the property, not outside of it.
Criticism
Although all the rules tend to improve your code, some may just be a burden in certain scenarios. Remember to be flexible and chose whether or not enforcing them will be relevant !
As an example, I often broke the No classes with more than two instance variables rule, especially in API controllers. I'm always injecting at least the logger, which take one of the two allowed instances. If you are also using a metric reported or any other service, you "consumed" all the class members without fitting in either of the categories aforementioned.
I personally think that breaking rules is fine, as long as it is well thought and deliberated, but I would love to hear your opinion about it !
Sources
- Object Calisthenics by William Durand
- Your code sucks, let's fix it !
Photo by Michael D Beckwith on Unsplash
Top comments (0)