Let’s do something useless

Ok, this one will be trivial, but it needs to be written and, what’s more important – remembered. Doing anything makes sense only if there is a reason to do it. You should not write a line of code just for its own sake. And there are several reasons why you shouldn’t do this. Firstly, there a chance that you really would never need this code. Secondly, you are making an effort that is needed for maintenance. Someone has to take care of this code, someone will read it and so on. And thirdly, useless code makes everyone confused, because “if someone added it, there has to be a reason why he did it”.

But it is not only the case that useless code is not used anywhere. There are situations when you can notice in your application code appearance of methods that, yes, are used, but are only the part of some functional whole. They bring no value on their own except problems.
And those are the “creations” that I would like to write about today.

single responsibility principle – it’s not all

SRP tells us that each class needs to be responsible for only one thing. When creating your own classes/methods, it is also important to remember that our elements need to follow those rules when we are taking context of the application into consideration.

What’s the matter?

You won’t create a method that returns a list of all customer’s invoices if the only thing you need is to get those that aren’t timely paid. Instead of such a method, you should create the one that returns the list of those that you are interested in. The problematic thing is that when you look at the method outside the context, it looks all right. Yet, when you will take context into consideration, then it is obvious the method is not a whole on its own. It is just a part of a bigger whole.

maybe you would like to look at the code?

Let’s look at the example that describes what I just shared.

Let’s imagine that we are working on an application which manages various types of events. Each event has got its own state:

enum EventState {
    CREATED, INITIALIZED, READY_TO_GO, FINALIZED, CANCELED
}

Sometimes it happens that an event somehow is hanging. That’s why once a day applications send notifications about those events to all people who are interested in which events weren’t finalized or canceled:

EventsBatch events = EventRepository.getAll().notFinalized().notCanceled();

if (events.notEmpty()) {
    notyify(events);
}

The code above is quite good, isn’t it? Well, not really. What problems can we expect from such implementation?

First, we can notice violation of SRP. Method’s code is not responsible only for sending a notification when specified conditions will occur. We also defines the events about which user will be notified.

Secondly, let’s imagine that at some moment in time we will add a new functionality and we need to do some kind of operations using exactly the same pool of events. Most of developers would go to EventRepository class first, but there they will see that the method they are looking for does not exist. Do you think that all of them will go over the code to find out whether someone had ever the same need, but instead of creating method he done it inline, as a yet another code instruction? Would you do that? Should we even take such a steps into consideration?
On the other hand, even if a developer will invest his effort into searching there is always a chance that code won’t be found.

And in such situation a developer would create method that doing exactly the same what is done in different place. And it result with next problems that directly comes from violation of DRY. We have two places that need to be refactored no each change (i.e. we are adding another non notifiable state – DUPLICATED). I believe you won’t have a problem imagining how easy it is to forget about any of them.

Also, implementation details can leak to the outside world. We can’t talk about encapsulation and, what is even worse, an external object is the one who takes care of valid state of the object. The truth is that until the batch will be useless events are filtered out. It is in invalid state.

We also have twice as big chances to make a mistake. We need to write more tests, because the same functionality is created in two different places. Even if we find bug in one place, there is no guarantee that we will remember to fix it in another. As a result, correctness of the same functionality in different places can be different. Worse than that, we will be sure that the bug was fixed and even if one day we will find out about an affected place, we won’t even think about an “already fixed” bug.

The last problem is the existence of getAll() method at all. It can happen that this method is never used in application as a result provider. What I mean is that we would not do any additional operation on method’s result, because it is exactly what we need. What if the usage of it ends always with “doing something more” with the result? What does that mean? Well, it means that we have more issues with DRY and SRP that we even thought. There can be many places in the code which generate problems mentioned above.

and in the end…

How many times did you write a method whose results you always had to change? Predicate that was always negated? The method that returns a list that you always have to filter?
If you have to do something with the result of the method execution, if you want to make it usable, then remember that it would be good to read this article once again and think how to refactor your code :)