Bad Code Smells: on Refactoring of BibSpace

You learn nothing if you do not refactor your code. Refactoring makes you and your code better. Refactoring a public code may be even more effective as you may be judged.

Bad Code Smells

In the book Refactoring - Improving the Design of Existing Code, Martin Fowler quoted Kent Beck's Grandma who said:

If it stinks, change it.

So started my journey with refactoring BibSpace. One lovely day, I noticed that the system is so big that I am able to maintain only its parts whereas the rest could slowly rot. In the ideal case, your lovely active users should report you all the smells in the form of bugs, but this is shame for the developer not to discover such smells on their1 own.

Why refactor?

There are multiple reasons why we should refactor code. Many were mentioned in Fowler's book, yet I would like to stress some more.

Because we learn

We learn new things, approaches, we discover libraries and want to apply them on our code to exercise. We develop ourselves by applying new techniques to the code and so we get better.

Because we design things badly

Sometimes, we want the things just to work and we bunch some lines of code that hopefully will work. Next, we start adding features – damn, this oneliner is not going to stay as such for long – and all of a sudden, we had thousand lines of code. It comes the time to clean it up, but sometimes it is better to rewrite it from scratch... and I did it in year 2014 when BibSpace was know as the system of Piotr for publications.

Because there is no time

I can't imagine a developer would say, that there is no time to writ a good code. But a person, whose main responsibility is not programming is allowed to say so. Usually, some features are requested to work immediately (or the phone will ring) and so we may end up with bad design and messy code.

How I refactor BibSpace

I was able to devote relatively little time to BibSpace development. By developing it, I learned how to code in Perl and use Mojolicious. Such a process requires continuous refactoring as you learn new things. I share some experiences and thoughts about refactoring BibSpace. But please remember, I still learn and some thing may change.

Mojolicious way of doing stuff

Mojolicious is a typical example of MVC framework for Perl. I haven't tried the competition, but the lovely raptor convinced me to try Mojolicious as first.

Mojolicious binds routes (urls) with controllers that process data and render a template. It says If you call this url in your browser, I take these data and pass it to that function. The function does something with the data and then renders a HTML template that you see on your screen. Easy. In the Internet, there are multiple examples how to do this. However, most of them are small applications that serve as tutorials. Several examples are bigger, but they may be too big for the beginners. The Mojolicious cookbook is nice, but I wish someone told me that...

Frontend is a separate application2

You should separate the web layer from your meat-code. This is not only about separating the HTML templates from the code! Your code should work without a web framework and you should be able to switch web frameworks fast.

To achieve this, we need to write thin controllers. It means, the code in a controller function should contain a call to as few functions as possible. Why? I explain soon, first example.

Bad

The controller function is far from being nice code, but yes, I have written it. The function contains direct calls to the database, is not commented, can be optimized in many places. But here comes the question worth 100 points: how would you test it?

sub add_author_post {  
    my $self       = shift;
    my $dbh        = $self->app->db;
    my $new_master = $self->param('new_master');

    if ( defined $new_master ) {

        my $existing_author = get_master_id_for_uid( $dbh, $new_master );
        if ( $existing_author == -1 ) {

            my $sth = $dbh->prepare(
                'INSERT INTO Author(uid, master) VALUES(?, ?)');
            $sth->execute( $new_master, $new_master );
            my $aid = $dbh->last_insert_id( undef, undef, 'Author', 'id' );

            my $sth2 = $dbh->prepare(
                'UPDATE Author SET master_id=?, display=1 WHERE id=?');
            $sth2->execute( $aid, $aid );

            if ( !defined $aid ) {
                $self->flash( msg =>
                        "Something went wrong. The Author has not been added"
                );
                $self->redirect_to( $self->url_for('/authors/add') );
                return;
            }
            $self->redirect_to( $self->url_for('/authors/edit/') . $aid );
            return;
        }
        else {
            $self->flash( msg =>
                    "Author with such MasterID exists! Pick a different one."
            );
            $self->redirect_to( $self->url_for('/authors/add') );
            return;
        }
    }
    $self->redirect_to( $self->url_for('/authors/add') );
}

Better

This function is much better. It is short, it delegates the work to an other function, and focuses only on the controller stuff. Unfortunately, it still passes the controller object $self to the function add_team_for_author. Smells bad.

sub add_to_team {  
    my $self      = shift;
    my $dbh       = $self->app->db;
    my $master_id = $self->param('id');
    my $team_id   = $self->param('tid');

    add_team_for_author( $self, $master_id, $team_id );

    $self->redirect_to( $self->get_referrer );
}

Best

In the following function, the meat is hidden behind $mentry->hide($dbh);. The rest is purely controller stuff: check if such object exists, get object, prepare message, redirect.

sub hide_entry {  
    my $self = shift;
    my $id   = $self->param('id');

    my $mentry = MEntry->static_get( $self->app->db, $id );
    if ( defined $mentry ) {
        $mentry->hide($dbh);
    }
    else{
        $self->flash( msg => "There is no entry with id $id" );
    }
    $self->redirect_to( $self->get_referrer );
}

The take-away-message is the following:

Develop the functionality (the meat) as a separate function and call it from the controller.

Benefits

The most important benefit is that you can test it. You can test the function $mentry->hide, you can test the controller, and you can test the template3 separately. So, you can easily find a bug - either in the code, the controller or the template. In the bad and better examples, you cannot test the code, controller, and template separately. You are forced to test two or three of of three parts together.

Current status

So, I am in the middle of refactoring BibSpace. I learn a lot during doing it and for sure I will later refactor my refactorings.

Milestone: Test coverage nearly 100%

Martin Fowler says, that there is no refactoring without a test suite with 100% coverage. True. I want to feel safe changing the code without breaking anything, so I write the tests currently. I have reached the coverage of 65% at the time of writing and I hope to hit 90% soon.

Milestone: Separate Functionalities, Controller, and Templates

In parallel, I follow the idea described in this post to separate my functionalities from the controller and the template.

  1. I build Moose classes to encapsulate code that relates to an object;
  2. I move away the code from controller functions and place it in a middle layer, so called F*4 functions. By testing the F* functions I cover the functional tests;
  3. I make the stash tiny. Stash is a place where the objects are passed from the controller to a template.

Milestone: Learn

I learn even more while I refactor. I discover features of Moose, new Mojolicious helpers that seemed useless before, I correct bugs and understand how they happen. I also discover Mojolicious bugs and challenges in that I post to stackoverflow to learn even more from other's answers.

Goal

I wish to have 100% code coverage of the Moose objects and the F* functions. Next, I will refactor templates and messages displayed to the user and move to testing the frontend. It is going to take some time until I get there5. After that, I plan to refactor for performance and then move to adding new cool features that roam around my head :)

How BibSpace smells?

Rather bad, I would say. But I hope this is the smell of a good blue cheese that served well can surprise positively :)


  1. I hope female developer will also read this.

  2. As said by Arkency in Frontend is a separate application.

  3. In the examples, I redirect to the template pointed by function

    undefined, but the template is involved in the rendering of course.

  4. I should have picked a better prefix...

  5. I code BibSpace mostly in my free time and there is not much of it left.