Clean Code, Dirty Code


Phew! It feels like forever since I wrote a blog post; I have been extremely busy – finished an internship and had to travel back to school. Well, I actually have a couple of drafts but didn’t get to publishing them. Am I too lazy? Na you sabi… :).

I don’t know why I seem to keep talking about code and JavaScript nowadays, I just can’t seem to stop; probably because I think JavaScript is the next BIG thing; what other language is currently experiencing rapid widespread adoption on all major platforms? Read this and this if you want to know more.

Clean code

clean code is what we developers should aspire to write, it is clean, minimal, simple, easy to understand, free of extraneous details, it is simply BEAUTIFUL (there I go again… drooling about software right?). Ideally you shouldn’t be able to remove pieces of it without breaking the system. Less code is always better, the less code you have to write; the better your system. Jeff Atwood actually says the best solution is no code at all.

So what makes code dirty?

Anything that makes it a pain to modify, work with or extend. Abstruse code, duplicated logic, global state and variables, tightly-coupled dependencies, contorted flow (I mean those convoluted loops and if constructs), wrongly-exposed variables (it-shouldn’t-be-there-but-I-can’t-help-it-syndrome) etc. In fact, if your codebase doesn’t read easily then you should get into it and clean it up. It’ll go a long long way in saving you painful work in the future. I have worked on a multi-thousand line codebase (> 25k lines and written by lots of people) and found it easy to handle and understand; in contrast I also inherited some smaller codebase (< 4k lines and written by a single person) and it was a pain to extend.

How to get there? Do Code reviews, relentlessly refactor and make sure you don’t leave anything to chance; you have to be professional about your job and code.

Here is an example in pseudocode:

function showMessage(message){
{
    if(message != null){
        show(message);
    }
    else{
        show(defaultMessage); 
        // This is the default action to do
    }
}
function showMessage(message){
{
    if(message == null){
        message = defaultMessage;
    }
    show(message);
}

Isn’t the second version just cleaner and easier to use? Here is another example:

    if(condition)
    {
        //do Something
        alertUser();     
    }
    else{
        //do Some other thing
        alertUser();
    }

The alertUser() function is getting called in both conditional branches so it can be moved out. Less code, simpler code, easier to read. Here is the cleaner version:

    if(condition)
    {
        //do Something
    }
    else{
        //do Some other thing
    }
    alertUser();

I feel that writing clean code is even more important than solving the problem you set out to solve. Most of our time is spent extending code, writing fixes and maintaining existing code. Keeping it simple and nice at the start will go a long way in making your job and mine easier.

11 thoughts on “Clean Code, Dirty Code

  1. In your first example you make matters worse.
    Usually a String can be passed as final (cannot be modified). In your second example you do an assignment creating a additional copy of a String on the stack.

    If you want to do it ‘better’, then do it like this:

    function showMessage(message){
    {
    msg:String;
    if(message == null){
    msg = defaultMessage;
    } else {
    msg = message;
    }
    show(msg);
    }

    That can be very well optimized by the compiler in most languages and message can be passed as final if your language allows you to. I always try to avoid re-use of objects and try to handle them as ‘const’, eg don’t modify objects after assignment, that better for execution speed and thread safety.

    However, you shouldn’t allow passing that message as null in the first place. It still will frown users when you pass a empty string to the showMessage function because they see a empty alert message.
    I would assert on null, and test for empty string in showMessage

    Your first and second examples are trying to show the same, call a function within a condition twice. I am not sure how your second sample is different.

    Like

    1. Thanks a lot rvt.

      It appears that you use static languages a lot; the examples are not tied to any particular paradigm; I mentioned that it’s just pseudocode and not production code.

      The examples are not limited to displaying messages or alerting users alone; the same concepts can be applying in a variety of similar scenarios.

      The first example shows that you can eliminate clutter and write simpler code instead of having to use two branches. It’s not limited to displaying messages alone.

      The seconds shows that if you have the same body of code being executed in branches of a conditional; you can always move that code out as the invariable always hold.

      Thanks for your views too.

      Like

      1. I agree with rvt for the first example – the “clean-up” code is less clear than the first, as it assumes that the second branch (“show (message)”) will not be executed if the first branch is executed, which is a complete fallacy in most procedural languages (e.g. VB, C++, Java, etc) In all these languages, the next line of code after the braces is ALWAYS executed, and so you will finish up showing an empty message box (as virtually all programs require a box to pop up (or something similar) in order to show a message).

        Like

  2. Cool stuff! A recommended book like ‘clean code’ by Robert C. Martin would be worth the mention. It treats this subject in detail.

    Like

Leave a comment

This site uses Akismet to reduce spam. Learn how your comment data is processed.