Identifying the dirt in our code - names, functions, and comments

The problem of having code that is not very clean is that, as we maintain it, the code will get more and more complex and its understanding become compromised.

When I talk about complex code, I don't mean business rules, but the complexity of understanding and code legibility. I'm talking about spending some time trying to understand what is p or what the read(p, i)function do when you don't have any context. What does it read? what are p and i? Aaaaahhh /o\

When we are in a place that we spend more time trying to understand an old code than thinking on the new code implementation, it could be a sign that something could be better. Spending more time in a task than we liked to, leave us frustrated

When is difficult to maintain code, we stop to care about its quality and consequently we write "bad" code. We leave the code even more complex and, the more complex it gets, and more difficult it is to maintain. Our productivity drops almost to 0 and it becomes risky to fix a problem without developing 3 more on this process. We become afraid to touch a code that already works "magically" (because we, obviously don't have a clue about what's happening)

The consequences of this are code so difficult to maintain that companies end up closing. I've seen it happen once. The only product had several bugs, it was impossible to fix a bug without creating a few more and the customer satisfaction has reached a level that they didn't want use the product anymore, and the company closed

If we know that is so harmful, why we do this?

Hurry.

Usually, we have such tight deadlines that we deliver our tasks poorly to skip to the next.

I want to avoid this from happening. How I do this?

This is the first part of a series about clean code. The idea, in this first topic, is talk about naming things, write better functions and a little about comments.

Next, I intend to talk about formatting, objects and data structures, how to handle errors, about boundaries (how to deal with another's one code), unit testing and how to organize your class better.

I know that it'll be missing an important topic about code smells, but this is a huge topic that has the potential for another series.

So, let's begin?

Give good names!

Use names that show what your code does:

int d could be int days

instead while x == 7, we could use while time == DAYS_OF_WEEK.

Avoid misinformation.

if accountsList is not a list, it should have a better name.

Varible that has very subtle name differences also fall in this topic, such as
VeryNiceControllerToDealWithStrings and VeryNiceControllerToStoreStrings.

How long did you take to spot the difference between the two variables?

Make a good distinction of your variables.

If we have two variables, one called banana e another called theBanana, how can we differentiate which one is which? This same rule applies for CustomerData, Costumer and CostumerInfo. What's the difference between then?

Use names that you can pronounce

It facilitates internal team communication. If you pair, you don't need to refer to a variable as wpdw. A workDaysPerWeek sounds better (:

Use searchable names

Have you ever tried to use ctrl+f on a variable that you couldn't remember the name? Or that it was so common that you just couldn't? Have you tried to search for a variable called f?

Avoid prefixes!

It's another thing that makes the search difficult. Not only the search but mainly autocomplete. if the variables start with icsd, like icsd_name or icsd_date we'll have a hard time to find what we are looking for.

Avoid mind mapping

When we use a huge quantity of variables that don't make much sense, we need to memorize what they are meanwhile we are trying to figure out what the code does.

user.each do |u|
  u.each do |s|
    s.each do |h|
      print h.name
    end
  end
end

(this code block has another problem with so much each inside each inside each insi.... but we'll talk about that later)

Class names

Should be nouns, and not verbs, because classes represent concrete objects. They are the representation of things.

Methods names

Should be verbs, and not nouns, because methods represent actions that objects must do.

Add context!

If you bump on a variable state alone, probably will have a hard time to figure out what it means.

But if this variable is near street and number it's easier to understand what it does.

Add context, but not for free.

Let's suppose that we are working on a project called chocolate ice cream. We don't need to add CIC in front of all variables.

At the same time, inside a User class, we don't need to call all the variables userAdress, userAge or userFavoriteColor.

Functions

They should be small

They should be small and readable, with few nests. In the ideal world, they should be 1 or 2 levels of indentation.

Do only one thing.

They should make only one thing and that's it.

But how do I know that it does only one thing?

def verifyNameAndLogout
  if rightName?
    showMessage
  end

  logout
end

If you look at the code, it seems that it does 3 things

1. Verify if the name is right
2. If yes, it displays a message
3. logout, regardless the name is right or wrong.

We understand that it does what is proposed in only one level inside the function, so we can say that this function do only one thing.

The only modification that we could do in this code would be relocating the if statement to another function, but in this case, it would be only a relocation, without any additional advantage

Functions can't be divided into sections. If you can divide, it's a sign that it could be more than one function

One abstraction level per function

When we talk about abstraction levels, we can classify the code in 3 levels:

1. high: getAdress
2. medium: inactiveUsers = Users.findInactives
3. low: .split(" ")

The ideal is not to mix the abstraction levels in only one function. When we mix, the function becomes a little harder to understand, and we can't read the code step by step, from top to bottom, like an article

The best scenario is that you can read code from top to bottom, like a narrative.

ex:

1. To include the setup and the teardown, we first include the setups, then we include the page content, and after that, the teardown.
2. To include the setup, we add the setup suite and then the default setup.
3. To include the test setup, we search for the hierarchy...

(and so on...)

Avoid switch statements and excessive if/else

The motive that we should avoid extensive switch/if-else is that beside them beeing to big and doing many things, they violate the open-closed principle of object-oriented programing or. also known as the O in S.O.L.I.D .

One entity should be closed for modification and open for expansion. When I talk about entity, I mean class, module, function and so on

When there are several if/else/switch in a single module, we have to modify the code if we want to add any extra case, what makes the code more fragile and difficult to test.

Sometimes we need to verify differents situations and take different actions depending on the situation.

One way to decrease the switch/if/else is to use polymorphism.

Using a simple example:

foreach (var animal in zoo) {
  switch (typeof(animal)) {
    case "dog":
      echo animal.bark();
      break;

    case "cat":
      echo animal.meow();
      break;
  }
}

this could be refactored in this way:

foreach (var animal in zoo) {
  echo animal.speak();
}

On this example, under the hood, we created an animal class, that has the method speak. Then we created a dog and a cat object, we give then the behavior to speak and that's it.

Sometime, polymorphism could be a little too much to solve this problem. Sometimes we rearrange the logic, create a few more function and fix our problem.

Less arguments is the way to go

When we talk about arguments, Less is more

When we have many arguments, the complexity of our function and, consequently, our tests greatly increases.

Another thing that happens is mistaking the argument order when we call the function.

Occasionally we can decrease the argument size creating a class.

A function with several arguments could be transformed in a class, and this function could be a method in this class. The arguments, much of the time, could be their properties and, this way, we don't need to pass any argument.

In some cases, we don't need this much, so we could use other ways to decrease the number of arguments, like grouping then.

Circle makeCircle(double x, double y, double radius) could be transformed in Circle makeCircle(Point center, double radius)

Function are verbs, arguments nouns

When we name function as verbs, and arguments as nouns, it becomes more legible and easier to understand the behavior that we expect, like write(name)

The argument order is also important so you don't forget when you'll use the function.

Just imagine to get the order right if the arguments were changed: assertExpectedEqualsActual(expected, actual)

Avoid flag as arguments.

Avoid passing a boolean as an argument, because it makes the refactor hard.

Try to verify the truth or falseness when you call the function, and not inside it.

A function shouldn't have collateral effects.

If we have a function called checkPassword, it shouldn't do a login inside its body.

We were not prepared to login at any time and this could cause undesirable side effects, besides test de function becomes more difficult.

Output arguments

Output functions should be avoided. If a function needs to change the state of something, it should change the object.

Sometimes we bump into functions like appendFooter(s) and we are not sure if the argument is an input (s is the footer and we are attaching it somewhere?) or output (we will attach s to the footer?).

If the first case is the correct one, if s is the footer, the best thing to do is objectInstance.appendFooter

Command Query Separation

A function should change the state of an object, or return some info about it.

This is an example of a function that doesn't follow the command Query Separation:

if name?
  name.change
  true
else
  false
end

A function that changes something and returns true or false should be replaced by two. One that verifies, and another that makes the change.

DRY - Don't repeat yourself

The problem with repeated code is, besides that it inflates the code, any change that we need to make we have to change in several places.

However, we must be aware that too much DRY can be harmful. We often confuse code duplication with business rule duplication. Sometimes it's okay to duplicate a code as long as you're not duplicating a business rule that should be unique.

There is another maxim also that says: you must write the same code a maximum of 3 times. The third time you should consider refactoring and reducing duplication.

Avoid returning an error code

When methods return error messages, it violates subtly the Command Query Separation. When we put an error inside an if block, it may cause some confusion, since it may be returning something or an error.

if name?
  name.change
else
  raise 'An error has occured'
end

In the example above, if it works, it performs an action. If it went wrong, it pops a mistake

Comments

Avoid

Comments on a code usually bring more evil than good. Most of the times, a comment about what a variable does, or details of how the things work could:

1. Become outdated, confusing your future self (Yesterday I lost some precious time because of an outdated comment)
2. Could be replaced for some better named variable/function/class.
3. They pollute the code unnecessarily.

It should be clear that avoid != forbid. If you are a java programmer and is doing something public, Javadocs is important. Sometimes it's interesting to explain some code (How many time you spend trying to figure out a regex pattern when you see one?), but, in 99% of the time, comments could be avoided.

How I will write the code if I have to pay attention to all these rules?

In a good article you put the ideas on a paper and goes refining the text. coding functions in the same way.

First, you make it work, then you refactor.

Uncle Bob, in clean code, defends that the best order to write code is:

1. Write unit tests.
2. Create code that works.
3. Refactor to clean the code.

Write all the tests, make it work and, when you are sure that everything is the way that's supposed to be, refactor removing all the dirty code and applying design patterns.

Particularly, I don't have the habit to write tests before the code (TDD), however, test the code before doing a refactoring is indispensable. Only then we can be sure that your code will always work, even after so much modification.

The Clean code book is focused on bad practices. It details several problems and does not delve into ways to solve them.

If you want to delve more into how to fix the dirty-code-problem, we could look into the book refactoring. In this book, there are details and several different ways to eliminate the dirty in the code

Comments

[Carolina Cunha]

I loved this article! In fact, I'm think I should print it :)

But I didn't understand that part. about abstraction:

When we talk about abstraction levels, we can classify the code in 3 levels:
1) high: getAdress
2) medium: inactiveUsers = Users.findInactives
3) low: .split(" ")

Can you explain it to me, please?

[Rachel Curioso :D]

The high level abstraction are functions that you create, like searchForsomething()

The medium level are methods in your object, like account.unverifyAccount

The low level are methods that the language provides, like map, to_downncase and so on (:

[Carolina Cunha]

Ahhh!Thanks!Sorry, was pretty clear now that I get it :)

Very nice article, I'am looking forward for the others.

[Yuri Predborskiy]

I believe repeating "Account" in account.unverifyAccount is not required. Do you think it is? Why?

[Rachel Curioso :D]

The Account in unverifyAccount is clearly an unnecessary addition of context. Thank you for the review

[Ghost]

shhh, don't say in public that you don't make your tests first. The TDD police may raid SWAT your house. (I don't test before either and I even add tests that I wouldn't think of beforehand, but don't tell anyone). Good article tho. How I've hated my past self making those errors, I can only hope my future self doesn't hate me that much :)

[Rachel Curioso :D]

hahahaha. Let me edit this post to prevent the TDD police to knock on my door.

I try to do TDD sometimes, but I think it's hard when I don't have clarity of what I'm developing. I intend to read Test Driven Development: By Example and try for real one day.

[Ghost]

I tried but doesn't work for me, I guess I'll have to remain a scum. If you succeed pray for us, we'll be right here sitting in the mud being worthless.

[Jason C. McDonald]

Cursory response that avoiding comments isn't the answer; rather, ensure they only state why the code is there, not what it does. (If code and intent-comment ever mismatch, that often indicates a bug that should be addressed.)

To Comment Or Not To Comment?

Jason C. McDonald ・ Jan 20 ・ 12 min read

#coding #culture #productivity

[Rachel Curioso :D]

You made a very good point
Great article, by the way (:

[Anna]

Great material. There's a lot of legacy code out there that is violating a lot of these rules and a lot of developers are shaking their heads trying to understand the flow.

[Rachel Curioso :D]

Every day I bump into some code that violates one of those principles.

(and almost every week I receive a code review that I'm the one doing this. I think that write clean code is something that you get used with time)

[Wilhelm Erasmus]

Absolutely amazing article! Thanks for sharing. You even unintentionally highlighted the danger of abbreviating variable names by swapping some of the letters around. I don't want to admit how many times I've gotten a compile-time error just because I swapped two of those damned letters.

[Brian Hannaway]

Great article, Rachel. Dont agree with the section on comments though. I think comments play an important role in creating maintainable code. I agree they need to be used sparingly, but I also think that good quality comments can add value to a code base. I covered this in a blog post recently. briansdevblog.com/2019/08/the-impo...

[shaunchong]

Thanks for the article!

Could you clarify about the section "Avoid returning an error code"? Instead of returning an error code, how should I then structure my code?

I usually raise an exception if an error occurs and do a try/catch from the calling function.

[Rachel Curioso :D]

You could return a message and not an error code. Another option is (in some cases) to change the user flow, redirecting the user to another page, and another option is to just log the error instead show to the user.

The problem with returning an error code is that the user won't know what the error means.
And raising an exception you'll break the application leaving the user frustrated.

[shaunchong]

Thanks!

I misunderstood your original article and thought you meant that we should not be returning error codes in our functions. I think it's ok to return error codes/throw exceptions in our functions but it should be caught (or we should detect the error code before it reaches the user) and display a friendly error message to the user.

[Omar Gaston Chalas]

This is a great article!

You've addreesed the most frecuently mistakes that we do often. I think we have to make the habit of refactor our codebase and put all these principles in practice.

Thanks for sharing Rachel.

[David Hedderwick]

Thanks for the article. I'm learning how to write clean code the hard way - I have a decade old website and body of C# code that has been written by multiple people in various different ways - knowing how to approach the code and what to look for to clean up (said dirt) is very helpful.

Thanks!

[Ross Holloway]

"Class names
Should be nouns, and not verbs, because classes represent concrete objects. They are the representation of things."

This feels to me like it contradicts SRP. Sure, some classes model objects, but what about those classes which perform a service?

[Rachel Curioso :D]

You are right (:
I didn't mention the classes that perform a service, but I see these classes as something palpable.

When I think about those classes I remember when I had to connect my application with someone else's, and we called this class "connectors". They perform a service, but they are an "object" that group various actions.
(when I talk about object, it's not in a "programming" way)