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.

You May Also Like

33rd Degree day 2 review

Second day of 33rd had no keynotes, and thus was even more intense. A good conference is a conference, where every hour you have a hard dilemma, because there are just too many interesting presentations to see. 33rd was definitely such a conference, and the seconds day really shined.

There were two workshops going on through the day, one about JEE6 and another about parallel programming in Java. I was considering both, but decided to go for presentations instead. Being on the Spring side of the force, I know just as much JEE as I need, and with fantastic GPars (which has Fork/Join, actors, STM , and much more), I won't need to go back to Java concurrency for a while.

GEB - Very Groovy browser automation

Luke Daley works for Gradleware, and apart from being cheerful Australian, he's a commiter to Grails, Spock and a guy behind Geb, a  browser automation lib using WebDriver, similar to Selenium a bit (though without IDE and other features).

I have to admit, there was a time where I really hated Selenium. It just felt so wrong to be writing tests that way, slow, unproductive and against the beauty of TDD. For years I've been treating frontend as a completely different animal. Uncle Bob once said at a Ruby conference: "I'll tell you what my solution to frontend tests is: I just don't". But then, you can only go so far with complex GUIs without tests, and once I've started working with Wicket and its test framework, my perspective changed. If Wicked has one thing done right, it's the frontend testing framework. Sure tests are slow, on par with integration tests, but it is way better than anything where the browser has to start up front, and I could finally do TDD with it.

Working with Grails lately, I was more than eager to learn a proper way to do these kind of tests with Groovy.

GEB looks great. You build your own API for every page you have, using CSS selectors, very similar to jQuery, and then write your tests using your own DSL. Sounds a bit complicated, but assuming you are not doing simple HTML pages, this is probably the way to go fast. I'd have to verify that on a project though, since with frontend, too many things look good on paper and than fall out in code.

The presentation was great, Luke managed to answer all the questions and get people interested. On a side note, WebDriver may become a W3C standard soon, which would really easy browser manipulation for us. Apart from thing I expected Geb to have, there are some nice surprises like working with remote browsers (e.g. IE on remote machine), dumping HTML at the end of the test and even making screenshots (assuming you are not working with headless browser).

Micro services - Java, the Unix Way

James Lewis works for ThoughtWorks and gave a presentation, for which alone it was worth to go to Krakow. No, seriously, that was a gem I really didn't see coming. Let me explain what it was about and then why it was such a mind-opener.
ThoughtWorks had a client, a big investment bank, lots of cash, lots of requirements. They spent five weeks getting the analysis done on the highest possible level, without getting into details yet (JEDI: just enough design initially). The numbers were clear: it was enormous, it will take them forever to finish, and what's worse, requirements were contradictory. The system had to have all three guarantees of the CAP theorem, a thing which is PROVED to be impossible.
So how do you deal with such a request? Being ThoughtWorks you probably never say "we can't", and having an investment bank for a client, you already smell the mountains of freshly printed money. This isn't something you don't want to try, it's just scary and challenging as much as it gets.
And then, looking at the requirements and drawing initial architecture, they've reflected, that there is a way to see the light in this darkness, and not to end up with one, monstrous application, which would be hard to finish and impossible to maintain. They have analyzed flows of data, and came up with an idea.
What if we create several applications, each so small, that you can literally "fit it in your head", each communicating with a simple web protocol (Atom), each doing one thing and one thing only, each with it's own simple embedded web server, each working on it's own port, and finding out other services through some location mechanism. What if we don't treat the web as an external environment for our application, but instead build the system as if it was inside the web, with the advantages of all the web solutions, like proxies, caches, just adding a small queue before each service, to be able to turn it off and on, without loosing anything. And we could even use a different technology, with different pair of CAP guarantees, for each of those services/applications.
Now let me tell you why it's so important for me.
If you read this blog, you may have noticed the subtitle "fighting chaos in the Dark Age of Technology". It's there, because for my whole IT life I've been pursuing one goal: to be able to build things, that would be easy to maintain. Programming is a pure pleasure, and as long as you stay near the "hello world" kind of complexity, you have nothing but fun. If we ever feel burned out, demotivated or puzzled, it's when our systems grow so much, that we can no longer understand what's going on. We lose control. And from that point, it's usually just a way downward, towards complete chaos and pain.
All the architecture, all the ideas, practices and patterns, are there for just this reason - to move the border of complexity further, to make the size of "possible to fit in your head" larger. To postpone going into chaos. To bring order and understanding into our systems.
And that really works. With TDD, DDD, CQRS I can build things which are larger in terms of features, and simpler in terms of complexity. After discovering and understanding the methods (XP, Scrum/Kanbad) my next mental shift came with Domain Driven Design. I've learned the building block, the ideas and the main concept of Bounded Contexts. And that you can and should use a different architecture/tools for each of them, simplifying the code with the usage patterns of that specific context in your ming.
That has changed a lot in my life. No longer I have to choose one database, one language and one architecture for the whole application. I can divide and conquer, choose what I want to sacrifice and what advantages I want here, in this specific place of my app, not worrying about other places where it won't fit.
But there is one problem in here: the limit of technologies I'm using, to keep the system simple, and not require omnipotence to be able to maintain, to fix bugs or implement Change Requests.
And here is the accidental solution, ThoughtWorks' micro services bring: if you system is build of the web, of small services that do one thing only, and communicate through simple protocol (like Atom), there is little code to understand, and in case of bugs or Change Requests, you can just tear down one of the services. and build it anew.
James called that "Small enough to throw them away. Rewrite over maintain". Now, isn't that a brilliant idea? Say you have a system like that, build over seven years ago, and you've got a big bag of new requests from your client. Instead of re-learning old technologies, or paying extra effort to try to bring them up-to-date (which is often simply impossible), you decide which services you are going to rewrite using the best tools of your times, and you do it, never having to dig into the original code, except for specification tests.
Too good to be true? Well, there are caveats. First, you need DevOps in your teams, to get the benefits of the web inside your system, and to build in the we as opposite to against it. Second, integration can be tricky. Third, there is not enough of experience with this architecture, to make it safe. Unless... unless you realize, that UNIX was build this way, with small tools and pipes.
That, perhaps. is the best recommendation possible.

Concurrency without Pain in Pure Java

Throughout the whole conference, Grzegorz Duda had a publicly accessible wall, with sticky notes and two sides: what's bad and what's good. One of the note on the "bad" side was saying: "Sławek Sobótka and Paweł Lipiński at the same time? WTF?". 
I had the same thought. I wanted to see both. I was luckier though, since I'm pretty sure I'll yet be able too see their presentations this year, as 33rd is the first conference in a long run of conferences planned for 2012. Not being able to decide which one to see, I've decided to go for Venkat Subramaniam and his talk about concurrency. Unless we are lucky at 4Developers, we probably won't see Venkat again this year.
Unfortunately for me, the talk ("show" seems like a more proper word), was very basic, and while very entertaining, not deep enough for me. Venkat used Closure STM to show how bad concurrency is in pure Java, and how easy it is with STM. What can I say, it's been repeated so often, it's kind of obvious by now.
Venkat didn't have enough time to show the Actor model in Java. That's sad, as the further his talk, the more interesting it was. Perhaps there should be a few 90min sessions next year?

Smarter Testing with Spock

After the lunch, I had a chance to go for Sławek Sobótka again, but this time I've decided to listen to one of the commiters of Spock, the best thing in testing world since Mockito. 
Not really convinced? Gradle is using Spock (not surprisingly), Spring is starting to use Spock. I've had some experience with Spock, and it was fabulous. We even had a Spock workshop at TouK, lately. I wanted to see what Luke Daley can teach me in an hour. 
That was a time well spent. Apart from things I knew already, Luke explained how to share state between tests (@Shared), how to verify exceptions (thrown()), keep old values of variables (old()), how to parametrize description with @Unroll and #parameterName, how to set up data from db or whatever with <<, and a bit more advanced trick with mocking mechanism. Stubbing with closures was especially interesting.

What's new in Groovy 2.0?

Guillaume Laforge is the project lead of Groovy and his presentation was the opposite to what we could see earlier about next versions of Java. Most visible changes were already done in 1.8, with all the AST transformations, and Guillaume spent some time re-introducing them, but then he moved to 2.0, and here apart from multicatch in "throw", the major thing is static compilation and type checking.
We are in the days, were the performance difference between Java and Groovy falls to a mere 20%.  That's really little compared to where it all started from (orders of magnitude). That's cool. Also, after reading some posts and successful stories about Groovy++ use, I'd really like to try static compilation with this language
Someone from the audience asked a good question. Why not use Groovy++ as the base for static compilation instead. It turned out that Groovy++ author was also there. The main reason Guillaume gave, were small differences in how they want to handle internal things. If static compilation works fine with 2.0, Groovy++ may soon die, I guess.

Scala for the Intrigued


For the last talk this day, I've chosen a bit of Scala, by Venkat Subramaniam. That was unfortunately a completely basic introduction, and after spending 15 minutes listening about differences between var and val, I've left to get prepared to the BOF session, which I had with Maciek Próchniak.

BOF: Beautiful failures


I'm not in the position to review my own talk, and conclude whether it's failure was beautiful or not, but there is one things I've learned from it.
Never, under none circumstances, never drink five coffees the day you give a talk. To keep my mind active without being overwhelmed by all the interesting knowledge, I drank those five coffees, and to my surprise, when the talk started, the adrenaline shot brought me over the level, where you loose your breath, your pulse, and you start to loose control over your own voice. Not a really nice experience. I've had the effects of caffeine intoxication for the next two days. Lesson learned, I'm staying away from black beans for some time.
If you want the slides, you can find them here.
And that was the end of the day. We went to the party, to the afterparty, we got drunk, we got the soft-reset of our caches, and there came another day of the conference.

You can find my review from the last day in here.