How To Clean Code - Part I

Intro

Clean code principles... every professional software engineer should know about them and most importantly, follow them. Software engineering as a profession mentions the first time in 1963 while developing guidance and navigation systems for the Appolo missions, so it exists for a long time, almost 60 years. In that period a lot of problems were encountered and solved with various design patterns, conventions, and principles. So this blog serves as a review of those rules which can help our team and ourselves to be better software engineers. Everything which is mentioned here is inspired by the book "Clean Code, A Handbook of Agile Software Craftsmanship" written by famous author Robert C. Martin aka Uncle Bob. The main thought through all coding sessions should be: "clean code should always read like well-written prose." by Grady Booch.

Naming

TLDR

• Use meaningful and intention revealing names in your code
• Short scope names should be short
• Large scope names can be longer but must be more descriptive

Can you remember the days when you first started coding? Those "Hello world" and "Your name is" programs, well let's go a bit further and try to remember the first algorithm you wrote, an array sorting, or maybe find in the collection piece. Probably back then we weren't thinking much about our variable names and we were naming them like:

int a;
int xyz;
String s;
int[] a;

Sure, for short programs like those were, it could pass, but imagine using names as those in a big, complex codebase. You'll surely agree that it would be really hard to read and understand what is happening. There are cases where we can use short names like that, but we'll get to that quickly.
Here is an example code from the early days:

    public static void bubbleSort(int[] a) {
        boolean s = false;
        int t;
        while (!s) {
            s = true;
            for (int i = 0; i < a.length - 1; i++) {
                if (a[i] > a[i + 1]) {
                    t = a[i];
                    a[i] = a[i + 1];
                    a[i + 1] = t;
                    s = false;
                }
            }
        }
    }

This is a standard bubble sort algorithm that consumes an array and doesn't return anything. Take a look at the name of a variable, is it self-explanatory, do you know from the name what it represents?
Now look at this code:

    public static void bubbleSort(int[] array) {
        boolean isSorted = false;
        int biggerValue;
        while (!isSorted) {
            isSorted = true;
            for (int i = 0; i < array.length - 1; i++) {
                if (array[i] > array[i + 1]) {
                    biggerValue = array[i];
                    array[i] = array[i + 1];
                    array[i + 1] = biggerValue;
                    isSorted = false;
                }
            }
        }
    }

Much easier to understand, isn't it? Well, that is the reason why is it important to invest time in the naming. Meaningful and intention revealing names make code much easier to understand, like well-written prose. Another important rule is to name classes, objects, and variables using only nouns while functions should be verbs, like bubbleSort() is a verb. Maybe you noticed the index variable in the for loop is just the letter i, but that is fine because we are all used to it, and we know what i stands for. Another reason why is so short name is that the scope of it is very short, only inside the loop. That brings me to another rule which says that the length of a variable name should depend on its scope. If the variable is more of local scope, its name should be short, but if it has a larger scope, a more descriptive name would be a better choice.

Functions

TLDR

• Always separate functions by their level of abstraction
• The number of arguments shouldn't be more than 4
• It would be better to create functions for true and false cases instead of passing a bool as an argument
• If possible, functions should be around 4 lines long.
• The function should do ONE thing and ONE thing only
• The function should not have any side effects

Again, let's remember the first days of our programming lessons. In those lessons, we were told to use functions when there is repeating code, to make it reusable. Let's google the definition and see the result, I found the following: "A function is a block of organized, reusable code that is used to perform a single, related action." So we will use that code a lot and that is why it is important to make it clean and readable. How to achieve that? Firstly, we should keep our functions short, according to Uncle Bob, 4 lines top. While that is ideal, it isn't written in the stone or compiler, so it can be a bit longer, but keep it as short as possible. What Uncle Bob wants to say with 4 lines tops functions is to extract the code from the functions to more short and simpler functions, but why? Look at the example:

    public RedirectView authenticateUserAndRedirectToMainPage(String username, String password) {
        Optional<User> user = Optional.empty();
        DataSource dataSource = DataSourceSingleton.getInstace();
        try (Connection connection = dataSource.getConnection();
             CallableStatement stmt = connection.prepareCall("authenticateUser")) {
            stmt.setString(1, email);
            stmt.setString(2, password);
            try (ResultSet rs = stmt.executeQuery()) {
                if (rs.next()) {
                    user = Optional.of(new User(
                            rs.getInt("ID"),
                            rs.getString("EMAIL"),
                            rs.getString("PASSWORD"),
                            rs.getBoolean("IS_ADMIN"))
                    );
                }
            }
        } catch (SQLException ex) {
            Logger.getLogger(DBUserRepository.class.getName()).log(Level.SEVERE, null, ex);
        }

        if (user.isPresent()) {
            RedirectView redirectView = new RedirectView();
            redirectView.setUrl("http://www.stackoverflow.com");
            return redirectView;
        } else {
            throw new RuntimeException("User not found...");
        }
    }

At first look, there is a lot to read, there are multiple levels of abstraction, exception management and function does more than one thing. All mentioned is red alarm...levels of abstraction, exceptions, and more just in one function. As school students, maybe we would say: "Oh, nice. All I need is in one function, staring at low a level and getting the final high-level result, amazing." Let's start refactoring the code and explain why step by step. The name of the function reveals that it does more than one thing, and it is not a good practice, function must do ONE thing and ONE thing only. After we create Optional of User, we work with the database by calling the stored procedure "authenticateUser" and get the result if the user exists. We can agree that this is a low level of abstraction. One more thing to worry about is the try-catch block that surrounds the database access code. Let's solve the problem by extracting and refactoring the code. Before we start, just one note that we will focus on refactoring the authenticateUserAndRedirectToMainPage function to keep it simple. Others will be ignored.

The first step is to extract the try-catch block to function.

    private Optional<User> authenticateUser(String username, String password) {
        DataSource dataSource = DataSourceSingleton.getInstace();
        try (Connection connection = dataSource.getConnection();
             CallableStatement stmt = connection.prepareCall("authenticateUser")) {
            stmt.setString(1, email);
            stmt.setString(2, password);
            try (ResultSet rs = stmt.executeQuery()) {
                if (rs.next()) {
                    return Optional.of(new User(
                            rs.getInt("ID"),
                            rs.getString("EMAIL"),
                            rs.getString("PASSWORD"),
                            rs.getBoolean("IS_ADMIN"))
                    );
                }
            }
        } catch (SQLException ex) {
            Logger.getLogger(DBUserRepository.class.getName()).log(Level.SEVERE, null, ex);
        }
        return Optional.empty();
    }

Great, first step done. A recommendation would be to extract this method to a new interface with an implementation class, for example, UserRepository. Now let's see how our code looks now.

    public RedirectView authenticateUserAndRedirectToMainPage(String username, String password) {
        Optional<User> user = authenticateUser(username, password);
        if (user.isPresent()) {
            RedirectView redirectView = new RedirectView();
            redirectView.setUrl("http://www.stackoverflow.com");
            return redirectView;
        } else {
            throw new RuntimeException("User not found...");
        }
    }

You will agree this is more pleasant to eyes. The next step would be to extract code inside the if clause.

    private RedirectView redirectToMainPage() {
        RedirectView redirectView = new RedirectView();
        redirectView.setUrl("http://www.stackoverflow.com");
        return redirectView;
    }

Nice, now our function looks like this:

    public RedirectView authenticateUserAndRedirectToMainPage(String username, String password) {
        Optional<User> user = authenticateUser(username, password);
        if (user.isPresent()) {
            return redirectToMainPage();
        } else {
            throw new RuntimeException("User not found...");
        }
    }

We are getting close to the goal, two steps are done, two remaining. By now I hope we all got a little smile on our faces because of the progress. The last two steps would be to replace the if-else body with lambda since we use Optional and to rename the function. If we look at the feature we had to implement, it was logging the user into the application. Exactly that should be our final method name, loginUser. If it is a bit confusing how is this good, because our function is doing two actions again, authentication and redirecting, well we have to merge them to make a feature. It is a higher level of abstraction, therefore more abstract naming and function body. The final result should look like this:

    public RedirectView loginUser(String username, String password) {
        return authenticateUser(username, password)
                .map(user -> redirectToMainPage())
                .orElseThrow(() -> new RuntimeException("User not found..."));
    }

We are done, let's make a short review. If you compare the first version of this function, before we did two things, now we do ONE and ONE only. Before, we mixed levels of abstraction, now we have one level of abstraction per function. Also, if you didn't notice, every function is one level of abstraction above the inner function. Before our code required concentration and time to figure out what is it doing, now we need a couple of seconds to read it and again, read it like well-written prose. Felling happy and satisfied, aren't you? It is in our nature to bring order to chaos. We haven't mentioned yet the number of arguments that are passed to function. According to Uncle Bob, as a number of lines, 4 tops. Of course, the ideal would be without arguments, but there are a lot of cases where that isn't possible. One more tip from Uncle Bob is to never send a boolean as an argument, but create two functions, one for true and one for a false case. Lastly, there is a rule that functions shouldn't have side effects. Maybe it isn't clear at first what it means, but think about it this way: Function has one purpose, we pass arguments to it if needed, function do its magic and either returns the result or void. Its magic shouldn't do anything with other parameters inside the class, just with the passed ones. If it changes anything else, it should be refactored.

This section was a bit longer, but we covered a lot of important rules and what should be avoided while writing our code.

Comments

TLDR

• Avoid writing comments
• Make a code explain itself, then there is no need for comments
• A lot of comments is a sign of bad code
• Good comments: legal, todo, javadocs...
• Commented-out code is a sin

The first and most important rule about comments is not to write them. Avoid them as much as possible. Think of writing a comment in the code like littering the street. Why? Let's again go back to school days, probably we would think comments are helpful because they describe what the code does and maybe keep the track of all changes made through the development. And partially, yes, we would find that helpful, to read a comment as a description of some complex algorithm, but when we write code as in the previous section, like well-written prose, comments are unnecessary. Code should be self-explanatory, do you agree? When we write functions that are concise, short, and well named, comments really are like litter on the street. So if you see lots of comments in the code, that is a badly written code. Even in the development process, avoid the comments, avoid them to log the changes, because most of the modern IDEs today have that integrated. IntelliJ for example has Git to show history, when it was edited, by whom, and much more. Also, avoid commenting out the code you don't use anymore because you think you might need it at some point, but probably you won't. Right, we talked about bad comments, but actually, there are some use cases where comments are a good practice. The first example is Javadoc comments. For instance, when we use an external library sometimes we need more details, for example, about the function we call. In IntelliJ by pressing Ctrl + left mouse click, it opens the code file where we can find Javadoc written by the author of a class. This is an example of how Javadocs look:

  /**
   * Reads single {@link CodeBook} from database specified by {@link StaticValue}
   *
   * @param staticValue value to load from database
   * @param <I>         {@link javax.persistence.Id} type
   * @param <T>         {@link javax.persistence.Entity} type
   * @return single {@link CodeBook}
   */
  <I extends Number, T extends CodeBook<I>> T load(StaticValue<I, T> staticValue);

The method above is generic, and you will agree the comment above is quite useful. The second example of a good comment is a legal comment, and it can usually be found in short form in the header. Common parts of those comments are licenses, copyrights, references to legal documents, and others. One more example we will talk about is TODO comments. It is fine to write them sometimes, as a note what should be done, and to mark it as unfinished code, so a reader can understand why is something written in the way it is and what will be written in the future. To find them, IDEs like IntelliJ have a special menu for TODO comments, so you can find them easily.

Formatting

TLDR

• IDEs now do it for ourselves
• Its role is to make code easy to understand
• Each company or team have its own rules

This is a short topic because formatting depends on the company in which you work and your personal preferences, so there are no strict rules. Since there are no strict rules, every company has its formatting, its code style. Let's see two examples of different formatting.

    if (clause) {
        code_to_execute
    } else {
        code_to_execute
    }

Other style:

    if (clause) {
        code_to_execute
    } 
    else {
        code_to_execute
    }

Does this affect the performance? Thankfully no, but what is its role then? We are visual beings, as they say, you eat with your eyes first. The same rule applies to code. When you start to code and when you're reading the code above or in another file, the visual structure is very important. Its visual structure determines how easy the code is to read and understand. Have you ever seen minified CSS, HTML, or JS code? If you didn't google it. Imagine how hard it would be to understand what the website looks like...That is why the formatting is so important and why all IDEs today do it automatically for us. Those default IDE formatting settings are more than enough to use, even our team uses them daily as a formatting tool. And that is it, that is the role of formatting.

Conclusion

All right, a lot of topics were covered above and these take practice learning and making them a habit. Try the approach in which you write the solution for the problem first and clean the code afterward. That way, you'll have the image in your head of what the solution should look like after you write it down. Remember to name your variables, constants, methods, and classes properly, make sure that your functions do ONE thing and ONE thing only and classes should have ONE responsibility and ONE responsibility only. As your code grows, so should you as a professional, therefore clean your code. We have a lot more to cover so stay tuned for the next part of this blog.