Want to talk?

It’s been awhile since my last article. I want to assure you that writing is still on top of my list of priorities. Recently I was just overwhelmed by lots of activities that are extremely important for me. All of them required some effort and preparation but I think that I’m almost ready to come back to writing. So expect more really soon!

Many parameters and lost information

The less code, the better? The fewer objects, the better? Is it true? As usual, it depends.

There are cases when by adding something more we are adding unnecessary complexity. It happens when we are creating interfaces or other abstractions just because “we may need this additional flexibility in future”. It happens when we forget about YAGNI principle and we are writing code that may make our life easier in case of new requirements that… may never come.

On the other hand we have got situations similar to the one I described in my recent article. I showed you an example where we added a few methods that internally are doing almost the same. Yet, by adding them we gained a lot - the code became easier to understand. This additional code gave us information about WHAT the object is doing instead of HOW it is achieved.

Today I would like to share with you another example which shows that less code sometimes may mean less readable code.

Once upon a time...

Today I would like to talk to you about history:

public class History {

public void store(
Author author, RefactoringType type, Scope scope, 
RefactoringJustification justification, Date today) {
    // some code
}

Is it easy to figure out what store method is storing? Is it possible to understand? Well, even if it is, I believe that we all may agree that it is definitely hard.

How do you extract necessary information from the method’s declaration? I can assume that firstly you read the class and method names to find the context. Good, we have it. We want to store some historical information. Now the hard part starts - you have to find out what we want to store. You cannot just simply read this information, because it is not present in the code. In that case you will probably try to find this piece of information by looking at the list of parameters. You will read them with the hope that you will be able to figure out what the author of the code wanted to store.
Or you may look at the message of the commit that introduced this code.
Or you may look into the method’s definition and look for the answer in the implementation.
Not the best ideas, though.

Don’t you think that it would be great to have this information easily accessible? To have code we could understand without making an additional effort? I believe this is exactly the way how we should write it.

Parameter Object for the rescue

Why do we not know everything just after reading the method’s declaration?
Somehow we were able to find out there’s something about history in here - the name of the class gives us this information.
We know that it is about storing something - the name of the method is pretty descriptive.
The problem is that we don’t know what we want to store in the history. Why? Because input parameters are not giving us this information.
Those parameters indicate what pieces we want to store, yet, do not explain what we should know when all of those pieces will be put together. We are getting information about implementation (parts that are used) and we have no clue what this code is supposed to do.

What we could do? We should hide implementation and explain what we want to achieve with this code. And that’s the moment when Parameter Object comes to rescue. You may treat it as a box for a few different objects, as a solution that may decrease dependencies. However, for me, the biggest benefit of using this pattern is the fact you will have to name this object and by doing so you will be forced to provide valuable information.
Let me show you:

public class History {

public void store(CodeDelta delta) {
    // some code
}

Now it is obvious what we want to store. We share useful information with the people reading our code. We also hide an implementation. They can focus on things that are important and won’t be bothered by all those additional details that are interesting only when you are writing or modifying the method.

So, what you say, the less or the more is the better?

Implementation, behavior and the amount of the code...

Implementation, behavior and the amount of the code... Today is the right time for another code-related question that I’d like to ask.
Take a look at these methods:

refactoring.setState(IN_PROGRESS);

refactoring.setState(VERIFIED);

refactoring.setState(DONE);

To make everything clear, we can easily assume that each invocation is done in a different place and after different activities have been completed.
There is nothing exciting in this code and probably you saw something similar in your code bases many times. Nothing complicated. We are just changing the state of the object.

So, now’s the time for the question: what’s wrong with this code?

Conjunctions we... hate

Recently I’ve written about implementation-related names and I’ve presented a few examples where the method name was incorrect because of its strong relation with the body.
At one moment, we had the following code:

boolean isComplexOrUnreadableWithTests() { 
    return (complex || unreadable) && tests.exist(); 
}

Just to remind you of the context: it was supposed to find out whether we may or may not do the refactoring:

if (code.isComplexOrUnreadableWithTests()) {
	doRefactoring(code);
}

Recently I’ve told you that the name is wrong because of its direct relation to the implementation. However, this is not the only problem. The use of conjunctions in the method’s name is a sign that we could not find a right name and we just list all known things that we’ve done. It does not matter whether this list is implementation- or logic-related.

The name should express the intention

This time I will start with a code sample. Take a look at this:

if (code.isComplexOrUnreadable()) {
    refactor(code);
}

Can you tell me what is wrong with this code? No?

Let me ask you another question then. How do you think the implementation of isComplexOrUnreadable() method looks like?
I assume that many of you would imagine something similar to this:

boolean isComplexOrUnreadable() {
    return complex || unreadable;
}

Am I right?

Ok, so let me ask you again: can you tell me what is wrong with this code?

Should you follow the rules?

In the last article I wrote that I’m a big fan of tools for static analysis. Those tools help you follow established rules. To be more specific, they’re yelling at you through red builds whenever you break these rules. Isn’t it great? Before your code gets under the review, we are already sure that many things have already been checked and we don’t have to bother ourselves with verifying the code from this side once again.
However, this solution comes with a problem. When a developer receives feedback, they know what is wrong, but not always know why a particular part of the code was recognized as invalid.
And lack of understanding of rules can lead to huge damage.

What is a problem?

Recently one of my colleague from the team has tried to push code for a review. Yet, static analysis build didn't let him do it. It failed. The problem lay in a missing constructor for a util class. Ok, fix it and move on, but… after adding the constructor another issue was found - there was not even a single test for a private constructor. For the sake of coverage!

I don’t want to talk whether our rules are good or not. I want to talk about a feeling that may appear after some time of receiving such messages.
What’s the feeling I’m writing about? Irritation. Not because you’ve made a mistake in your code. It’s not a problem, everyone does. Not because build was red. It’s great, it should fail in case of our mistake. Not because we have to solve an issue. It’s reasonable that you are fixing issues in your code. The problem is that we learnt nothing. There is a message which describes WHAT is the problem, but it doesn’t explain WHY it is a problem.

Why it is a problem?

When you don’t understand the reason next problem found by static-analysis tool becomes annoying noise, not a good suggestion. You start looking for an exception from the rule, for an excuse to break it. This is not because you don’t want to create a high quality code. You do want, but until you don’t understand the reason behind the rule it may look stupid to you. It may look worthless.

What will happen if you don’t find an exception? Probably you will start to obey this rule. Automatically. But it will still be against you, it will bother you. And you won’t follow such suggestion, because you know why it is important. No, you will follow it because you have to. Such practice will be full of negative emotions. And if a new person some day asks you why you’re writing code in this way you will tell them that it is because “some stupid rules”. And now there will be two people who will apply it into their code without understanding.

Is it really a problem?

Is it really so bad? Isn’t it better to have a developer who writes good code without understanding rather than allowing them to write worse code?
Well, yes. If we are looking at this from this angle, this is true. However, we want to work with good developers, we want to spread the knowledge and understanding of good practices. We want to teach others and share experience with them. It will be a win-win situation for both sides. We want to work with people who can improve our solutions and challenge our ideas.
Over time, the quality of the code will get worse if we have to work with people who follow the rules blindly. Why? Because of the lack of knowledge. Code review, refactoring, design and redesign, etc. those are activities that require specific knowledge. If we don’t share it, there will be a huge number of developers who won’t do it because they wouldn’t know how.

Fix it!

After we fixed all issues in the code we had a great discussion about values of those static analysis tools. Values and the risks that I explained above.
When we talk about problems we start to think what we can do to avoid them. Or at least minimize. How to change a description into an explanation?
We come up with the idea to change default messages. Add explanation why a described problem is a problem. Or at least add a link. We should avoid leaving people without knowledge.
Our idea is to change those failing builds into a great lesson!

This is our idea and I’m pretty excited about it. I know it would take time to improve all messages, but on the other hand I’m also pretty sure it is worth the effort.

What do you think about it? Maybe you have your own idea? Share them in comments. It would be great to know your ideas.

What’s really matter? When you break a rules!

I’m a big fan of tools like PMD, FindBugs, checkstyle, etc. Those tools are really helpful. Thanks to them you don’t need to worry about many small issues. You don’t have to verify whether an author of the code you are reviewing follows coding standards or other agreed practices. Your tools will do it for you.