SRP as part of SOLID

Clean Code Alliance organized a meetup about SOLID principles.
I had the opportunity to talk about Single Responsibility Principle at part of SOLID.
It’s a presentation I gave several times in the past.

It was fun talking about it.
There were many interesting and challenging questions, which gave me lots of things to think of.

Title:
SRP as part of SOLID

Abstract:
Single Responsibility Principle (SRP), is the part of the SOLID acronym.The SOLID principles help us design better code. Applying those principles helps us having maintainable code, less bugs and easier testing.The SRP is the foundation of having better designed code.In this session I will introduce the SOLID principles and explain in more details what SRP is all about.Applying those principles is not sci-fi, it is real, and I will demonstrate it.
Yesterday I gave a talk in a meetup about the SRP in SOLID.

Bio:
Eyal Golan is a Senior Java developer and agile practitioner. Responsible of building the high throughput, low latency server infrastructure.Manages the continuous integration and deployment of the system. Leading the coding practices. Practicing TDD, clean code. In the path for software craftsmanship.

Following me, Hayim Makabee gave a really interesting talk about The SOLID Principles Illustrated by Design Patterns

Here are the slides.

And the video (in Hebrew)

Thanks for the organizers, Boris and Itzik and mostly for the audience who seemed very interested.

Linkedin Twitter facebook github

Working with Legacy Test Code

Legacy Code and Smell by Tests

Working with unit tests can help in many ways to improve the code-base.
One of the aspects, which I mostly like, is that tests can point us to code smell in the production code.
For example, if a test needs large setup or assert many outputs, it can point that the unit under test doesn’t follow good design, such as SRP and other OOD.

But sometimes the tests themselves are poorly structured or designed.
In this post I will give two examples for such cases, and show how I solved it.

Test Types

(or layers)
There are several types, or layers, of tests.

  • Unit Tests
    Unit test should be simple to describe and to understand.
    Those tests should run fast. They should test one thing. One unit (method?) of work.
  • Integration Tests
    Integration tests are more vague in definition.
    What kind of modules do they check?
    Integration of several modules together? Dependency-Injector wiring?
    Test using real DB?
  • Behavioral Tests
    Those tests will verify the features.
    They may be the interface between the PM / PO to the dev team.
  • End2End / Acceptance / Staging / Functional
    High level tests. May run on production or production-like environment.

Complexity of Tests

Basically, the “higher level” the test, the more complex it is.
Also, the ratio between possible number of tests and production code increase dramatically per test level.
Unit tests will grow linearly as the code grows.
But starting with integration tests and higher level ones, the options start to grow in exponential rate.
Simple calculation:
If two classes interact with each other, and each has 2 methods, how many option should we check if we want to cover all options? And imagine that those methods have some control flow like if.

Sporadically Failing Tests

There are many reasons for a test to be “problematic”.
One of the worst is a test that sometimes fails and usually passes.
The team ignores the CI’s mails. It creates noise in the system.
You can never be sure if there’s a bug or something was broken or it’s a false alarm.
Eventually we’ll disable the CI because “it doesn’t work and it’s not worth the time”.

Integration Test and False Alarm

Any type of test is subject for false alarms if we don’t follow basic rules.
The higher test level, there’s more chance for false alarms.
In integration tests, there’s higher chance for false alarms due to external resources issues:
No internet connection, no DB connection, random miss and many more.

Our Test Environment

Our system is “quasi legacy”.
It’s not exactly legacy because it has tests. Those test even have good coverage.
It is legacy because of the way it is (un)structured and the way the tests are built.
It used to be covered only by integration tests.
In the past few months we started implementing unit tests. Especially on new code and new features.

All of our integration tests inherit from BaseTest, which inherits Spring’s AbstractJUnit4SpringContextTests.
The test’s context wires everything. About 95% of the production code.
It takes time, but even worse, it connects to real external resources, such as MongoDB and services that connect to the internet.

In order to improve tests speed, a few weeks ago I change MongoDB to embedded. It improved the running time of tests by order of magnitude.

This type of setup makes testing much harder.
It’s very difficult to mock services. The environment is not isolated from the internet and DB and much more.

After this long introduction, I want to describe two problematic tests and the way I fixed them.
Their common failing attribute was that they sometimes failed and usually passed.
However, each failed for different reason.

Case Study 1 – Creating Internet Connection in the Constructor

The first example shows a test, which sometimes failed because of connection issues.
The tricky part was, that a service was created in the constructor.
That service got HttpClient, which was also created in the constructor.

Another issue, was, that I couldn’t modify the test to use mocks instead of Spring wiring.
Here’s the original constructor (modified for the example):

private HttpClient httpClient;
private MyServiceOne myServiceOne;
private MyServiceTwo myServiceTwo;

public ClassUnderTest(PoolingClientConnectionManager httpConnenctionManager, int connectionTimeout, int soTimeout) {
	HttpParams httpParams = new BasicHttpParams();
	HttpConnectionParams.setConnectionTimeout(httpParams, connectionTimeout);
	HttpConnectionParams.setSoTimeout(httpParams, soTimeout);
	HttpConnectionParams.setTcpNoDelay(httpParams, true);
	httpClient = new DefaultHttpClient(httpConnenctionManager, httpParams);

	myServiceOne = new MyServiceOne(httpClient);
	myServiceTwo = new MyServiceTwo();
}

The tested method used myServiceOne.
And the test sometimes failed because of connection problems in that service.
Another problem was that it wasn’t always deterministic (the result from the web) and therefore failed.

The way the code is written does not enable us to mock the services.

In the test code, the class under test was injected using @Autowired annotation.

The Solution – Extract and Override Call

Idea was taken from Working Effectively with Legacy Code.

  1. Identifying what I need to fix.
    In order to make the test deterministic and without real connection to the internet, I need access for the services creation.
  2. I will introduce a protected methods that create those services.
    Instead of creating the services in the constructor, I will call those methods.
  3. In the test environment, I will create a class that extends the class under test.
    This class will override those methods and will return fake (mocked) services.

Solution’s Code

public ClassUnderTest(PoolingClientConnectionManager httpConnenctionManager, int connectionTimeout, int soTimeout) {
	HttpParams httpParams = new BasicHttpParams();
	HttpConnectionParams.setConnectionTimeout(httpParams, connectionTimeout);
	HttpConnectionParams.setSoTimeout(httpParams, soTimeout);
	HttpConnectionParams.setTcpNoDelay(httpParams, true);
	
	this.httpClient = createHttpClient(httpConnenctionManager, httpParams);
	this.myserviceOne = createMyServiceOne(httpClient);
	this.myserviceTwo = createMyServiceTwo();
}

protected HttpClient createHttpClient(PoolingClientConnectionManager httpConnenctionManager, HttpParams httpParams) {
	return new DefaultHttpClient(httpConnenctionManager, httpParams);
}

protected MyServiceOne createMyServiceOne(HttpClient httpClient) {
	return new MyServiceOne(httpClient);
}

protected MyServiceTwo createMyServiceTwo() {
	return new MyServiceTwo();
}
private MyServiceOne mockMyServiceOne = mock(MyServiceOne.class);
private MyServiceTwo mockMyServiceTwo = mock(MyServiceTwo.class);
private HttpClient mockHttpClient = mock(HttpClient.class);

private class ClassUnderTestForTesting extends ClassUnderTest {

	private ClassUnderTestForTesting(int connectionTimeout, int soTimeout) {
		super(null, connectionTimeout, soTimeout);
	}
	
	@Override
	protected HttpClient createHttpClient(PoolingClientConnectionManager httpConnenctionManager, HttpParams httpParams) {
		return mockHttpClient;
	}

	@Override
	protected MyServiceOne createMyServiceOne(HttpClient httpClient) {
		return mockMyServiceOne;
	}

	@Override
	protected MyServiceTwo createMyServiceTwo() {
		return mockMyServiceTwo;
	}
}

Now instead of wiring the class under test, I created it in the @Before method.
It accepts other services (not described here). I got those services using @Autowire.

Another note: before creating the special class-for-test, I ran all integration tests of this class in order to verify that the refactoring didn’t break anything.
I also restarted the server locally and verified everything works.
It’s important to do those verification when working with legacy code.

Case Study 2 – Statistical Tests for Random Input

The second example describes a test that failed due to random results and statistical assertion.

The code did a randomize selection between objects with similar attributes (I am simplifying here the scenario).
The Random object was created in the class’s constructor.

Simplified Example:

private Random random;

public ClassUnderTest() {
	random = new Random();
	// more stuff
}

//The method is package protected so we can test it
MyPojo select(List<MyPojo> pojos) {
	// do something
	int randomSelection = random.nextInt(pojos.size());
	// do something
	return pojos.get(randomSelection);
}

The original test did a statistical analysis.
I’ll just explain it, as it is too complicated and verbose to write it.
It had a loop of 10K iterations. Each iteration called the method under test.
It had a Map that counted the number of occurrences (returned result) per MyPojo.
Then it checked whether each MyPojo was selected at (10K / Number-Of-MyPojo) with some kind of deviation, 0.1.
Example:
Say we have 4 MyPojo instances in the list.
Then the assertion verified that each instance was selected between 2400 and 2600 times (10K / 4) with deviation of 10%.

You can expect of course that sometimes the test failed. Increasing the deviation will only reduce the number of false fail tests.

The Solution – Overload a Method

  1. Overload the method under test.
    In the overloaded method, add a parameter, which is the same as the global field.
  2. Move the code from the original method to the new one.
    Make sure you use the parameter of the method and not the class’s field. Different names can help here.
  3. Tests the newly created method with mock.

Solution Code

private Random random;

// Nothing changed in the constructor
public ClassUnderTest() {
	random = new Random();
	// more stuff
}

// Overloaded method
private select(List<MyPojo> pojos) {
	return select(pojos, this.random);
}

//The method is package protected so we can test it
MyPojo select(List<MyPojo> pojos, Random inRandom) {
	// do something
	int randomSelection = inRandom.nextInt(pojos.size());
	// do something
	return pojos.get(randomSelection);
}

Conclusion

Working with legacy code can be challenging and fun.
Working with legacy test code can be fun as well.
It feels really good to stop receiving annoying mails of failing tests.
It also increase the trust of the team on the CI process.

Linkedin Twitter facebook github

Working With Legacy Code, What does it Really Mean

At the end of January I am going to talk in Agile Practitioners 2015 TLV.
I’ll be talking about Legacy Code and how to approach it.

As the convention’s name implies, we’re talking practical stuff.

So what is practical in working with legacy code?
Is it how to extract a method? Or maybe it’s how to introduce setter for a static singleton?
Break dependency?
There are so many actions to make while working on legacy code.

But I want to stop for a minute and think.
What does it mean to work on legacy code?
How do we want the code to be after the changes?
Why do we need to change it? Do we really need to change it?

Definition
Let’s start with the definition of Legacy Code.
If you search the web you will see definitions such as “…Legacy code refers to an application system source code type that is no longer supported…” (from: techopedia)

People may think that legacy code is old, patched.

The definitions above are correct (old, patched, un-maintained, etc.), but I think that the definition coined by Michael Feathers (Working Effectively with Legacy Code) is better.
He defined legacy code as

Code Without Tests

I like to add that legacy code is usually Code that cannot be tested.
So basically, if 10 minutes ago, I wrote code which is not tested, and not testable, then it’s already Legacy Code.

Questioning the Code
When approaching code (any code), I think we should ask ourselves the following questions constantly.

  • What’s wrong with this code?
  • How do we want the code to be?
  • How can I test this piece of code?
  • What should I test?
  • Am I afraid to change this part of code?

Why Testable Code?
Why do we want to test our code?

Tests are the harness of the code.
It’s the safety net.

Imagine a circus show with trapeze. There’s a safety net below (or mattress)
The athletes can perform, knowing that nothing harmful will happen if they fall (well, maybe their pride).

Recently I went to an indie circus show.
The band was playing and a girl came to do some tricks on a high rope.
But before she even started, she fixed a mattress below.

And this is what working with legacy code is all about:
Put a mattress before you start doing tricks…
Or, in our words, add tests before you work / change the legacy code.

Think about it, the list of questions above can be answered (or thought of) just by understanding that we need to write tests to our code.
Once you put your safety net, your’re not afraid to jump.
⇒ once you write tests, you can add feature, fix bug, refactor.

Conclusion
In this post I summarized what does it mean to work with legacy code.
It’s simple:
working with legacy code, is knowing how to write tests to untested code.

The crucial thing is, understanding that we need to do that. Understanding that we need to invest the time to write those tests.
I think that this is as important as knowing the techniques themselves.

In following post(s) I will give some techniques examples.

A girl is doing trapeze with a mattress below

A girl is doing trapeze with a mattress below

Linkedin Twitter facebook github

Law of Demeter

Reduce coupling and improve encapsulation…

General
In this post I want to go over Law of Demeter (LoD).
I find this topic an extremely important for having the code clean, well-designed and maintainable.

In my experience, seeing it broken is a huge smell for bad design.
Following the law, or refactoring based on it, leads to much improved, readable and more maintainable code.

So what is Law of Demeter?
I will start by mentioning the 4 basic rules:

Law of Demeter says that a method M of object O can access / invoke methods of:

  1. O itself
  2. M’s input arguments
  3. Any object created in M
  4. O’s parameters / dependencies

These are fairly simple rules.

Let’s put this in other words:
Each unit (method) should have limited knowledge about other units.

Metaphors
The most common one is: Don’t talk to strangers

How about this:
Suppose I buy something at 7-11.
When I need to pay, will I give my wallet to the clerk so she will open it and get the money out?
Or will I give her the money directly?

How about this metaphor:
When you take your dog out for a walk, do you tell it to walk or its legs?

Why do we want to follow this rule?

  • We can change a class without having a ripple effect of changing many others.
  • We can change called methods without changing anything else.
  • Using LoD makes our tests much easier to construct. We don’t need to write so many ‘when‘ for mocks that return and return and return.
  • It improves the encapsulation and abstraction (I’ll show in the example below).
    But basically, we hide “how things work”.
  • It makes our code less coupled. A caller method is coupled only in one object, and not all of the inner dependencies.
  • It will usually model better the real world.
    Take as an example the wallet and payment.

Counting Dots?
Although usually many dots imply LoD violation, sometimes it doesn’t make sense to “merge the dots”.
Does:
getEmployee().getChildren().getBirthdays()
suggest that we do something like:
getEmployeeChildrenBirthdays() ?
I am not entirely sure.

Too Many Wrapper Classes
This is another outcome of trying to avoid LoD.
In this particular situation, I strongly believe that it’s another design smell which should be taken care of.

As always, we must have common sense while coding, cleaning and / or refactoring.

Example
Suppose we have a class: Item
The item can hold multiple attributes.
Each attribute has a name and values (it’s a multiple value attribute)

The simplest implementations would be using Map.

Let’s have a class ItemsSaver that uses the Item and attributes:
(please ignore the unstructured methods. This is an example for LoD, not SRP 🙂 )

Suppose I know that it’s a single value (from the context of the application).
And I want to take it. Then the code would look like:

I think that it is clear to see that we’re having a problem.
Wherever we use the attributes of the Item, we know how it works. We know the inner implementation of it.
It also makes our test much harder to maintain.

Let’s see an example of a test using mock (Mockito):
You can see imagine how much effort it should take to change and maintain it.

We can use real Item instead of mocking, but we’ll still need to create lots of pre-test data.

Let’s recap:

  • We exposed the inner implementation of how Item holds Attributes
  • In order to use attributes, we needed to ask the item and then to ask for inner objects (the values).
  • If we ever want to change the attributes implementation, we will need to make changes in the classes that use Item and the attributes. Probably a-lot classes.
  • Constructing the test is tedious, cumbersome, error-prone and lots of maintenance.

Improvement
The first improvement would be to ask let Item delegate the attributes.

And the test becomes much simpler.

We are (almost) hiding totally the implementation of attributes from other classes.
The client classes are not aware of the implementation expect two cases:

  1. Item still knows how attributes are built.
  2. The class that creates Item (whichever it is), also knows the implementation of attributes.

The two points above mean that if we change the implementation of Attributes (something else than a map), at least two other classes will need to be change. This is a great example for High Coupling.

The Next Step Improvement
The solution above will sometimes (usually?) be enough.
As pragmatic programmers, we need to know when to stop.
However, let’s see how we can even improve the first solution.

Create a class Attributes:

And the Item that uses it:

(Did you noticed? The implementation of attributes inside item was changed, but the test did not need to. This is thanks to the small change of delegation.)

In the second solution we improved the encapsulation of Attributes.
Now even Item does not know how it works.
We can change the implementation of Attributes without touching any other class.
We can make different implementations of Attributes:
– An implementation that holds a Set of values (as in the example).
– An implementation that holds a List of values.
– A totally different data structure that we can think of.

As long as all of our tests pass, we can be sure that everything is OK.

What did we get?

  • The code is much more maintainable.
  • Tests are simpler and more maintainable.
  • It is much more flexible. We can change implementation of Attributes (map, set, list, whatever we choose).
  • Changes in Attribute does not affect any other part of the code. Not even those who directly uses it.
  • Modularization and code reuse. We can use Attributes class in other places in the code.

Using Reflection for Testing

I am working on a presentation about the ‘Single Responsibility Principle’, based on my previous post.
It take most of my time.

In the meantime, I want to share a sample code of how I use to test inner fields in my classes.
I am doing it for a special case of testing, which is more of an integration test.
In the standard unit testing of the dependent class, I am using mocks of the dependencies.

The Facts

  1. All of the fields (and dependencies in our classes are private
  2. The class do not have getters for its dependencies
  3. We wire things up using Spring (XML context)
  4. I wan to verify that dependency interface A is wired correctly to dependent class B

One approach would be to wire everything and then run some kind of integration test of the logic.
I don’t want to do this. It will make the test hard to maintain.

The other approach is to check wiring directly.
And for that I am using reflection.

Below is a sample code of the testing method, and the usage.
Notice how I am catching the exception and throws a RuntimeException in case there is a problem.
This way, I have cleaner tested code.

The Single Responsibility Principle

Introduction
In this post I would like to cover the Single Responsibility Principle (SRP).
I think that this is the basis of any clean and well designed system.

What is SRP?
The term was introduced by Robert C. Martin.
It is the ‘S’ from the SOLID principles, which are the basis for OOD.
http://en.wikipedia.org/wiki/SOLID_(object-oriented_design)
Here’s the PDF paper for SRP by Robert C. Martin https://docs.google.com/file/d/0ByOwmqah_nuGNHEtcU5OekdDMkk/

From Wikipedia:

…In object-oriented programming, the single responsibility principle states that every class should have a single responsibility, and that responsibility should be entirely encapsulated by the class. All its services should be narrowly aligned with that responsibility….

From Clean Code:

A class or module should have one, and only one, reason to change.

So if a class (or module) needs to be modified for more than one reason, it does more than one thing. I.e. has more than one responsibility.

Why SRP?

  • Organize the code
    Let’s imagine a car mechanic who owns a repair shop.
    He has many many tools to work with. The tools are divided into types; Pliers, Screw-Drivers (Phillips / Blade), Hammers, Wrenches (Tubing / Hex) and many more.

    How would it be easier to organize the tools?
    Few drawers with different types in each one of them?
    Or, many small drawers, each containing a specific type?

    Now, imagine the drawer as the module. This is why many small modules (classes) are more organized then few large ones.

  • Less fragile
    When a class has more than one reason to be changed, it is more fragile.
    A change in one location might lead to some unexpected behavior in totally other places.
  • Low Coupling
    More responsibilities lead to higher coupling.
    The couplings are the responsibilities.
    Higher coupling leads to more dependencies, which is harder to maintain.
  • Code Changes
    Refactoring is much easier for a single responsibility module.
    If you want to get the shotgun effect, let your classes have more responsibilities.
  • Maintainability
    It’s obvious that it is much easier to maintain a small single purpose class, then a big monolithic one.
  • Testability
    A test class for a ‘one purpose class’ will have less test cases (branches).
    If a class has one purpose it will usually have less dependencies, thus less mocking and test preparing.
    The “self documentation by tests” becomes much clearer.
  • Easier Debugging
    Since I started doing TDD and test-first approach, I hardly debug. Really.
    But, there come times when I must debug in order to understand what’s going on.
    In a single responsibility class, finding the bug or the cause of the problem, becomes a much easier task.

What needs to have single responsibility?
Each part of the system.

  • The methods
  • The classes
  • The packages
  • The modules

How to Recognize a Break of the SRP?

  • Class Has Too Many Dependencies
    A constructor with too many input parameters implies many dependencies (hopefully you do inject dependencies).

    Another way too see many dependencies is by the test class.
    If you need to mock too many objects, it usually means breaking the SRP.

  • Method Has Too Many Parameters
    Same as the class’s smell. Think of the method’s parameters as dependencies.
  • The Test Class Becomes Too Complicated
    If the test has too many variants, it might suggest that the class has too many responsibilities.
    It might suggest that some methods do too much.
  • Class / Method is Long
    If a method is long, it might suggest it does too much.
    Same goes for a class.
    My rule of thumb is that a class should not exceed 200-250 LOC. Imports included 😉
  • Descriptive Naming
    If you need to describe what your class / method / package is using with the AND world, it probably breaks the SRP.
  • Class With Low Cohesion
    Cohesion is an important topic of its own and should have its own post.
    But Cohesion and SRP are closely related and it is important to mention it here.
    In general, if a class (or module) is not cohesive, it probably breaks the SRP.

    A hint for a non-cohesive class:
    The class has two fields. One field is used by some methods. The other field is used by the other methods.

  • Change In One Place Breaks Another
    If a change in the code to add a new feature or simply refactor broke a test which seems unrelated, it might suggest a breaking the SRP.
  • Shotgun Effect
    If a small change makes a big ripple in your code. If you need to change many locations it might suggest, among other smells, that the SRP is broken.
  • Unable to Encapsulate a Module
    I will explain using Spring, but the concept is important (not the implementation).
    Suppose you use the @Configuration or XML configuration.
    If you can’t encapsulate the beans in that configuration, it should give you a hint of too much responsibility.
    The Configuration should hide any inner bean and expose minimal interfaces.
    If you need to change the Configuration due to more than one reason, then, well, you know…

How to make the design compliant with the Single Responsibility Principle
The suggestions below can apply to other topics of the SOLID principles.
They are also good for any Clean Code suggestion.
But here they are aimed for the Single Responsibility Principle.

  • Awareness
    This is a general suggestion for clean code.
    We need to be aware of our code. We need to take care.
    As for SRP, we need to try and catch as early as we can a class that is responsible for too much.
    We need to always look for a ‘too big method’.
  • Testable Code
    Write your code in a way that everything can be tested.
    Then, you will surly want that your tests be simple and descriptive.
  • TDD
    (I am not going to add anything here)
  • Code Coverage Metrics
    Sometimes, when a class does too much, it won’t have 100% coverage at first shot.
    Check the code quality metrics.
  • Refactoring and Design Patterns
    For SRP, we’ll mostly do extract-method, extract-class, move-method.
    We’ll use composition and strategy instead of conditionals.
  • Clear Modularization of the System
    When using a DI injector (Spring), I think that Configuration class (or XML) can pinpoint the modules design. And modules’ single responsibility.
    I prefer to have several small to medium size of configuration files (XML or Java) than having one big file / class.
    It helps see the responsibility of the module and easier to maintain.
    I think that the configuration approach of injection has an advantage of annotation approach. Simply because the Configuration approach put the modules in the spotlight.

Conclusion
As I mentioned in the beginning of this post, I think that Single-Responsibility-Principle is the basis of a good design.
If you have this principle in your mind while designing and developing, you will have a simpler more readable code.
Better design will be followed.

One Final Note
As always, one needs to be careful on how to apply practices, code and design.
Sometimes we might do over-work and make simple things over complex.
So a common sense must be applied at any refactor and change.