Animating Developers, 4 months later

Four months ago I had a chance to present Agile Skills Project and experiments we do at TouK, to create a learning ecosystem and culture of constant improvement. Time to report progress of our experiments.

Map of New Ideas

The request came from our HQ (Headquarters, which is our group of main company owners): lets use Jira to gather ideas and initiatives about how to improve our company. You know, Keizen style.

The reasoning is, that though some people, including myself, always loudly present their opinions, new concepts, and basically try to change the company from the ground up (whether the stakeholders like it or not), others need some encouragement and a safe way to suggest ideas for improvement.

Our map of new ideas is a simple Jira project, where anybody can suggest anything he/she thinks is worthwhile. The new idea is verified by a group responsible for deciding whether it's in line with company's profile and possible with our current resources. If so, the idea and needed resources, are assigned to someone who, from now on, is responsible for making it happen. It doesn't have to be the person proposing the idea, but quite often it is. The ideas are also grouped by the area they correspond to, one from culture, evolution, fitness, relations, survival or contribution.

As for the moment, our statistics look as follow:

• Proposed: 36
• Selected: 6 (waiting for a victim)
• Assigned: 8 (but not started yet)
• Ongoing: 6
• Refused: 1
• Finished: 26

Things that appear on the map vary, including for example other experiments described here (Weekly Workshops, OCRs), open sourcing some libraries, drawing diagrams of all the systems we create for our main clients (very useful for new developers) or creating database environment for regression tests on Oracle.

As you see, it goes quite well, and I believe it's a great way to get your developers involved in the company improvement. As a side effect, people feel they have more influence over where the company is heading, which is always good and gives a nice motivational kick (“Hey look, now I'm not only building software, I'm building my company as well!”).

Thing to remember: this is a map for company-wide ideas. In the beginning there was a confusion over whether stuff about how to improve one's project/team should be here, but since our teams are self-organized, there is no need to involve the rest of our company. The power is in one's hands anyway.


Weekly Workshops

Every Friday at 4pm, we have a one hour long technology workshop open to everyone. It usually takes the form of a lecture, with slides, some coding, and examples, but the form is open. Other features are not, and that is very important for it's success.

This is a concept I've borrowed from Supermedia Interactive, when Piotr Szarwas was still the head of the development department and my boss. I'm sure it's popular in many companies, but Piotr taught me how to do it correctly.

The goal is to share knowledge and learn new things.

The date and time are set in stone. Every Friday, 4pm. It's during our work hours, so we had to choose time, which won't stop or slow down our normal development. Since our sprints are week long, Friday 4pm is usually after the retrospective anyway. Let's face it: at 4pm on Friday, we are either busy putting down some fires, learning, or slacking off anyway.

Everyone can present anything, as long as there are people who want to listen to. We keep a calendar on confluence for coordination. We also have a page with suggested topics, where people put things they would like to listen about, for others to pick up and investigate. One rule is very important thought: if we do not have a volunteer for next Friday, we are going to pick one.

Yes, the participation (as a lecturer) is mandatory for everyone. It basically means, each developer should give at least one lecture per year. No excuses.

At first, there was some turmoil about that. People do not like to be forced to do anything. But the first drawing showed that it's a good idea. Randomly chosen people were giving great presentations. The reason is, that everyone has something interesting to show, it's just that a carrot of general appreciation is not enough sometimes. Sometimes you also need a stick. That's how a human mind works.

So what are our workshops about? Here are a few topic examples:

• Clojure – lisp for java programmers.
• Rapid Application  Development using Grails and Vaadin
• Smartclient: RAD even faster.
• Apache Hadoop and projects around it.
• JBoss Envers – easy entity auditing
• How to get users from Gmail and Facebook: OAuth 2.0, OpenId, Spring Security and Spring Social in web applications

Weekly OCRs

I've already described that  in here so I'm not going to repeat myself. What has changed, is that we have it on a weekly basis, we have it split in two: java and database OCRs separately, and that we have designated people responsible for making it happen (not for leading the meeting, but for choosing a victim, if there is no volunteer).

Java OCRs are held on Friday at 3pm, for all the same reasons as given above for workshops.

I've noticed that some people see it as a chance to brainstorm and refactor some really troublesome code, they work with, which they normally do not want or have no time to touch. That is a great idea, and since we use our own code for OCRs, and since we actually try to commit the changes, it's more like getting help for your project, than only sharing some thoughts.

Database teams do it in a little different way, but not being there, I'm not inclined to explain it.


Quest system during work hours

The quest ecosystem described on Agile Skills Project web site, about which I've been talking on Agile Warsaw is a great self-motivational tool, but the question we had, was how a company can help people get it started? My idea was, that having everyone choose his own mentor and booking an hour a month to talk with him about the progress or lack thereof, can do the trick. But that assumes, people are working back home to meet their goals, which is not true for everyone. Witek Wolejszo wanted to check whether making it a work-time activity will spread the technique to those, who don't want to use their free time.

Eight people with different background and free time activity were chosen for a month long experiment, where they would choose an educational goal and try to achieve it within working hours.

The feedback was generally negative. People complained for these main obstacles:

low quest priority (everything for the client is more important) ends with process starvation
quests as defined were things to be done alone, and we prefer working with other people (pairs)
no motivation from quest done for yourself only, when it doesn't have an immediate use and no one else is waiting for the outcomes

At the same time people didn't think it was a totally bad idea, but would rather like, if quests were somehow corresponding to our current products and projects and had defined timeboxes.

This made us close the experiment and start with two others, described below. We do not know yet how these will work out.


Task exchange board.

When creating a backlog for a project, there are usually some tasks for which the domain knowledge available to team members is not needed. This include investigation into new libraries and technologies (spikes), setting up things very technology oriented (auditing by Jboss Envers, excel export via  Apache POI, etc.), creating black-box and/or open source libraries, writing proof of concepts or collecting and releasing some tools from the project as an Open Source libraries. All those tasks are candidates for Task Exchange Board.

When a backlog is done, the team can decide which tasks to put onto the board. These should be estimated like on a sprint planning, should have a defined time expectation or a timebox (in case of spikes) and Definition of Done.

Once on the Board, anyone in the company, even from different project or department, as long as he has enough time (i.e. his PM/team leader has nothing against it), can pick up an interesting task and do it using the budget of the team which created the task.

This way, people can do something important and interesting in a new technology, on full time, without changing the current project. If you have enough of what you are doing, this may be a productive brake for you. If you are currently only supporting some existing project or waiting for bug reports, why not to do something interesting and learn something new on the way? This may be a equivalent  of a quest for those, who do not have any time at home.

These tasks, may also be done by members of the team which created them.

One important thing has to be understood: since the team creating the task doesn't know who is going to take it, it's up to the guy taking the task, to finish within or before estimated time. Basically, if you are not sure you can finish this on time, or do not want to risk spending your free hours on it, don't take it. This should not slow down the development time. If it did, no team would put another task on the board.


What technologies you want to work with?

This time-fridge contains
technologies we do not use anymore.
ZX-Spectrum, Commodore 64,
Commodore 128; Amstrad,
Macbook Air...
Old kind of stuff.
We are an old company :)

We have a team of guys responsible for assigning people to new projects or moving them between existing. This team has a sky level view on what's currently going on in the whole company, all new and planned projects.

So far, the team was making up decisions based on their own perspective of what one is capable of. We wanted to change it a bit, to make TouK a better place for people who feel a rush on some technology, so we created a confluence page, where everyone can submit what technologies he would like to work with.

Now, this looks very simple, but frankly speaking, I wasn't aware (and I believe nobody else was) of how often some of technologies would be mentioned by different people. As an effect, we can see now, that we have a lot of developers wanting to take part in a full-Grails, Java free project. While in theory, every team can decide for itself, what technologies it uses, seldom someone would take such a drastic, risky step, as changing the language for the whole project. By gathering information about what people would like to work with, we can create a team very dedicated and motivated to use some technology. That may pay off pretty well.

We shall see, whether the guys assigning people to projects, will take that into account.

More to go
There is a lot of stuff, we haven't tried yet. This includes for example mentoring, RPG character cards, exchanging knowledge with other companies by switching developers for a few days. Hopefully, I'll have more to report in a few months.

Getting rid of null parameters with a simple spring aspect

What is the most hated and at the same time the most popular exception in the world?

I bet it's the NullPointerException.

NullPointerException can mean anything, from simple “ups, I didn't think that can be null” to hours and days of debugging of third-party libraries (try using Dozer for complicated transformations, I dare you).

The funny thing is, it's trivial to get rid of all the NullPointerExceptions in your code. This triviality is a side effect of a technique called “Design by Contract”.

I won't go into much details about the theory, you can find everything you need on Wikipedia, but in the nutshell Design by Contract means:

• each method has a precondition (what it expects before being called)
• each method has a postcondition (what it guarantees, what is returned)
• each class has an constraint on its state (class invariant)

So at the beginning of each method you check whether preconditions are met, at the end, whether postconditions and invariant are met, and if something's wrong you throw an exception saying what is wrong.

Using Spring's internal static methods that throw appropriate exceptions (IllegalArgumentException), it can look something like this:

import static org.springframework.util.Assert.notNull;
import static org.springframework.util.StringUtils.hasText;

public class BranchCreator {
    public Story createNewBranch(Story story, User user, String title) {
        verifyParameters(story, user, title);
        Story branch = //... the body of the class returnig an object
        verifyRetunedValue(branch);
        return branch;
    }

    private void verifyParameters(Story story, User user, String title) {
        notNull(story);
        notNull(user);
        hasText(title);
    }

    private void verifyRetunedValue(Story branch) {
        notNull(branch);
    }
}

You can also use Validate class from apache commons instead of spring's notNull/hasText.

Usually I just check preconditions and write tests for postconditions and constraints. But still, this is all boiler plate code. To move it out of your class, you can use many Design by Contract libraries, for example SpringContracts, or Contract4J. Either way you end up checking the preconditions on every public method.

And guess what? Except for Data Transfer Objects and some setters, every public method I write expects its parameters NOT to be null.

So to save us some writing of this boiler plate ocde, how about adding a simple aspect that will make it impossible in the whole application, to pass null to anything other than DTOs and setters? Without any additional libraries (I assume you are already using Spring Framework), annotations, and what else.

Why would I like to not allow for nulls in parameters? Because we have method overloading in modern languages. Seriously, how often do you want to see something like this:

Address address = AddressFactory.create(null, null, null, null);

And this is not much better either

Microsoft.Office.Interop.Excel.Workbook theWorkbook = ExcelObj.Workbooks.Open(openFileDialog.FileName, Type.Missing, Type.Missing, Type.Missing, Type.Missing, Type.Missing, Type.Missing, Type.Missing, Type.Missing, Type.Missing, Type.Missing, Type.Missing, Type.Missing, Type.Missing, Type.Missing);

The solution

So here is a simple solution: you add one class to your project and a few lines of spring IoC configuration.

The class (aspect) looks like this:

import org.aspectj.lang.JoinPoint;
import static org.springframework.util.Assert.notNull;

public class NotNullParametersAspect {
    public void throwExceptionIfParametersAreNull(JoinPoint joinPoint) {
        for(Object argument : joinPoint.getArgs()) {
            notNull(argument);
        }
    }
}

And the spring configuration is here (remember to change the namespace to your project).

<aop:config proxy-target-class="true"> 
    <aop:aspect ref="notNullParametersAspect">
        <aop:pointcut expression="execution(public * eu.solidcraft.*..*.*(..))
                          &amp;&amp; !execution(public * eu.solidcraft.*..*Dto.*(..))
                          &amp;&amp; !execution(public * eu.solidcraft.*..*.set*(..))" id="allPublicApplicationOperationsExceptDtoAndSetters"> 
            <aop:before method="throwExceptionIfParametersAreNull" pointcut-ref="allPublicApplicationOperationsExceptDtoAndSetters"></aop:before>     
        </aop:pointcut> 
 
        <task:annotation-driven>
            <bean class="eu.solidcraft.aspects.NotNullParametersAspect" id="notNullParametersAspect"></bean>
        </task:annotation-driven>
    </aop:aspect>
</aop:config>

The "&&" is no error, it's just && condition escaped in xml. If you do not understand aspectj pointcut definition syntaxt, here is a little cheat sheet.

And here is a test telling us that the configuration is succesfull.

public class NotNullParametersAspectIntegrationTest extends AbstractIntegrationTest {
    @Resource(name = "userFeedbackFacade")
    private UserFeedbackFacade userFeedbackFacade;

    @Test(expected = IllegalArgumentException.class)
    public void shouldThrowExceptionIfParametersAreNull() {
        //when
        userFeedbackFacade.sendFeedback(null);

        //then exception is thrown
    }

    @Test
    public void shouldNotThrowExceptionForNullParametersOnDto() {
        //when
        UserBookmarkDto userBookmarkDto = new UserBookmarkDto();
        userBookmarkDto.withChapter(null);
        StoryAncestorDto ancestorDto = new StoryAncestorDto(null, null, null, null);

        //then no exception is thrown
    }
} 

AbstractIntegrationTest is a simple class that starts the spring test context. You can use AbstractTransactionalJUnit4SpringContextTests with @ContextConfiguration(..) instead.

The catch

Ah yes, there is a catch. Since spring AOP uses either J2SE dynamic proxies basing on an interface or aspectj CGLIB proxies, every class will either need an interface (for simple proxy based aspect weaving) or a constructor without any parameters (for cglib weaving). The good news is that the constructor can be private.

Open Code Review

Łukasz Lenart at Warszawa Design Patterns Study Group gave a very interesting presentation about Open Code Review in his company (you can read the details here). I've failed to notice whether it's his own idea, or somebody's else, but that's not as important as the fact how well it works.

Open Code Review, in short, is when a small group of programmers (up to 6) meet once in a while for a brainstorming session, take some printed out code, and try to find out, for an hour or so, what could be done better.

It's basically about sharing knowledge and insight with others. It can be a very beneficial experience, especially for teams that do not use Pair Programming, or to pairs that do not switch very often, but I think anyone can find it useful.

The power of this technique is reflection. When we code, when we create software, we often find ourselves thinking in a specific way. We tend to take things for granted. We start to look at the code with our mind set on a particular solution, and as a result, we can't see the forest for the trees. This is how human mind works, it's nothing new. After a few weeks of walking the same route, you stop noticing the buildings along the way.

It's also one of the reasons why Pair Programming is so efficient. “You cannot solve a problem with the same mind-set that got you into the problem in the first place”, as some believe Einstein said. Having another mind next to you, looking at the same code, gives a chance to spot the problem as it is being created, but more importantly, it is very helpful in the process of discovering the best design. Open Code Review works in a similar way. It allows you to learn different mindsets and different styles of solving problems. Nothing is as refreshing as being able to see from other points of view.

Apart from this, OCR meetings also help in spreading knowledge (people read different books) and experience (people work on different projects with different technologies and problems).

This idea inspired me to organize similar meetings at TouK and I've got very good feedback so far. Our formula crystallized by looking at the the past and finding out what was cool and what was not. Our take on Open Code Review is quite different to what Łukasz proposed.

But first, things that we do the same way: we take maximum 6 people (plus the moderator), we gather them up in a conference room, we hand out code printed on paper.

The code we take is not Open Source, as Łukasz suggested. His reasoning was, that if you take somebody's code, it could turn personal, we all put some heart into our work, after all. This is where we disagree: we have found it very valuable to take internal code from our projects. We are not afraid of our code being criticized. As Robert C. Martin wrote: every good programmer should get used to professional code review. Bad programmers can benefit from it even better. And if you have some Prima Donnas in your team, unwilling to hear constructive critique to their code... screw them. Nobody's able to work with them anyways.

Second thing we do differently, is that we do not comment the code on paper. We actually have a working environment (IDE and stuff) with three keyboards/mice, thanks to USB hubs. We either comment the code directly, writing TODOs, for bigger changes that would take more time than it's left, or we do the refactoring right away. This allows us to make a commit to the repository after the session and improve some project a bit. It's also much easier to talk about the code, when you have a keyboard at hand. Sometimes it's just way simpler to show what you mean than to explain it with words.

We also take a bit longer for each meeting: usually an hour and a half, or two hours. This is the optimum time, to learn and do something meaningful, and not get tired. And we do not split attendees into teams, we want people to talk a lot, and we want people to talk with each other.

So what is our recipe for an Open Code Review? It's like this:

Ingridients

1. Invite anyone who wants to participate, from all over company, but take no more than 4 to 6 programmers plus a moderator
2. Take a conference room with a laptop, three or four keyboards/mice, and a projector
3. Set up the meeting for 1.5 or 2 hours
4. Moderator prints out a class (or a few classes) that
1. has between 150 and 300 lines of code
2. has unit/integration tests for all non trivial methods
3. is from a project that works with our environment (IDE/Maven etc.)
4. is not trivial and not completely shitty (that would be too easy and nobody will learn anything)

Instructions

1. Give everyone printouts and 10 to 15 minutes to read the code and make notes
2. Ask whether you should start top-down or from some other point in the code (some classes are broken conceptually and it's better to fix them by talking about their responsibilities first). If in doubt start from the top.
3. For every block/method/property ask whether anyone thinks that it could be done better. The person that has the idea, has the power (keyboard) to do it.
1. If it's a small change, fix the code
2. If it's a big change that would take longer than the meeting, write a TODO
3. If someone disagrees, talk, talk, talk until consensus is found or you agree that you disagree and move on
4. Run (write if needed) tests, to make sure the code works. This is a real refactoring, you cannot break the code. Broken code is not considered better.
4. If you cannot find anything more that you could get better, use the principles from “Clean Code” (or some other book) to guide you.
5. At the end of the meeting, commit the code.

Things to remember

If you are the moderator, remember to shut up and keep quiet. You probably had a bit more time to read the code so it's natural you'll have noticed more flaws, but let the people do the exploration and discovery. When they don't have anything else to say, then it's the time for you to speak.

If you are the author, no matter what, DO NOT EXPLAIN the code. Either the code speaks for itself, or is badly written.

Enjoy.

PS: this practice has been unfortunately stopped, because I've started to go to conferences and could not moderate any more. Do not make my mistake and communicate clearly, that ANYONE can call for an Open Code Review, and that the moderator should be changed every two weeks or so. I've failed at doing that, and now I have to revive the whole thing.

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<SalesOrderExportDetail> 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<SalesOrderDto> salesOrderDtos = salesOrderWebServiceClient.getSinceLastVisitOrderedByNumber();
    ImportSummary importSummary = tryToImportEach(salesOrderDtos, dateImportStartedAt);
    orderRepository.save(importSummary);
}

private ImportSummary tryToImportEach(List<SalesOrderDto> 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.

Writer versus Constructor

What's the favorite toy from your childhood? For me, both my sisters and pretty much everyone I know that would be Lego. If you could only afford it (and in the times when I was a kid, that wasn't so easy in the post-soviet Poland), there was no greater fun, than the joy of unlimited freedom to create whatever you can only imagine. And the fact, that you had limited resources wasn't a real obstacle. It was a challenge.

Nearly all the programmers I know, fell in love with programming because of the same reason. To be able to put pure ideas to life, to create something meaningful out of nothing. Truth is, no other matter is so flexible, as the stuff we work with.

We are developers because we love to construct things that work. To give soul to the machine. To make it act on our behalf. It's like magic.

That's why it is so difficult for us to understand, that a real software craftsman is a writer, not a constructor.

Uncle Bob is talking about it at the beginning of “Clean Code”. When you are writing code, you are actually reading a lot more. How is that possible? Let's have a look at an example: you have a project going, and you take a new feature from the Scrum/Kanban wall of features. And you:

1. read the code to find a place where putting some more code will give you the result you seek
2. read the code to better understand the surroundings, to eliminate potential side effects of your changes
3. write the test and the code that does what the feature is expected to do

You can start with an acceptance test first, but that only changes the order, not the fact, that the proportions of reading to writing, are always in favor of reading. Big time.

So, if reading takes so much time, how important is to make things readable?

People are working hard to learn how to write faster, how not to use mouse in their IDE, they memorize all the keyboard shortcuts, use templates and so on, but the speed up is not significant in real work. Why? Because we are only writing code 1/10 of our time. The rest is spent on reading and thinking.

A good craftsman is a good writer, because he makes the code readable, easy to grasp, easy to understand. There is a huge shift in mentality, between a constructor and a writer. Above all, the constructor's goal is to make things work. The writer's goal, on the other hand, is to make things easy to understand.

The question is, who are we writing code for? The computer? Hell no. If that was true, we would be still writing in an assembly language (or maybe even in a machine code). The whole point of procedural, functional, object oriented languages was to make it easier for us to understand the code. And that's because we don't write the code for the computer.

We write it for other programmers.

To make the shift in mentality easier, let mi show you the differences in mindsets, between the constructor and the writer.

The Constructor

The constructor is in love with technology for the sake of technology. He likes to “technologically masturbate” himself with new toys. He thinks that simplicity is for pussies, the real thing is always complex. He never removes anything, always adds more. He doesn't care that he cannot understand how his earlier solutions work, if they work why would he want to change them? He creates monsters like EJB1/2, that can do pretty much everything, but nobody is able to handle them. The he writes software to be able to use/confgure the software he has written before, because it's already too complex for anybody (maybe except for him) to understand how to use it directly.

A bigger framework is a solution to his every problem. And even if it's a new problem, he tries to modify his beloved one-to-rule-them-all tool, to be able to solve it. He is dreaming about the day, that he will have a single, big, multipurpose, futuristic wrench that works as if by magic, and does everything, from printing log files to saving the planet.

The constructor works best alone. He gets angry at people that cannot understand his programs in a second. When he works with other programmers, he is always afraid, he doesn't like others to play with his tools. When they try to do pretty much anything, they always blow something else up. Because of unforeseen side effects, a small change can devastate days of work. And it happens a lot. It gets messy, so he hates to work with others.

The Writer

The writer reads a lot, so he wants to make his code as readable, as simple as possible. He learns stuff like DSLs, patterns, eXtreme Programming, to make things easy. Easy is his keyword. He cares less about frameworks. He usually knows a few languages from different worlds, because he believes there is no single silver bullet, and different solutions are better for different problems. His code has few comments, because it doesn't need them. His code is self-documenting. His code is expressive. His code is simple, simplicity is his main value. Brilliant things are simple.

The writer always thinks about his reader. He walks hand to hand with his readers. He guides them, he cares for them. He never leaves them in the dark. His skills are in communication: he wants his intentions to be understood.

The writer works like a sniper. His changes (whether bug fixes or new features) are minimal, but the effects are exactly as expected. He never does Shotgun Surgery. He doesn't violate DRY. He is a precise man and he works with a scalpel. His code is side-effect free, simple and readable, so he knows where and how to hit. He knows what the effects will be. He keeps accidental complexity low.

The writer is agile. He doesn't set up traps, and he doesn't fall into traps. He works hard to master basic skills. He believes that without those basic skills, no framework can save him anyway.

The writer likes to work with other programmers. He is using pair programming and code reviews to make sure that his code is easy to understand by others. He wants to have another mind watching his back, and catching him when he falls. He is supportive, he is friendly. He understands that no man is an island, and that the quality of his work will be measured by the number of WTFs shouted by other developers.

A good craftsman is a good writer.

And you know what one of the most successful writers of recent times, Stephen King, says?
That he's only a craftsman.

PS: all the pictures are from Fallout games.
PS2: I've failed at giving appropriate credits to the author of the concept of programmer as a constructor. Let me fix that right away: the source of the idea that a programmer as a writer has opposition in programmer-constructor comes from a post by Andrzej Szczodrak that you can find in here