Refactor, don't reinvent the wheel

Recently I saw a training regarding clean code and refactoring. One of shown examples of bad code was something like this:

public void setName(String name) {
    this.name = name;
    if (this.name != null ) {
        if (this.name.length() > 30) {
            this.name = name.substring(0,30);
            this.name = this.name.toUpperCase();
        } else {
            this.name = this.name.toUpperCase();
        }
    }
}

And after a few slides the code was refactored to the final version:

public void setName(String name) {
    if (!isValid(name)) {
        this.name = null;
    }

    this.name = limit(name, to(30)).toUpperCase();
}

private boolean isValid(String name) {
    return name != null;
}

private String limit(String input, int limit) {
    return input.substring(0, limit);
}

private int to(int x) {
    return x;
}

And I will argue that's a very wrong approach or a really bad example. Why? Because every language has its idioms and most commonly used tools that became de-facto standards and are well known and understood. In java world it's guava, apache commons, lambdaj etc. Using those libraries, you can be much more functional, null-safe and concise. You can use well known existing functions instead of creating new ones and learn them again in each project. In my opinion, much more readable would be:

public void setName(@Nullable String name) {
    this.name = StringUtils.upperCase( StringUtils.left(name, 30));
}

or in case we'll need it more than once:

public static upperCasePrefix(@Nullable String input, int limit) {
    return = StringUtils.upperCase( StringUtils.left(input, limit));
}

public void setName(@Nullable String name) {
    this.name = upperCasePrefix(name, 30);
}

Use unicode for better names

Let's say in a java application we have a few tabs and sometimes we hide some of them. So now we want to document a new requirement and of course we do it as a test:

@Test
public void should_hide_more_tab_when_no_additional_information_is_available() {
   ...
}

but wait... what does it exactly mean? Should our application hide more tabs then it usually does? Or is there a tab named 'more' that should be hidden? How can we clarify this? After a quick look at the unicode char table, we pick the ʻ char (or any other that makes you happy). It's a '02BB turned comma' and more information can be found for example here. There is a table with detailed information about that character and the interesting part is:

Character.isJavaIdentifierPart()  Yes

Cool! So let's write:

@Test
public void should_hide_ʻmoreʻ_tab_when_no_additional_information_is_available() {
   ...
}

Is this test more readable now?

ps.
For example in racket (a dialect of lisp) you can define lambdas using λ:

(λ(x) (+ x 1))

The myth of random data in unit tests

Many times I see people generate random data for any irrelevant variable in tests:

String anyName = RandomStringUtils.random(8);

Customer customer = customerBuilder()
                               .withName(anyName)
                               .withAge(18)
                               .build();

assertThat(customer).isAdult();

First of all, it would probably be better if this test looked somehow like this:

Customer customer = newCustomerWithAge(18)

assertThat(customer).isAdult();

I know, I know: sometimes tests are a bit more complex and badly written and you just need the name as a constant. So why not simply:

private static final String ANY_NAME = "John";
...
customer = customerBuilder()
                      .withName(ANY_NAME)
                      .withAge(18);
                      .build();

assertThat(customer).isAdult();

Does the random generator make you feel safer? If the name is irrelevant, why bother generating it? It just makes your code less readable.

But some people go even further. Let's say we want to test StringUtils.contains from the Apache Commons. Some people want to generate the significant parameters:

String random1 = randomString();
String random2 = randomString();
...
assertTrue(StringUtils.contains(random1 + random2 + random3, random2));

Easy, right? But how will we test if it returns false correctly? Now our random data needs to obey some specific constraints. So it's rather hard to generate the data without, in fact, implementing the functionality again in tests. Another problem is that when you have such tests you think everything is tested and you stop thinking about corner cases.

But is everything really tested? What about nulls? what about empty strings? What about combinations of them? And even if your generator can produce nulls and empty strings, still: is everything tested?

How often will your random test run before the tested code goes on production? If you do continuous delivery then the test will run a few times during your local development, once on your CI server and... that's it. If you're not so lucky to do continuous delivery then let's assume your commit goes on production in 3 weeks. Probably soon there will be feature freeze and branch stabilization. How many times will this test run? 50 times on CI server? Random tests are totally useless when running only a few times. Of course you may expect those tests will run very many times during local development of the rest of your team but...

If it fails on someone's else machine, are you sure he will record the test result? Wait! There will be no result! There will be only information that true was expected but false was returned. So you have to remember about adding logs to all your random tests. And even if logs are being dumped, are you sure that other developer (who has to deliver his own, completely different functionality) take care about irrelevant, non-deterministic test failure? Because other option is simply re-run tests, see the green light, commit and go home. No one will ever know.

Let's face it, it can't work this way. If you are not sure if your test data is good enough then:

• Simplify your code. Extract methods/classes, avoid ifs, avoid nulls, be more immutable and functional.
• Try to analyze the edge cases and include them in your tests.
• If needed, throw away the part of code and start again doing TDD. If you've never tried it, you will be surprised how different the design can be.

Seriously, those two rules will almost always be enough. That's because the sad truth is that the vast majority of all the development is a typical corpo maintenance. It's not a rocket science and all the complexity is usually incidental. But the refactoring can be expensive. And if above rules are not enough:

• Generate a lot of random data sets, look at them and check if some of them differs from what you had in mind when designing your code. And, of course, add new cases to your tests.
• Use mutation testing.
• Whenever a bug is discovered during the development, uat or production, add new cases to your tests to avoid regression.
• Do real random testing. Keep the testing server running 24/7. Every generated data that breaks the tests should be logged and added to your deterministic unit tests.

Date type in H2 vs Oracle

I often use H2 to locally develop code that is supposed to work on Oracle. Yes, yes, I can hear all the 'Why don't you work on production-like environment?!'. Well, I do it because I'm lazy. When the development configuration has no dependency on external infrastructure then every new developer or tester can simply do: git clone ... && mvn test && mvn jetty:run and it just works. Immediately. Without bothering me with questions about the setup, configuration, passwords etc. CI server can build each new branch simultaneously. Local tests run quickly as there is no network communication. It's simply really convenient.

Of course, it has its price. Compatibility. Most applications are really simple and doesn't need any vendor-specific features. But sometimes...

create table my_table (my_column date);
insert into my_table values (timestamp '2013-01-23 13:23:34');
select my_column from my_table;

So what's the result of the select statement?

oracle: January, 23 2013 13:23:34+0000
H2:     2013-01-23

What happened with time? Let's see more details

select cast(my_column as timestamp) from my_table;

oracle: January, 23 2013 13:23:34+0000
H2:     2013-01-23 00:00:00.0

Yep, the time is silently truncated. Oracle Date stores time as opposed to H2, mysql, postgres and probably most others. And this will affect all frameworks you use: jdbc, dbunit, hibernate etc. So, if possible, use Timestamp type or design your application in a way it doesn't matter.

Tested databases (thanks to sqlfiddle.com):

• Oracle 11g R2
• H2 1.3.171
• MySql 5.1.61
• PostgreSQL 9.3.1

404 error with spring mvc testing

We have a standard rest application built with spring mvc (v3.2.3). Let's add another controller (MyController) with one method that does some computations and returns an empty response with 200 OK. Test first:

import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get;
import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.*;

import org.junit.*;
import org.springframework.test.web.servlet.MockMvc;
import org.springframework.test.web.servlet.setup.MockMvcBuilders;

public class MyControllerTest {

 private MockMvc mockMvc;

 @Test
 public void test() throws Exception {
  mockMvc.perform(get("/my/test"))

   .andExpect(status().isOk())
   .andExpect(content().string(""));
 }

 @Before
 public void setUp() throws Exception {
  mockMvc = MockMvcBuilders.standaloneSetup(new MyController()).build();
 }
}

Nothing new, everything just as in spring's tutorial. So now let's write the code:

import org.springframework.stereotype.Controller;
import org.springframework.web.bind.annotation.RequestMapping;

@Controller
public class MyController {

 @RequestMapping("/my/test")
 public void test() {
  System.out.println("test method executed"); //yes, yes, I know
 }
}

Test is green, that was easy. Let's see our code in action. Start jetty and check the address /my/test. Method is executed and we get... 404 not found. Wtf?!

Probably the quickest way to find the problem is to compare the method with other working controllers. Of course, I forgot about @ResponseBody. After adding it, jetty displays the page correctly and test still passes. But the purpose of writing tests is to have protection against such mistakes. So why the test was green?

For void methods without @ResponseBody spring forwards request processing to DispatcherServlet which, in this case, fails trying to resolve a view for the specified url. But for some reason mockMvc reports empty response and status 200. I reported it as a bug but it got status 'Works as Designed'. So how can we eliminate the false positive? We need to explicitly check if no forwarding is done. And there is an existing ResultMatcher for this:

forwardedUrl(null)

It can be added with another andExpect inside each test. But turning it on globally will save you from such mistakes in future:

@Before
public void setUp() throws Exception {
 mockMvc = MockMvcBuilders.standaloneSetup(new MyController())
                             .alwaysExpect(forwardedUrl(null))
                             .build();
}

After adding this matcher, the test without @ResponseBody fails:

java.lang.AssertionError: Forwarded URL expected:<null> but was:<my/test>

Testing time-dependent code

Often people do something like that

public List<Notification> findNotificationsToSend() {

    List<Notification> notifications = repository.findDailyNotifications();
    int today = Calendar.getInstance().get(DAY_OF_WEEK);
    if (today == SUNDAY) 
        notifications.addAll(repository.findAllWeeklyNotifications());
    return notifications;
}

and then you can be sure they don't use TDD. Because there is absolutely no way you can create such a bad code when you have tests. But what can we do about the calendar? Same as always. Have you noticed that when there is a call to a database or other external system, people immediately say: 'extract and mock'? But when there is a call to jvm's infrastructure they have no idea what to do. And the answer is simple: 'extract and mock'. Does it mean people just repeat previously seen schemes without thinking?
We can start refactoring with:

public class TimeProvider {

    public int dayOfWeek() {
        return Calendar.getInstance().get(DAY_OF_WEEK);
    }
}

That's a good start. Now it's easy to test the findNotificationsToSend but TimeProvider can still contain some complicated time calculations which are not testable. And it will grow with calendar-dependent methods. How to clean it up?

• Switch to joda time. It has much better api that protects TimeProvider from uncontrolled growing.
• TimeProvider should contain only often used, calendar-like, parameterless methods dependent on current time. And nothing else. 'isSunday' and 'beginningOfQuarter' are fine but 'shouldIncludeWeeklyNotification' is not.
• Completely separate jvm's infrastructure access from time calculations. In this case I usually choose inheritance over composition because TimeProvider won't ever grow in any additional dependencies. After all, even business guys don't change the definition of Sunday.

The following code usually works for me.

abstract class TimeProvider {

    protected abstract long currentMillis();

    public final DateTime now() {
        return new DateTime(currentMillis());
    }

    public final boolean isSunday() {...} //if often used

    // other common business methods. all final.
}

public final class RealTimeProvider extends TimeProvider {

    protected long currentMillis() {
        return System.currentTimeMillis();
    }
}

public class TestTimeProvider extends TimeProvider {

    private long currentMillis; 

    public TestTimeProvider() {
        this("2013-05-17"); // preset time; handy for tests
    }

    public TestTimeProvider(String currentTime) {
        setTime(currentTime);
    }

    public void setTime(String currentTime) {
        currentMillis = parseTime(currentTime);
    }

    protected long currentMillis() {
        return currentMillis;
    }

    private static long parseTime(String time) {...}
}

Of course, we use TestTimeProvider in unit and spring-context tests. Often it's more handy than mocks. If needed, add similar support for timezone.
Now we can test findNotificationsToSend, control time during integration tests and test isSunday method:

@RunWith(ZohhakRunner.class)
public class TimeProviderTest {

    TestTimeProvider timeProvider = new TestTimeProvider();

    @TestWith({
                 "2013-04-14,  true",
                 "2013-04-15,  false"
    })
    public void should_detect_sunday(String date, boolean shouldBeSunday) {
        timeProvider.setTime(date);

        boolean isSunday = timeProvider.isSunday();

        assertThat(isSunday).isEqualTo(shouldBeSunday);
    }
}

One place where time provider alone is not enough is integration testing, when we start the whole server to imitate production environment and connect to it over http. But that's a story for another post.

Testing cron expression

Many people, using spring scheduling, write

@Scheduled("* * 3 * * ?")
public void myCronJob() {...

and then they wait a few days to check logs if the job is triggered correctly. And what about:

0 0/5 14,18,3-39,52 ? JAN,MAR,SEP MON-FRI 2002-2010

Fortunately, testing a cron expression is simple. But first we need a constant:

public static final String EVERYDAY_3_AM = "* * 3 * * ?"

And now, with spring's scheduling, we can use org.springframework.scheduling.support.CronSequenceGenerator

import static org.fest.assertions.api.Assertions.assertThat;

import java.text.ParseException;
import java.text.SimpleDateFormat;
import java.util.Date;

import org.junit.BeforeClass;
import org.junit.runner.RunWith;
import org.springframework.scheduling.support.CronSequenceGenerator;

import com.googlecode.zohhak.api.Coercion;
import com.googlecode.zohhak.api.TestWith;
import com.googlecode.zohhak.api.runners.ZohhakRunner;

@RunWith(ZohhakRunner.class)
public class CronTest {

  static CronSequenceGenerator everyday_3am;

  @TestWith({
    "2013-06-10 22:20,    2013-06-11 03:00",
    "2013-06-13 01:12,    2013-06-13 03:00"
  })
  public void should_trigger_at_the_nearest_3_AM(Date now, Date nearest_3am) {

    // when
    Date nextExecution = everyday_3am.next(now);
  
    //then
    assertThat(nextExecution).isEqualTo(nearest_3am);
  }

  @BeforeClass
  static public void parseExpression() {
    everyday_3am = new CronSequenceGenerator(Constants.EVERYDAY_3_AM);
  }

  @Coercion
  public Date coerce(String date) throws ParseException {
    return new SimpleDateFormat("yyyy-MM-dd hh:mm").parse(date);
  }
}

We use static variable just to avoid multiple parsing of the same expression, as for complex scenarios there might be many parameters.
The same can be achieved with quartz library. To do this just replace CronSequenceGenerator with org.quartz.CronExpression

Date nextExecution = everyday_3am.getNextValidTimeAfter(now);

everyday_3am = new CronExpression(Constants.EVERYDAY_3_AM);