DDevelopment & Design

Boy Scout Rule in practice

I’ve started doing Scrum in late 2008 (thanks to Piotr Burdyło). And though I’ve immediately seen benefits of practices like daily standups, sprint planning, reviews and retrospectives, I had a big problem with finding out how to deal with refactoring within this method.

Before Scrum, whenever I found a place in code that I thought should be done better, I’d put a TODO comment. I would return to these comments whenever I saved some time on what I was doing, or when I decided that it has already been too ugly for my standards. And since my standards are pretty tight, I’ve been  (and making others) refactoring quite often and it worked well for me.

When doing code review, I’d write TODOs, discuss them with the authors of the code, and make them do clean up as the top priority.

But what were my options in Scrum?

 

1. Don’t tell anybody, and just use your left over time to clean up the TODOs

This is against transparency, which Scrum is advocating, and against my moral backbone (no lying). And since you are evaluating tasks/features better and better, sooner or later you’ll have no left over time for TODOs anyway. Ergo: this won’t do.


2. Create additional tasks/features for refactoring

These are not visible from the product owner point of view, so some Scrum Masters are strongly pushing against them.

Ok, so I had to choose the second option, and make my Scrum Master live with it. But then, another problem emerged: unless you put a cleaning task for every class that has TODOs inside, sooner or later you’ll end up with tasks like “Do all the TODOs in my Big-Ugly-Module”.

This will happen because doing TODOs may have very low priority, because people in a hurry don’t bother doing them, and because some people don’t write TODOs at all, rather thinking: “yeah, this is ugly, lets fix it, when we fix the rest of the module”.

So it seams that your code is predestined to go down the great spiral of (bio)degradation, until your team can’t stand it anymore and somebody asks for a Scrum task to clean up the mess.

Is there a solution to this? Is there a third way?

Sure there is.

What if we can switch the direction the code quality goes in? What if our code can get better with time, instead of getting worse? That would be cool and it would solve our problem with TODOs in Scrum.

 

Boy Scout Rule

Leave every code you read in better shape than you found it.

Ha! That sounds like something everyone would sign under, and completely forget about a few minutes later, right? I mean, phrases like this sure look nice, but the real battlefield, the front-line, is a mean and messy place, right? You sit in your trench, trying to survive and keep the project alive, while bullets fly above your head. How can you try to make things better, when you couldn’t keep them clean in the first place? No way to do it, man, there is too little time, right?

Wrong.

Let me show you how to use this rule in practice, without any significant time burden.
Real case scenario

You are in the middle of writing an application, you take a task from the Scrum white-board. It’s a new feature. It says: “When importing orders, every shipping address should be verified. If it’s not in our shippable zone, it should be canceled (status change + send an email to the frontend application)”.

Ok, so you want to write a new acceptance test, for this feature. But first you need to know what to test, which part of the application is responsible for importing orders. You search the project structure, and you spot the OrderService interface. You nose tells you, that it should be somewhere there. You take a look.

public interface OrderService {
    /**
     * Imports orders.
     */

    public void importOrders();
    /**
     * Export orders.
     */
    public void exportOrders(DateTime date);

    /**
     * Create raport of placed orders
     */
    public OrderRaport createRaport(DateTime date);
}

Yeah, it’s there. The importOrders() method. You search for implementations, and as you expect, there is an OrderServiceImpl.

WTF is with this blur, you ask. It’s intentional. It’s how much you see during your first second of looking at the code. At the first second you, as a good writer, have already noticed two things:

  1. The method has 30 lines. It’s probably too long. Good method should be as short as possible. This, according to Robert C. Martin, means between 1 and 7 lines.
  2. The are several different BLOCKS of code.

What does the second thing mean? It means, that the author has noticed the first thing as well, so he decomposed the method into different blocks, by putting an empty line between them.

This is so natural, so popular, nobody thinks about it anymore. We put empty lines between different blocks of code, to help us read the code. When we read the code, we can skip such blocks, because we already know, what those blocks do.

We put together similar stuff, so we can put a name into our mind-map, that given block does something. Next, we can fast read and skip it consequently.

There are two big problems with such method:

  1. it requires you to read all the details (every line) of the code at least once, before you put names of those blocks in you mind
  2. it is not the way Object Oriented Programming is supposed to separate stuff that make a single concept

So how about fixing it, while we read it? We have to read and understand it anyway. How about not keeping the names of blocks in our memory, but putting them into the code instead.
Comment every block of code

You are doing it anyway, but instead of saving the names/explanations in your head, save them in the code for a moment.

So our code will look like this:

@Override
public void importOrders() {
//create date before importing from web service
    DateTime date = new DateTime();

//import data transfer objects from web service
    List orders = salesOrderWS.getSalesOrdersFromWebService();

//create summary for importing orders
    SalesOrderImport salesImport = new SalesOrderImport();

//for each order try
    for(SalesOrderExportDetail salesOrder : orders) {
        try {

//convert it to domain object
            Order order = new Order();
            order.setValue(new BigDecimal(salesOrder.getValue(), new MathContext(2, RoundingMode.HALF_EVEN)));
            order.setAddress(convertToAddress(salesOrder.getShippingAddress()));
            order.setBuyerAddress(convertToAddress(salesOrder.getBuyerAddress()));

//calculate vat
            double vat = calculator.vatRate(order.getAddress());
            order.setVatRate(vat);
            BigDecimal value = calculator.value(vat);
            order.setVatValue(value);

//calculate value in PLN
            Currency curr = order.getCurrency();
            if(curr != Currencies.PLN) {
                order.setPln(converter.convert(curr, order.getValue()));
            }

//save in repository
            orderRepository.save(order);

//prepare information whether it was succesfull                
            salesImport.imported(order);
        } catch(Exception e) {
            salesImport.notImported(e);
        }
    }

//save the summary for importing orders
    salesImport.setDate(date);
    orderRepository.save(salesImport);
}

 

Put the comments without any indentation, so that you won’t forget to get rid of them, and they won’t mix with other comments in the code. Make those comments verbose, make them say everything that is important on this level of abstraction. You are a programmer, you probably write faster than you think, so there was no additional time spend on this action.

Now for the second step.

 

Eliminate blocks of code using explicit names

The goal is to compress all the information from inside the block and the comment into one-liner. That saves the time the reader needs to get the meaning of the code next time. It usually means, replacing the block with a method, with a name created from the comment. So for example this:

//calculate vat
    double vat = calculator.vatRate(order.getAddress());
    order.setVatRate(vat);
    BigDecimal value = calculator.value(vat);
    order.setVatValue(vat);

 

will be replaced by this:

order.calculateVat();

 

Who said that the new method has to be in the same class? In this situation, it makes much more sense to put the use of the calculator into the order, after all it has all the data (we are using aspectj load time weaving to get the calculator into the order, but there are plenty of other methods if you don’t want to use aspectj).

And this:

order.calculateVat();
//convert it to domain object
    Order order = new Order();
    order.setValue(new BigDecimal(salesOrder.getValue(), new MathContext(2, RoundingMode.HALF_EVEN)));
    order.setAddress(convertToAddress(salesOrder.getShippingAddress()));
    order.setBuyerAddress(convertToAddress(salesOrder.getBuyerAddress()));

Will be replaced by this:

Order order = orderConverter.toOrder(salesOrderDto);

Here we have decided to put the body inside a new class: OrderConverter, because the convertion is a serious responsibility and the OrderService look like it’s got other things to do already (Single Responsibility Principle).

Even one-liner can be a bit fixed, for example in this part:

//create date before importing from web service
    DateTime date = new DateTime();

The comment is put, because the name of the variable does not communicate it’s intention. We can get rid of the comment if we give the necessary information to the reader in other way. For example like this:

DateTime dateImportStartedAt = new DateTime();

We should eliminate every block and split every part of the code that serves a different need. Even for-each loops and try-catch expressions. For-each responsibility is to iterate over a collection, and call someone else to do the job on an element. Try-catch is responsible for handling an error, so somebody else should do the unsafe job for it.

This is the Single Responsibility Principle on a method level. Make every method have only one reason to change, and your life (and the readability of your code) will be better.

Since these are very basic and easy refactoring steps, that your IDE does automatically with little effort from you, the time burden is insignificant. You had to understand the class anyway, and on the way, you’ve made it better, easier to understand. Now the part of your class responsible for imports looks like this:

@Override
public void importOrders() {
    DateTime dateImportStartedAt = new DateTime();
    List salesOrderDtos = salesOrderWebServiceClient.getSinceLastVisitOrderedByNumber();
    ImportSummary importSummary = tryToImportEach(salesOrderDtos, dateImportStartedAt);
    orderRepository.save(importSummary);
}

private ImportSummary tryToImportEach(List salesOrderDtos, DateTime dateImportStartedAt) {
    ImportSummary importSummary = new ImportSummary(dateImportStartedAt);
    for(SalesOrderDto salesOrderDto : salesOrderDtos) {
        importSummary.add(tryToImport(salesOrderDto));
    }
    return importSummary;
}

private OrderImportOutcome tryToImport(SalesOrderDto salesOrderDto) {
    OrderImportOutcome orderImportOutcome = null;
    try {
        orderImportOutcome = importOne(salesOrderDto);
    } catch(Exception e) {
        orderImportOutcome = new OrderImportFailure(e);
    }
    return orderImportOutcome;
}

private OrderImportSuccess importOne(SalesOrderDto salesOrderDto) {
    Order order = orderConverter.toOrder(salesOrderDto);
    order.calculateVat();
    order.updatePlnValueIfNeeded();
    orderRepository.save(order);
    return new OrderImportSuccess(order);
}

Every method has a meaningful name. No more than 7 lines of code per method. You don’t need to divide code into blocks, methods are an object oriented solution to divide the code. By the way, you’ve delegated some responsibilities to other classes. You’ve refined the code, made it more precise, more clear, more readable.

But somewhere in your head there is a voice saying: “Hey, I had just one, 30-lines method, now I have four, taking the same space. If I do it to other methods, I’m gonna have like 15 methods per class. I’ll get lost, it’s no way easier for me”.

But that is, because you have one more big mistake to fix.
The name is the responsibility

Have a look at the name of the interface: OrderService. Can you tell me what this class is responsible for? Every class should have just one responsibility, what is it for OrderService?

Everything about orders?

What kind of responsibility is “everything”? There is no such thing as “everything”? Thinking this way you can create single class named God-Emperor and end your OOP right there. You’ll never need other classes anymore.

No, my friend. The name SomethingService is a great mistake, just like a SomethingController or SomethingManager. I know quite a few SomethingManagers in real life, that you wouldn’t be able to find any responsibilities for. They are called that way, because this is what they do: NOTHING. They just are.

But we do not speak corporation-bullshit (even though some of us work at corporations), we are Agile developers, we are writers, we name things as they are.

So our OrderService interface should be split into, let’s say: OrdersImporter, OrdersExporter and OrdersReportFactory. That would make responsibilities and our intentions clear. If you do that, you end up with a WebServiceOrdersImporter implementation class, that has only 30 lines, 4 methods, only one of them public.

Now that’s simple, isn’t it?
Conclusion

It doesn’t have to be expansive or difficult to make code better while you are reading it.

You shouldn’t wait for the code to get really ugly. You shouldn’t write TODOs only for obvious problems. Every time you have to read and understand some code, ask yourself a question: Can I make it easier? Can I make it more readable? Can I make it better?

Take at least a small step, every time you answer is yes. Good is better than good-enough. This helps turn the tide, change the (bio)degradation into positive evolution. In the end, your code gets better with time.

No one writes good code right from the start. Good code evolves from normal code, by using the Boy Scout Rule.

And if you cannot find any way to make the code better, at least eliminate blocks of code with methods. Stick to “7 lines per method”, and “Single Responsibility of a method” rules.

Solution Architect @ TouK

— Previous article

Writer versus Constructor

Next article —

Open Code Review

You May Also Like
DDevelopment & Design

How to use mocks in controller tests

Even since I started to write tests for my Grails application I couldn't find many articles on using mocks. Everyone is talking about tests and TDD but if you search for it there isn't many articles.

Today I want to share with you a test with mocks for a simple and complete scenario. I have a simple application that can fetch Twitter tweets and present it to user. I use REST service and I use GET to fetch tweets by id like this: http://api.twitter.com/1/statuses/show/236024636775735296.json. You can copy and paste it into your browser to see a result.

My application uses Grails 2.1 with spock-0.6 for tests. I have TwitterReaderService that fetches tweets by id, then I parse a response into my Tweet class.


class TwitterReaderService {
    Tweet readTweet(String id) throws TwitterError {
        try {
            String jsonBody = callTwitter(id)
            Tweet parsedTweet = parseBody(jsonBody)
            return parsedTweet
        } catch (Throwable t) {
            throw new TwitterError(t)
        }
    }

    private String callTwitter(String id) {
        // TODO: implementation
    }

    private Tweet parseBody(String jsonBody) {
        // TODO: implementation
    }
}

class Tweet {
    String id
    String userId
    String username
    String text
    Date createdAt
}

class TwitterError extends RuntimeException {}

TwitterController plays main part here. Users call show action along with id of a tweet. This action is my subject under test. I've implemented some basic functionality. It's easier to focus on it while writing tests.


class TwitterController {
    def twitterReaderService

    def index() {
    }

    def show() {
        Tweet tweet = twitterReaderService.readTweet(params.id)
        if (tweet == null) {
            flash.message = 'Tweet not found'
            redirect(action: 'index')
            return
        }

        [tweet: tweet]
    }
}

Let's start writing a test from scratch. Most important thing here is that I use mock for my TwitterReaderService. I do not construct new TwitterReaderService(), because in this test I test only TwitterController. I am not interested in injected service. I know how this service is supposed to work and I am not interested in internals. So before every test I inject a twitterReaderServiceMock into controller:


import grails.test.mixin.TestFor
import spock.lang.Specification

@TestFor(TwitterController)
class TwitterControllerSpec extends Specification {
    TwitterReaderService twitterReaderServiceMock = Mock(TwitterReaderService)

    def setup() {
        controller.twitterReaderService = twitterReaderServiceMock
    }
}

Now it's time to think what scenarios I need to test. This line from TwitterReaderService is the most important:


Tweet readTweet(String id) throws TwitterError

You must think of this method like a black box right now. You know nothing of internals from controller's point of view. You're only interested what can be returned for you:

  • a TwitterError can be thrown
  • null can be returned
  • Tweet instance can be returned

This list is your test blueprint. Now answer a simple question for each element: "What do I want my controller to do in this situation?" and you have plan test:

  • show action should redirect to index if TwitterError is thrown and inform about error
  • show action should redirect to index and inform if tweet is not found
  • show action should show found tweet

That was easy and straightforward! And now is the best part: we use twitterReaderServiceMock to mock each of these three scenarios!

In Spock there is a good documentation about interaction with mocks. You declare what methods are called, how many times, what parameters are given and what should be returned. Remember a black box? Mock is your black box with detailed instruction, e.g.: I expect you that if receive exactly one call to readTweet with parameter '1' then you should throw me a TwitterError. Rephrase this sentence out loud and look at this:


1 * twitterReaderServiceMock.readTweet('1') >> { throw new TwitterError() }

This is a valid interaction definition on mock! It's that easy! Here is a complete test that fails for now:


import grails.test.mixin.TestFor
import spock.lang.Specification

@TestFor(TwitterController)
class TwitterControllerSpec extends Specification {
    TwitterReaderService twitterReaderServiceMock = Mock(TwitterReaderService)

    def setup() {
        controller.twitterReaderService = twitterReaderServiceMock
    }

    def "show should redirect to index if TwitterError is thrown"() {
        given:
            controller.params.id = '1'
        when:
            controller.show()
        then:
            1 * twitterReaderServiceMock.readTweet('1') >> { throw new TwitterError() }
            0 * _._
            flash.message == 'There was an error on fetching your tweet'
            response.redirectUrl == '/twitter/index'
    }
}

| Failure:  show should redirect to index if TwitterError is thrown(pl.refaktor.twitter.TwitterControllerSpec)
|  pl.refaktor.twitter.TwitterError
 at pl.refaktor.twitter.TwitterControllerSpec.show should redirect to index if TwitterError is thrown_closure1(TwitterControllerSpec.groovy:29)

You may notice 0 * _._ notation. It says: I don't want any other mocks or any other methods called. Fail this test if something is called! It's a good practice to ensure that there are no more interactions than you want.

Ok, now I need to implement controller logic to handle TwitterError.


class TwitterController {

    def twitterReaderService

    def index() {
    }

    def show() {
        Tweet tweet

        try {
            tweet = twitterReaderService.readTweet(params.id)
        } catch (TwitterError e) {
            log.error(e)
            flash.message = 'There was an error on fetching your tweet'
            redirect(action: 'index')
            return
        }

        [tweet: tweet]
    }
}

My tests passes! We have two scenarios left. Rule stays the same: TwitterReaderService returns something and we test against it. So this line is the heart of each test, change only returned values after >>:


1 * twitterReaderServiceMock.readTweet('1') >> { throw new TwitterError() }

Here is a complete test for three scenarios and controller that passes it.


import grails.test.mixin.TestFor
import spock.lang.Specification

@TestFor(TwitterController)
class TwitterControllerSpec extends Specification {

    TwitterReaderService twitterReaderServiceMock = Mock(TwitterReaderService)

    def setup() {
        controller.twitterReaderService = twitterReaderServiceMock
    }

    def "show should redirect to index if TwitterError is thrown"() {
        given:
            controller.params.id = '1'
        when:
            controller.show()
        then:
            1 * twitterReaderServiceMock.readTweet('1') >> { throw new TwitterError() }
            0 * _._
            flash.message == 'There was an error on fetching your tweet'
            response.redirectUrl == '/twitter/index'
    }

    def "show should inform about not found tweet"() {
        given:
            controller.params.id = '1'
        when:
            controller.show()
        then:
            1 * twitterReaderServiceMock.readTweet('1') >> null
            0 * _._
            flash.message == 'Tweet not found'
            response.redirectUrl == '/twitter/index'
    }


    def "show should show found tweet"() {
        given:
            controller.params.id = '1'
        when:
            controller.show()
        then:
            1 * twitterReaderServiceMock.readTweet('1') >> new Tweet()
            0 * _._
            flash.message == null
            response.status == 200
    }
}

class TwitterController {

    def twitterReaderService

    def index() {
    }

    def show() {
        Tweet tweet

        try {
            tweet = twitterReaderService.readTweet(params.id)
        } catch (TwitterError e) {
            log.error(e)
            flash.message = 'There was an error on fetching your tweet'
            redirect(action: 'index')
            return
        }

        if (tweet == null) {
            flash.message = 'Tweet not found'
            redirect(action: 'index')
            return
        }

        [tweet: tweet]
    }
}

The most important thing here is that we've tested controller-service interaction without logic implementation in service! That's why mock technique is so useful. It decouples your dependencies and let you focus on exactly one subject under test. Happy testing!