EP19 - Advance TDD1: Refactoring assertEquals

142 views
Skip to first unread message

Nhu Nguyen

unread,
Jul 11, 2013, 5:53:03 PM7/11/13
to clean-code...@googlegroups.com
Around 21:00, Uncle Bob refactored his test from:

@Test
public void givenNull_returnsEmptyString() {
      assertEquals("", invertedName(null));
}

@Test
public void givenEmptyString_returnsEmptyString() {
      assertEquals("", invertedName(""));
}


into this:

@Test
public void givenNull_returnsEmptyString() {
        assertInverted(null, "");
}

@Test
public void givenEmptyString_returnsEmptyString() {
        assertInverted("", "");
}

public
void assertInverted(String originalName, String invertedName){
        assertEquals(invertedName, invertedName(originalName));
}

In the video, he said the assertEquals are too similar and wordy. But looking at the code at this stage, I find the before case is easier to read. I can see it in one line what the given name and the expected name is. But after refactoring, the assertInverted method is more descriptive. It's also easier to change how to call invertedName and how to compare the result. Is there any other benefit for this refactor? Is it necessary?

Uncle Bob

unread,
Jul 12, 2013, 11:00:47 AM7/12/13
to clean-code...@googlegroups.com
As you'll see in E20, the wrapping of the AssertEquals is called a "Composed Assert".  I've gotten into the habit of composing almost all my asserts because it un-clutters the tests and provides me with a function name that I can use to describe the logic of what I am asserting.  

In the case you've pointed out, composing the assert is of only minor benefit; but it's part of my discipline now, so I do it without even thinking about it.

Caio Fernando Bertoldi Paes de Andrade

unread,
Jul 12, 2013, 11:13:37 AM7/12/13
to clean-code...@googlegroups.com
Nhu,

The benefit isn't really perceivable at first sight on those two degenerate tests.
If you take a look at the next ones, you will find way more readable

assertInverted("Mr. First Last", "Last, First");

than

assertEquals("Last, First", invertedName("Mr. First Last");

Cheers,
Caio

--
The only way to go fast is to go well.
---
You received this message because you are subscribed to the Google Groups "Clean Code Discussion" group.
To unsubscribe from this group and stop receiving emails from it, send an email to clean-code-discu...@googlegroups.com.
To post to this group, send email to clean-code...@googlegroups.com.
Visit this group at http://groups.google.com/group/clean-code-discussion.
 
 

Juan Manuel Gimeno Illa

unread,
Jul 12, 2013, 12:14:07 PM7/12/13
to clean-code...@googlegroups.com
Related to readability, one question I have is: why have you inverted the parameters? The xUnit convention for the assert methods is that the first parameter is the expected result and the second is the computed one. Breaking this convention, in my opinion, diminishes the readability of the composed assertion.

Juan Manuel

Caio Fernando Bertoldi Paes de Andrade

unread,
Jul 12, 2013, 12:35:37 PM7/12/13
to clean-code...@googlegroups.com
I personally think that (actual, expected) makes more sense, sounds more like speech:
I want to assert that this thing here is equals to that one there. This thing here being the test subject, and that one there being what I expect.

But I don't see any problems in refactoring the assertInverse to change the order of parameters, if you think it's clearer to follow the convention.

Cheers,
Caio

Jon Reid

unread,
Jul 12, 2013, 1:57:49 PM7/12/13
to clean-code...@googlegroups.com
The ambiguity of argument order in assertEquals is one reason to prefer Hamcrest marchers (which are included in JUnit). The Hamcrest syntax is

    assertThat(actual, predicate)

where the predicate is a "matcher" such as equalTo(expected).

The bigger reason to use Hamcrest is when your assertions move beyond simple equality to dealing with collections. Advanced matchers eliminate most loops & branches when iterating or searching. And you can write your own matchers.

Check hamcrest.org for currently supported languages.

- Jon Reid

Emil Sit

unread,
Jul 13, 2013, 11:57:19 PM7/13/13
to clean-code...@googlegroups.com
On Friday, July 12, 2013 1:57:49 PM UTC-4, Jon Reid wrote:
The ambiguity of argument order in assertEquals is one reason to prefer Hamcrest marchers (which are included in JUnit). The Hamcrest syntax is

    assertThat(actual, predicate)

where the predicate is a "matcher" such as equalTo(expected).

There's also FEST Asserts, which moves the predicate out so that you write things like assertThat(actual).isNotNull(). I find that even more readable than Hamcrest.


Reply all
Reply to author
Forward
0 new messages