I’ve being coding for about 2 decades now. And still I don’t find it to be an exact science, as some would like to suppose. Otherwise, they wouldn’t ask you for time estimates on feature requests or try to use tools like MS Project to manage a software project, as if Gantt charts could be useful for this kind of project.

Of course, I can completely understand the reasons for the ones willing to bring such project management tools to the software world. Good luck to them! But I won’t talk about this subject in this article as it is too big. I would just like to state that software can be better understood when compared to sciences like Music or general Arts.

Both require lots of experience, personal feelings and are hard to estimate conclusion times since it is almost always something completely new. Although there are some recipes for certain kinds of music or movies, but then they are no longer art.

Some time ago I was asked to estimate how long it would take for me to implement a search system over some HTML documents taken from EDGAR filings. I’m pretty sure that this wouldn’t be something new for some of you who have already had experience with search engines before, but that wasn’t my case definitely. So, I knew I should research about tools like Lucene for search indexing, but I have never worked with them before. So how could I estimate this?

As I started following the tutorials, I thought the main problem was solved in the first 2 days, but I couldn’t predict that I would spend so much time reading about the configuration files for Solr, and how search params could be adjusted. There is a lot of stuff to know about and configure for your needs.

Particularly, one of the curiosities I’ve noticed is that even if my configuration was set to enable AND-like search for all typed terms, if it happens for a user to prepend some word with a plus (“+”) or minus (“-”), then non-prepended words would become optional. I had enabled the DisMax mode, by the way.

The challenge

So, I’d like to talk specifically about this specific challenge as it is a good example for demonstrating some techniques I’ve learned last year after reading Clean Code. Although being very Java-oriented, this book has a few simple rules that can be applied to every language and be really effective. Just like in Music and Movie Making, Software Writing is also a science in which there are lots of resources to learn from and that can be used in a systematic way. Learning those tools and techniques will help developers to deliver more in less time.

Developers should invest time on well-written code because they’ll spend most of their time reading code. So, it makes sense to invest time and money on tools that will make it easier to browse some code as well as investing some time polishing their code so that they become more readable too.

Before talking about those simple rules, I’d like to show you how I might write this code in my early times. Don’t waste your time trying to understand this code. Then, I’ll show you the code that I’ve actually written in a couple of hours, exactly as I have estimated before, since it didn’t have any external dependencies. So, basically, this is the trend:

Transform terms like ‘some +required -not-allowed “any phrase” id:(10 or 20 or 30)’ into ‘+some +required -not-allowed +“any phrase” +id:(10 or 20 or 30)’.

Pretty simple, right? But even software like this can be bug-prone. So, here is a poor implementation (in Groovy, as I’m a Grails programmer in my current job). Don’t try to really understand it (more on this later), just take a look at the code (dis)organization. I didn’t even try to compile it.

How not to code

Ok, I’ll agree that for this specific case, the code may be not that bad, but although processQuery is not that big, you’ll need some time for figuring it out what is happened if you’re required to modify this method.

Also, looking at it, could you be sure it will work for all cases? Or could you tell me what is the reason for some specific line? What is this code protected from? How comfortable would you be if you were to modify this code? How would you write automated tests for processQuery?

Also, as the logic gets more complex, coding this way could led to some messy code like the one I’ve just taken from a file in the project that integrates Hibernate to Grails:

I do really hope never to have to understand such code… I’d be curious to find how would such an automated test be written for this invokeMethod, as I couldn’t find the tests in this project.

What is wrong with this code?

Back to the original implementation, what would be wrong with such code?

1. It takes a lot of time for understand the code (the read cost);
2. It is hard to test;
3. Parsing the query is too much responsibility the SolrService class;
4. If you just need some way of indexing and searching results in stored documents, you shouldn’t be relying in a specific solution, like Solr. If you decide later to change your Search solution from Solr to Elastic Search or using Lucene directly, you’ll need to change a lot of code. It would be better to have a wrapper for something simple like this;
5. It can become hard to change/maintain/debug;

Even if you try to split processQuery into smaller methods, you would be required to pass some common values over and over again, like query and the expressions array, that would not only be an in-parameter but would be an out-parameter too as it would have to be changed inside some methods… When that happens, it is a hint that the overall code needs a separate class for doing the job. This is one of the simple rules I’ve learned in Clean Code.

The simple rules of Clean Code

The top-bottom code writing approach

While reading the original example, the first thing you’ll see is the processQuery method declared in the SolrService class. What does it do? Why do we need it? Who is using it? Only when we look forward, we’ll be able to detect that it is being used from the search method.

I was always used to write code that way, writing the least dependent methods first and the higher level ones as the latest ones. I guess I thought they should be declared first before they could be mentioned. Maybe that was true for some procedural languages I’ve started with before my first experience with OOP while reading a book about C++.

But in all OO languages I know about, it is ok to declare your methods in any order. Writing them top down makes it easier for another reader to understand your code because he/she will read your high-level instructions first.

Avoid more than 5 lines in a single method

Keeping your methods really small will make it easier to understand them and to write unit tests against them too. They’ll also be less error-prone.

Avoid methods with more than 2 or 3 parameters

Having lots of parameters in methods makes it really complicate to associate what is the meaning of each parameter. Looking at this code, could you understand what is the meaning of the last parameters?

You’d certainly have to checkout the API for AbstractUpdateRequest.

Avoid out-parameters

This is a typical example where you’d probably be better served by a separate class.

When you find out some situation where you’d like to return multiple values (I’m not talking about returning a single list, here) and you need some parameter for returning them (and out-parameter), you should reconsider if you’re taking the right path.

Also, you should really try to avoid modifying any parameter as debugging such code can be really frustrating.

Provide good names for your variables, classes and methods

This one is gold. Good names are essential for a good code reading experience. It can save you several hours trying to understand some snippet of code.

Take a look at the signature of the invokeMethod method in the Grails-Hibernate integration example code:

Wouldn’t it be easier to understand what it does if the signature was changed to this one?

What does “obj” mean in the actual implementation? It could be anything with such generic description. Investing some time choosing good names for your methods and variables can save a lot of time from others trying to understand what the code does.

The result of applying such simple rules

Just by making use of those simple rules, you’ll be able to:

1. Easily read your code;
2. Easily write your unit tests;
3. Easily modify and evolve your logic;
4. Have a well-documented code;
5. Get rid of some otherwise hard-to-find bugs.

Some extra rules

Some rules I’ve being using for my entire life and I’m not sure if they are all documented in the Clean Code book or not. But I’d like to talk a bit about them too.

Don’t use deep-depth blocks for handling validation rules

I’ve seen pseudo-code like this, so many times:

Often, there are even more validation rules inside each block and this style gets really hard to read. And, worse than that, it is only protecting the software from crashing or generating an unexpected exception, but it does not properly handle bad inputs (negative numbers).

Also, usually do_real_calculations_with(number) is written as pages of code in a way you won’t be able to see the enclosing brackets of the block in a single page. Take a look again at the Hibernate-Grails integration code to see if you can easily find out where the block beginning at “if (isCriteriaConstructionMethod(name, args)) {” ends.

Even when you don’t have to do anything if the necessary conditions are not met, I’d rather code this way:

This is a real example found in PersistentManagerBase.java from the Tomcat project.

It is hard to see what bracket is closing which bracket in the end… This could be rewritten as:

Simple code should be handled first

The pattern is:

Here is a concrete example taken from ActiveRecord::Explain.

I would rather write such code as:

Of course, this isn’t exactly the same as the original code in the case yield generates some exception for the “else” code, but I’m sure this could be worked around.

Don’t handle separate exceptions when you don’t need to

I’ve often found this pattern while reading Java code and I believe that is the result of using some Java IDE. The IDE will tell the developer that some exceptions were not handled and will automatically fill the code as:

If you’re only interested in gracefully handle exceptions to give your user a better feedback, why doesn’t you just write this instead:

The original challenge actual implementation

And, finally, following those techniques, here is how I actually coded that original challenge and implemented the tests in JUnit:

I’ll omit the implementation of SearchResult class, as it is irrelevant to this specific challenge. I just want to point out that I’ve abstracted the search feature in some wrapper classes for not exposing Solr internals.

And here is the real implementation code:

And the unit tests:

Conclusion

That is it. I’d like you to share your opinions on other techniques I may have not talked about here. Are there any improvements that you think would make this code even easier to understand? I’d really appreciate any other considerations you might have since I’m always very interested in writing Clean Code.