Category Archives: article - Page 2

XP with Kanban instead of Scrum

Posted by on 2011/02/05 1 comment

Abstract

I’m going to step into a minefield, because in this post, I’m going to share my subjective experiences with Scrum, and I’m going to share the reasons why I moved from Scrum to XP + Kanban. I’m using the minefield metaphor, because every single sentence I’m going to share can be exploded with good explanations, mostly from advanced scrum practitioners.

Again, this post is going to be very subjective, and is based on my experiences from the last two years. So please keep this in mind, and be gentle when you share a comment.

The Background

Every organization is different. Ours is a strange one: we inherited Scrum, while the organizations we were working with were still doing waterfall. Scrum has a very different terminology, and since nobody else is doing it, they do not understand us. We had a lot of problems from misinterpretations. Story points are a good example.

For example, we told the project management that we are going to deliver 400 story points. They asked in reply how much that was in man hours? We answered that in Scrum we don’t count man hours, we are working with story points. They insisted on the man hours, and since they pay our bills, we gave in and made an estimation in man hours. Almost the same happened with sprints, because the higher-level organizations had release cycles of six months, while we had sprints of two weeks. This means that we deliver a lot of features in one giant step, and lose all the benefits of continuous delivery, like faster feedback on our product. And the list goes on.

After a while, I had the feeling that we are doing several things unnecessarily, and our management does not understand our output, and they don’t care what we are doing internally. Some of us made conversions between our Scrum measures and the company-required measures, which turned out to be a disaster. No one can make a good conversion in this situation. We started to fail, but not at the beginning, but at the worst place ever: around global delivery dates. Additionally, we spent two years trying to change our environment, and the way how people think in this environment. We failed at it, of course. The environment is huge, one small organization cannot make a difference, unfortunately.

My first thought was that we may be doing Scrum wrong. And the next one was that maybe Scrum is not the agile method that we should choose. There are lots of methodologies out there, we should try them out, and after that we’ll see whether we did Scrum wrong, or it just wasn’t for us. Henrik Kniberg has a very entertaining and thoughtful presentation on this topic. I highly recommend reading it.

So I decided that we are going to try out the Kanban methodology. The principles of Kanban would help us handle frequent changes and the diversified work item backlog. However, Kanban isn’t a software development methodology like Scrum. You cannot do Kanban, you need a process on which you apply the principles of Kanban. I usually hear the expression that “We are working in Kanban“, which is partially true. Actually they are working according to their process following Kanban principles. There is a difference. A very well known and good example is Scrum-ban. It is a Kanban system which is applied on the Scrum methodology.

We needed a process under Kanban, and I chose eXtreme Programming. I needed something without an exact requirement on iterations. XP also talks about iterations, but it is not based on them that strictly. DSDM was a candidate for 5 minutes, but it is also iteration-based. Crystal was also a candidate, but since we already have experience with XP, we know its principles – although we still have to practice –, I chose XP.

Read more »

VN:F [1.9.17_1161]
Rating: 10.0/10 (4 votes cast)

Kanban Nightmares

Posted by on 2010/11/28 6 comments

I’ve mentioned in my previous post that I have less time for coding nowadays. I’ve started to lead a new team with the intention to introduce Kanban in their way of working. In this post, I’m going to share the result of this introduction.

In these last few years I’ve had opportunities to observe coaches and consultants how they help teams and organizations. I’ve been reading about how it should be done in the proper way. In most of the cases, the person who helps the teams asks questions, points out certain areas of improvement, keeps general coding sessions, but does not get involved in the teams’ life. No offence, but I don’t see how someone can help without taking responsibility. The intention he has is loud and clear: make the teams self-aware, self-organizing and independent. But there is a fair question, though: “How can a team be self-organizing if, before agile was introduced, they had been told what to do for years? How can they make decisions?” After these years, I can state that this approach is not really effective for novice teams. It is very good for advanced teams, who might miss some improvement opportunities occasionally, but are able to react if that missing piece comes to light by a coach or external advisor.

Another common mistake and myth is that the ScrumMaster should not be involved in the team’s decision making process and daily work. In novice teams, no matter what experts say, the team members want to follow the ScrumMaster, they want to hear what he thinks and they want to follow his lead. This may sound non-agile, but if someone wants to be successful with agile methodologies, he shouldn’t ignore this fact. The ScrumMaster should coach the team like the managers at Toyota in TPS, he should give background information, and show examples on how to make decisions and how to be agile, and not just do agile. This cannot be done without the involvement of the ScrumMaster in the beginning. Teaching by example is very important. If done properly then, based on my experience, teams won’t be lost when the ScrumMaster is no longer with the team. They will continue their work, they will start being agile, and won’t depend on the ScrumMaster. There is going to be a slight setback in their work, because the team will have less new ideas due to the absence of the ScrumMaster. Fortunately, they will have learnt from the ScrumMaster how to be agile by then – and not just do agile -, so someone will start driving the changes, others will follow, and a real agile team will start forming.

Read more »

VN:F [1.9.17_1161]
Rating: 7.0/10 (1 vote cast)

jMock versus Mockito

Posted by on 2010/08/01 11 comments

I’ve been using jMock for more than a year now, but recently I came across mockito. In this post, I’ll show the difference between jMock and mockito – without judging which one is better – using the basic features I’ve been using the most often from jMock. I admit that there are more features than presented in this post, but for starters, I find these enough.

The versions under comparison:

The Test Object

For this demonstration, I’m using a very fictional, but simple code example which fits the purpose of comparison.

The skeleton of the main class which is going to be tested:

public class PosTerminal {
 
	private static final String SHOP_ACCOUNT_NUMBER = "00000000-00000000";
	private final ReceiptPrinter receiptPrinter;
	private final BankConnection bankConnection;
 
	public PosTerminal(BankConnection bankConnection,
                           ReceiptPrinter receiptPrinter) {
		this.bankConnection = bankConnection;
		this.receiptPrinter = receiptPrinter;
	}
 
	public boolean buyWithCard(int amountOfMoney, String cardNumber) {
	}
}

The skeleton of the first injected class:

public class BankConnection {
 
	public Account getAccountByCardNumber(String cardNumber) {
		return null;
	}
 
	public Account getAccountByAccountNumber(String accountNumber) {
		return null;
	}
}

The skeleton of the second injected class:

public class ReceiptPrinter {
	public void print(String string) {
	}
}

And the interface for accounts:

public interface Account {
        boolean withdraw(int amountOfMoney, String destinationAccountNumber);
        boolean enter(int amountOfMoney, String sourceAccountNumber);
        void rollback();
        void commit();
}

Creating Mocks

jMock

import org.jmock.Mockery;
import org.junit.Test;
 
public class PosTerminalTest {
    @Test
    public void shouldPrintSuccessfulWithdrawal() {
        Mockery context = new Mockery();
 
        BankConnection bankConnection = context.mock(BankConnection.class);
        ReceiptPrinter receiptPrinter = context.mock(ReceiptPrinter.class);
        // the test case continues...
    }
}

mockito

import static org.mockito.Mockito.mock;
import org.junit.Test;
 
public class PosTerminalTest {
    @Test
    public void shouldPrintSuccessfulWithdrawal() {
        BankConnection bankConnection = mock(BankConnection.class);
        ReceiptPrinter receiptPrinter = mock(ReceiptPrinter.class);
        // the test case continues...
   }
}

There is no significant difference here. jMock uses a context for handling mocks, while mockito solves it with statically imported methods.

Mocking Classes

If you compile and execute the examples above, the following exception is raised when the jMock test case is executed:

java.lang.IllegalArgumentException: com.zsoltfabok.dojo.mocks.BankConnection is not an interface

jMock is designed to mock interfaces, not classes. It forces the developer to use interfaces, which is a good programming practice, but sometimes it is an overhead. Imagine that you are working on a legacy code. There is no way to extract interfaces for every single class just to satisfy a mocking framework. Fortunately, you can set jMock so that it can mock classes:

        // the same as before ...
        Mockery context = new Mockery() {{
            setImposteriser(ClassImposteriser.INSTANCE);
        }};
        // the same as before ...

There is no need for such setting in the mockito based test case.

Two Of The Same Kind of Mocks

During testing the PosTerminal class, we need to fetch two Accounts from the BankConnection. One for the card owner (customer) and one for the shop. They will be mocks, too:

jMock

        // the same as before ...
        Account customerAccount = context.mock(Account.class);
        Account shopAccount = context.mock(Account.class);
        // the same as before ...

mockito

        // the same as before ...
        Account customerAccount = mock(Account.class);
        Account shopAccount = mock(Account.class);
        // the same as before ...

No difference here at first sight. After executing the jMock test case, the following exception is raised:

java.lang.IllegalArgumentException: a mock with name account already exists

Internally, jMock uses name references for handling mocks, and if no name is specified, the name will be the type of the mock. In the example above, we have two Accounts with the same name. The problem can be solved easily:

        // the same as before ...
        Account customerAccount = context.mock(Account.class, "customer account");
        Account shopAccount = context.mock(Account.class, "shop account");
        // the same as before ...

Annotation

Based on the mockito and jMock documentation it is possible to setup mocks with annotations, which is excellent, because with this approach, I can make the test case much simpler.

jMock

Although the current stable jMock documentation describes the @Mock annotation, I was unable to find it in the framework.

mockito

import org.mockito.Mock;
// Other import statements ...
 
@RunWith(MockitoJUnitRunner.class)
public class PosTerminalTest {
    @Mock
    private BankConnection bankConnection;
    @Mock
    private ReceiptPrinter receiptPrinter;
    @Mock
    private Account customerAccount;
    @Mock
    private Account shopAccount;
 
    @Test
    public void shouldPrintSuccessfulWithdrawal() {
    }
}

Note that in order to make it work, you need a @RunWith annotation.

Call Verification and Return Value Handling

This is the good part; how our class communicates with the injected classes. The verification is quite simple: check whether or not a method of the mock is called. The other thing is how our class can handle the return value of a method of a mock.

jMock

    @Test
    public void shouldPrintSuccessfulWithdrawal() {
        PosTerminal posTerminal = new PosTerminal(bankConnection, receiptPrinter);
 
        context.checking(new Expectations() {{
            oneOf(bankConnection).getAccountByCardNumber("1000-0000-0001-0003");
            will(returnValue(customerAccount));
 
            // Other oneOf() will() statements ...
 
            oneOf(receiptPrinter).print("Successful withdrawal");
        }});
 
        posTerminal.buyWithCard(100, "1000-0000-0001-0003");
    }

mockito

    @Test
    public void shouldPrintSuccessfulWithdrawal() {
        PosTerminal posTerminal = new PosTerminal(bankConnection, receiptPrinter);
 
        when(bankConnection.getAccountByCardNumber("1000-0000-0001-0003"))
            .thenReturn(customerAccount);
        // Other when(...).thenReturn(...) calls ...
 
        posTerminal.buyWithCard(100, "1000-0000-0001-0003");
 
        verify(receiptPrinter).print("Successful withdrawal");
    }

The order in which the methods are called is important. In case of jMock, the Expectations shall come before the method call, and in case of mockito the verify() always afterwards. There is no significant difference between jMock and mockito in this case, besides readability. The verify() method call clearly states that the we are just checking if a method is called. This clear difference makes the code more readable for me. Another readability thing is that for me, the inner class Expectations is less readable. If the mocks are not fields, then, because of the inner class, I have to make the mocks final.

Argument Matchers

Sometimes one or more arguments are not relevant when a method of a mock object is called.

jMock

        context.checking(new Expectations() {{
            // Other oneOf() will() statements ...
 
            oneOf(customerAccount).withdraw(with(any(Integer.class)),
                    with(any(String.class)));
            will(returnValue(true));
 
            oneOf(shopAccount).enter(with(100), with(any(String.class)));
            will(returnValue(true));
 
           // Other oneOf() will() statements ...
        }});

mockito

import static org.mockito.Matchers.anyInt;
import static org.mockito.Matchers.anyString;
import static org.mockito.Matchers.eq;
// Other parts of the code ...
    @Test
    public void shouldPrintSuccessfulWithdrawal() {
       // Other when(...).thenReturn(...) calls ...
 
        when(customerAccount.withdraw(anyInt(), anyString())).thenReturn(true);
        when(shopAccount.enter(eq(100), anyString())).thenReturn(true);
 
        // Other when(...).thenReturn(...)  and verify(...) calls ...
    }

The are no differences here, but I find the mockito version more readable. One additional thing is that neither of the frameworks support mixing the matchers with exact matches, for example:

jMock

Mind the missing with() call:

        context.checking(new Expectations() {{
            // Other oneOf() will() statements ...
 
            oneOf(shopAccount).enter(100, with(any(String.class)));
            will(returnValue(true));
 
           // Other oneOf() will() statements ...
        }});

The following exception will be raised during execution:

java.lang.IllegalArgumentException: not all parameters were given explicit matchers: either all parameters must be specified by matchers or all must be specified by values, you cannot mix matchers and values

mockito

Mind the missing eq() call:

       // Other when(...).thenReturn(...) calls ...
 
        when(shopAccount.enter(100, anyString())).thenReturn(true);
 
        // Other when(...).thenReturn(...)  and verify(...) calls ...

The following exception will be raised during execution:

org.mockito.exceptions.misusing.InvalidUseOfMatchersException:
Invalid use of argument matchers!
2 matchers expected, 1 recorded.
This exception may occur if matchers are combined with raw values:
//incorrect:
someMethod(anyObject(), “raw String”);
When using matchers, all arguments have to be provided by matchers.
For example:
//correct:
someMethod(anyObject(), eq(“String by matcher”));
org.mockito.exceptions.misusing.InvalidUseOfMatchersException: Invalid use of argument matchers!2 matchers expected, 1 recorded.This exception may occur if matchers are combined with raw values:    //incorrect:    someMethod(anyObject(), “raw String”);When using matchers, all arguments have to be provided by matchers.For example:    //correct:    someMethod(anyObject(), eq(“String by matcher”));

The output of mockito provides a bit more information.

Number of Invocations

Sometimes the number of invocations of a certain method is important. The approaches of jMock and mockito are quite the same, except that the number of invocations is used with the verify() method in mockito.

jMock

        context.checking(new Expectations() {{
           // Other oneOf() will() statements ...
 
            exactly(1).of(receiptPrinter).print(with(any(String.class)));
        }});

or

        context.checking(new Expectations() {{
           // Other oneOf() will() statements ...
 
            atLeast(1).of(receiptPrinter).print(with(any(String.class)));
        }});

or

        context.checking(new Expectations() {{
           // Other oneOf() will() statements ...
 
            never(receiptPrinter).print(with(any(String.class)));
        }});

mockito

       // Other when(...).thenReturn(...) calls ...
 
        verify(receiptPrinter, times(1)).print(anyString());
 
        // Other verify() calls ...

or

       // Other when(...).thenReturn(...) calls ...
 
        verify(receiptPrinter, atLeast(1)).print(anyString());
 
        // Other verify() calls ...

or

       // Other when(...).thenReturn(...) calls ...
 
        verify(receiptPrinter, never()).print(anyString());
 
        // Other verify() calls ...

There are plenty of other possibilities. jMock has allowing() and ignoring() options, which are not available in mockito. On the other hand, mockito has an only() method which makes the code more readable. In jMock, if an unexpected method call is made, a RuntimeException is thrown. In mockito this feature is not available. There is an issue for it with number 162.

Consecutive Calls

jMock

        context.checking(new Expectations() {{
            // Other oneOf() will() statements ...
 
            oneOf(shopAccount).enter(with(100), with(any(String.class)));
            will(onConsecutiveCalls(returnValue(true), returnValue(false),
                 returnValue(false)));
        }});

mockito

        // Other when(...).thenReturn(...) calls ...
        when(shopAccount.enter(eq(100), anyString())).thenReturn(true, false, false);

No difference, but I find mockito more readable.

Verify Call Order

jMock

    @Test
    public void shouldPrintSuccessfulWithdrawal() {
        PosTerminal posTerminal = new PosTerminal(bankConnection, receiptPrinter);
 
        final Sequence sequence = context.sequence("account order");
        context.checking(new Expectations() {{
            // Other oneOf() will() statements ...
 
            oneOf(customerAccount).withdraw(with(any(Integer.class)),
                    with(any(String.class)));
            will(returnValue(true));
            inSequence(sequence);
 
            atLeast(1).of(shopAccount).enter(with(100), with(any(String.class)));
            will(returnValue(true));
            inSequence(sequence);
        }});
 
        posTerminal.buyWithCard(100, "1000-0000-0001-0003");
    }

mockito

    @Test
    public void shouldPrintSuccessfulWithdrawal() {
        PosTerminal posTerminal = new PosTerminal(bankConnection, receiptPrinter);
 
        posTerminal.buyWithCard(100, "1000-0000-0001-0003");
 
        InOrder inOrder = Mockito.inOrder(customerAccount, shopAccount);
        inOrder.verify(customerAccount).withdraw(anyInt(), anyString());
        inOrder.verify(shopAccount).enter(anyInt(), anyString());
 
        verify(receiptPrinter).print(anyString());
    }

One significant difference here: in case of mockito the involved mocks need to be injected when creating the InOrder (Sequence equivalent) object.

Exception Handling

jMock

        context.checking(new Expectations() {{
            oneOf(bankConnection).getAccountByCardNumber("1000-0000-0001-0003");
            will(throwException(new RuntimeException()));
         }});

mockito

        when(bankConnection.getAccountByCardNumber("1000-0000-0001-0003"))
        	.thenThrow(new RuntimeException());

No difference, other than readability.

Setup

In order to make the examples compile and run I had to add the following external libraries.

jMock

  • jmock-2.5.1.jar
  • jmock-junit4-2.5.1.jar
  • hamcrest-core-1.1.jar
  • hamcrest-library-1.1.jar
  • jmock-legacy-2.5.1.jar
  • cglib-2.1_3-src.jar
  • cglib-nodep-2.1_3.jar
  • objenesis-1.0.jar

mockito

  • mockito-all-1.8.5.jar

Some of the jMock libraries were added after I tried to run my test cases and it took me quite some time to find out the right build path. With mockito, I just had to add one simple jar file.

Conclusion

jMock and mockito are both very well documented with examples and good practical hints. However, based on the examples above I find the test cases more readable with mockito. In my next project I’m definitely going to work with mockito. One good advice: if you make the same decision, do not mix jMock and mockito in your test code. It will make your code unreadable, and additionally it might not even work: it would be hard to execute a mockito mock related code in a jMock TestRunner (mind the @RunWith annotations).

VN:F [1.9.17_1161]
Rating: 10.0/10 (4 votes cast)

File Lottery Kata

Posted by on 2010/07/19 No comments

When I do some coding, I use test driven approach, have lots of fake and mock objects, and I use dependency injection for putting everything together. I usually don’t work alone, and from time to time I have to introduce these methods to someone with less experience in this particular area. Unfortunately, it takes me a lot of time to find the right example for them, because, frankly, the existing solutions are not that lightweight: spring context with configuration, annotations, different mock libraries, or even fake objects where mocking is impossible.

So I came up with a simple code kata exercise, which allows one to

  • practice the usage of dependency injection
  • try out different dependency injection frameworks
  • practice faking and mocking
  • try out different mocking libraries

The kata is quite simple (15 minutes):

Implement an application which follows the iterator design pattern. This particular application receives a directory as argument, and at each consecutive call, it returns a file name from the directory that hasn’t been returned before, in a random order. When the application reaches the end of the content of the directory, it shall start over, again returning the directory contents in a random order. If the directory is empty, then every call shall return an empty string. If the argument is not a directory but a file, then its name shall be returned on each consecutive call.

The random order makes testing the application quite complicated. In order to do proper testing – have an expected random number in each test case – the random generator shall be injected and mocked (or faked). If you find the right way to do it, then this exercise is pretty straightforward.

A very simple solution with fake test objects is available on my github page.

VN:F [1.9.17_1161]
Rating: 0.0/10 (0 votes cast)

Basics of Code Review

Posted by on 2010/05/13 3 comments

I’m going to keep a workshop about code review for my colleagues so I’ve collected every idea, practice, suggestion, recommendation into this post.

Any comments and suggestions are welcome.

Abstract

In this article, I would like to talk about some code review techniques which I consider useful when changes or bug fixes have to be reviewed in legacy code. I have no intention to quote the existing articles, documents, books, because they are good as they are in their current state. I’m focusing more on the practical parts which help deliver quality product quite fast.

By legacy code, I mean the code base, which has not been developed in agile way, and about which the reviewer does not know every detail. This situation is quite common and requires a different approach than reviewing a code base which currently under development.

Before a Code Review

Preparation

After a developer finishes a task, a user story or a bug fix, they should ask somebody for a code review. Developers usually do not like doing code reviews. There are a lot of pros and cons, but let’s assume that the developer would like to deliver quality product and understands the benefits of code review:

  • faster feedback on the work done
  • knowledge distribution (the reviewer will learn about the change and may see new techniques or tricks)
  • finding introduced bugs faster and cheaper

In companies where agile has just been introduced, management tends to say that code review is a waste of time, because if you are doing pair programming, you review the code while you are writing it. This is a common mistake, because pair programming does not replace code review. During implementation, two people know about the change. If they ask for a review, a third person will also know about the change. This is one way of knowledge distribution. On the other hand, if two developers introduce a change, and a senior developer says that the change is fine, but there are other ways to do it, ways which the developers were unaware of, they can learn new things. This is another way of knowledge distribution. This view is usually ignored, but it is very important.

Even with knowledge distribution there is no guarantee that the pair finds and implements the right solution. A code review after pair work can give very useful feedback on the performance of the pair. They’ll know what they did was right under the given circumstances. Also, the team introduced another useful feedback loop (first feedback loop during pair programming, second during review, third during continuous integration etc.)

One can argue what will happen if two senior developers make a change together. In this case, the reviewer can learn from their work and bring another perspective into the game. Imagine a case where the seniors have been working on the project for three years. They know every single detail about it. This knowledge is very valuable. On the other hand, they might be unable to think outside the box. A less experienced, but fresh mind can show them something that may make the change more robust, less resource consuming or something even better.

This is not a must, but a recommendation. If the team members are very experienced and they decide not to do code review after pair work, I can understand that. Nevertheless, I recommend doing reviews when somebody is doing solo work. Joshua Kerievsky said once that most of the problems in their code was introduced when somebody did solo work. 

For a code review, the developer needs one or more reviewers – I prefer more. The characteristics of a good reviewer candidate:

He or she

  • did not contribute to the change
  • has a lot of business knowledge
  • knows the programming language and the code
  • has some testing skills

I agree that people who have these skills are very hard to find, still it is crucial to pick the person most skilled in the fields described above for review. The first three options are obvious, therefore check the fourth. The code written has to be testable. This does not mean only unit testability, but also acceptance testability. Finding a person with such testing skills is even harder. Maybe it’s worth talking to a tester and tell them what the change is about, so that they can give early feedback.

Do not write review request to mailing lists. A social psychology study proves that if a larger group is asked for help, one will have less chance of getting help than asking a small group or an individual. Find the person who fits best, and ask him or her directly. For example, if you have changed code that works with databases, ask the database expert of the team for review. In this particular example, it is very important to check certain aspects like data loss, performance, database size. It is perfectly fine that everyone dares change database related code base, but like with security related matters it’s worth having the expert review the change to avoid unnecessary error reports.

The First Important Step

As a first step, it’s worth checking the following list:

  • the continuous integration is green – if not available: the code compiles
  • the code follows the agreed code conventions
  • there are comments in the version control system describing the change
  • unit tests are available

If any of the items are missing, I recommend not to continue the code review, because every hour a developer spends away from development is considered as waste. A good review takes time – sometimes it takes minutes, but it can take also days -, but there is no need to increase this time unnecessarily, if it can be avoided by following some simple rules. If not everybody is aware of the rules, I suggest having a working agreement and making these rules clear for everybody.

Reasons for not continuing without all of the requirements above met:

  • The fact that the code does not compile suggests that the developer did not finish his or her work, and the dependencies have not been check for some reason. After performing this check, the code may change, so it will have to be reviewed again, which in the end will render the time spent on the first review wasted.
  • Following the code conventions is one of the XP techniques worth applying, because it makes comparison much easier. Imagine the situation when you have a piece of code which follows the conventions, but somebody commits some refactoring without the conventions applied. You can spend a lot of time separating the logical differences from the formatting differences. People tend to be stuck checking the formatting differences. That is not really necessary, because tools can do it for them. Therefore it is in everyone’s best interest to review only the logical parts.
  • It is hard to understand legacy code. If somebody changes it, and does not make a note about the contents of the change, the original code becomes more incomprehensible. Not every version control system allows changing the commit messages, but if yours does support it, ask the developer to add the comment.
  • Checking in code without testing is very dangerous and not recommended. If someone checks in a piece of code without test cases, that can suggest that the change wasn’t verified. After testing, the author may change code, so the effort spent on review will become a waste again.

During the Code Review

Code Review Methods

There several ways to do code review, but for now, consider only these two:

  • the author explains the code to the reviewer and they check the code together – the feedback is immediate
  • the author checks the code alone, and gives feedback after the review

I recommend doing the code review alone. Similar to testing, the author/developer can force the reviewer/tester in one direction so that they miss an important detail. Of course, if the reviewer has questions, he or she shall ask the author, but do not let the author take control over the review session.

There are plenty of ways to find out what has changed and where. It is a common way of working to take the diff/compare tool of the version control system and review the code there. I suggest not doing that. It is a very good thing to have the list of the affected files and the changed areas, but this will narrow down the view of the reviewer to the lines affected by the change. If they do the review with a compare tool, they can check that the change is good, but will have a hard time finding out what is missing; for example an important dependency injection or an exception handler.

If the checklist is complete, the reviewer shall try to understand what the change is about. If it is a change, check the product backlog, if it is a bug fix, check the problem description. Without knowing what the change is about it is very hard to do quality work. Hence, it is better to start the review with the review of the unit test cases. They can tell you about the nature of the change, and you can decide whether the change is correct or not. So always start with the test cases.

If you find something that is not included in the checklist above not stop the review just then, even if it is a major problem. I’ll talk about this later, but the code review has to be performed until the quality of the code satisfies the reviewer and the team. If the code contains 2 major and 4 minor findings, and the reviewer highlights only the first major one and stops the review, an important quality feedback is missed.

Separation of Changes

Usually, a change in the code affects only a few lines. However, agile teams tend to do a lot of refactoring, which is a good thing, but it is a nightmare when it comes to code review. A particular change consists of two things:

  • refactored parts
  • the changed parts

If you face some code where the refactored  and the changed parts were checked in together, you have no other option but to talk to the author, so that he or she can help you separate them. If you are lucky, the change affects one file, and the refactoring another twenty. It is not cheating to point out where the change is, so that the reviewer does not waste a lot of time reviewing refactoring that has been done automatically by the editor tool. Therefore, you can check the refactored part later, or not check it at all and let the regression test suite verify it.

Nevertheless, if this is the case, you can have one comment for the author: do not commit refactoring together with the changes. Try to do them separately. It kind of fits into the idea that says write test cases first; refactor the code, so that the change is obvious; do the change and refactor again. Following the commit recommendation above, this means three individual commits: one after the first refactoring, one after the change, and one at the end. With this approach, the reviewer can prioritize his or her review process: change must be checked every time, and if there is little time left, the reviewer can decide whether to check the first refactoring. I would check it as well every time, because this is the change which has been made on the legacy code, and it is very important to make sure that the system continues to work as before. The last refactoring can be verified using the regression test suite. If there aren’t any, you have to review that part as well.

Categorization

Large companies have templates even for code review. Usually these templates contain a category for review findings. They are used for deciding whether to perform the suggestions of the reviewer. Having categories is fine, but treat the review findings as user stories instead. Show the findings to the author and the team, and decide together how to continue. It makes no sense to implement only the major findings or a number of small items. Every code review shall be ended with a discussion between the author and the reviewer(s) – and, optionally, the team. If a finding makes no sense, or ignoring them does not harm the current code base, you can skip it.

Practices

References

A common mistake I observed is that people are using the version control system’s diff tool or an external tool called by the version control system for code review. They are focusing on the changed part only, which saves some time for them, but can cost a lot later. Have a look at the example below:

original:

class A {
  boolean flag;
  public A() {
    flag = true;
  }
}

the change:

  boolean flag = true;

If you check only in the class scope, it is a fine operation. On the other hand, another class has a reference:

class B {
  private A a;
  public void foo() {
    if (a.flag) {
      // something
    } else {
      // something else
}

Using just a compare tool you will miss this impact. The recommendation here is to check the changes in the IDE you are using and check the references as well. If you see a field without the private modifier, be curious and check the references. If you are lucky, the regression test suite will find the problem later, but usually legacy systems do not have a lot of regression test cases.

Look for common mistakes

Have a look at Jeff Atwood’s Top 25 list. Look for these problems in the changed code.

Synchronization

If you see a change which affects code protected by the synchronized keyword, look for the wait, notify and notifyAll calls in order to make sure that the change does not introduce a [dead]  lock situation.

Abuse of design patterns

Design patterns are good, but people sometimes abuse them. Use the right pattern for the right task. Do not solve everything with the factory pattern or with the command pattern. If the change seems to be an introduction of a design pattern, double check the pattern, and make sure that it is implemented properly.

Look for duplication

There is no way that every team member has a deep knowledge of the legacy code base. Therefore it may happen that somebody introduces a utility method or class that already existed. These utility classes have very unique characteristics: no operation on fields, descriptive and general name, used in different places in the changed code. Do a search on the name, or the used method, and there is a big chance that the utility method or class already exists somewhere in the legacy code base.

Dependencies

If a change affects the constructor, it may indicate a dependency change. It is a good place to start the review.

Length and complexity of test cases

As I said before, I like checking test cases first, because they tell a lot about the code. Large and complex test cases indicate large and complex code. If you meet such test cases, it’s worth talking to the author about the change before continuing the review. You may be about to spend a lot of time on a code which may have to be refactored or, in the worst case, is not even be ready. So talk to the author, pair up, solve the problem.

Wasting resources (memory, CPU, disk)

I strongly believe that a lot of resource wasting operations can be found by code review. If the changed code operates on streams or javax.sql classes, check that every stream or connection is handled and closed properly. Look for interesting loops and data structure operations.

Conditional expressions

If a conditional expression has been changed, the first thing I usually do is check the coverage of the branches. It is not part of the standard code review practices, but it helps me find out what is going on, and the possible effects of the change. The next thing is that I check every possible outcome of the expressions. It is important to check for things which may be missing. The test cases can help in this case.

Consistency Check

Consistency is very important. Let’s have an example; the class solves every possible loop with while. A change requires another loop, and the fixer decides to use a for loop because he feels more comfortable with it. Even if the code works, after a certain amount time, nobody will remember why we have a lot of while loops and only one single for loop. Although it is a small thing, people will spend some time figuring this out. Eventually, they will end up at a particular commit, which does not make too much sense in their current point of view, and it will be hard to refactor the code, because the for has to be changed into a while. This is also a waste. The recommendation here is to be consistent with the class/package even if the author thinks differently. Do the fix, and refactor the class in two different commits.

Spent time

Limit your time spent on review. It is easier said than done, but it is easy to get stuck in a code review. Limit yourself so that you’ll check each and every logical change (see prioritization above), and maybe just a couple of the refactoring or minor changes.

Feedback

Follow the life of the reviewed code, so that you can have a feedback on your review. If the acceptance fails on a trivial error, it’s worth sitting down and checking the review way of working.

After the Review

List of Findings

The result of the code review is a list which contains the findings. It is a good practice to give hints in case of tricky problems. I mentioned above that sending code review invitation to mailing lists is not recommended, but sending the list of findings to the entire team is a good idea. With this approach, everyone can have a look and decide which findings will be implemented – like a small product backlog.

The Next Review

Never have “accepted after change” as an outcome of the review. Nobody guarantees that the findings will be implemented as requested. Also, usually, a major finding means that additional changes will be done besides the existing ones. So I recommend having the review sessions until the current state of the code satisfies the reviewer and the team. If the changes are minor and the team is fine with them, the second review can be a peer review, but try to keep the basic rule: the first review should be done alone.

Correction

If there are a lot of changes, pair up with the author and do the fixes together. In this case, you need a new reviewer afterwards, but this way you can share the new code base with someone else as well.

Intermediate Review

It is a good practice to keep intermediate reviews if the changes seems to be big, or the author developer has the habit of checking in a large set of files. During these reviews, the first checklist should still be applied, because the earlier you find the problems, the cheaper they will be to fix.

VN:F [1.9.17_1161]
Rating: 10.0/10 (1 vote cast)