Legacy Code to Testable Code #5: Extract Class

This post is part of the “Legacy Code to Testable Code” series. In the series we’ll talk about making refactoring steps before writing tests for legacy code, and how they make our life easier.

A few years ago I got this from Erik Talboom: "A private method is a design smell". It took me a while to fully understand it and to apply it.

There's a nice logic in there.

If at some point, we're extracting a private method from inside a public method, it probably means the public method did too much. It was long, and procedural, and probably made a number of operations. Under those circumstances, it made sense to extract some of the logic into the private method. It made sense that we could name it more clearly, as the extracted method was simpler.

It also means that the public method broke the Single Responsibility Principle. After all, we just broke it in two (at least). If that's the case, the private method we extracted contains a separate functionality from the rest of the public method.

This functionality we extracted should probably be tested. It would be easier to test, because it's smaller. If we keep it private, but called from the public method, we'd prefer not to test it directly, but through the public interface. If however we extract it to a new class, testing both will be easier, because both are simpler.

Testing and simplicity go hand in hand, and extracting into a separate class makes sense in so many cases. Too bad it's not applied often.

Here's a simple example:
public void UpdateStreet(string newStreet) { if(string.IsNullOrEmpty(street) && !street.StartsWith(" ") && !street.StartsWith("@")) { Address address = new Address(this); address.SetStreet(newStreet); } }

It makes sense to extract the validation:

public void UpdateStreet(string newStreet) { if (ValidateStreet(newStreet)) { Address address = new Address(this); address.SetStreet(newStreet); } } private bool ValidateStreet(string street) { return string.IsNullOrEmpty(street) && !street.StartsWith(" ") && !street.StartsWith("@"); }

If we keep it like that, testing the validation is problematic. Instead, we can extract the method into separate class called StreetValidator:

public void UpdateStreet(string newStreet) { if (StreetValidator.Validate(newStreet)) { Address address = new Address(this); address.SetStreet(newStreet); } }

Now we can test the Validate method, and then the original UpdateStreet method separately.

We could also expose the method as public, or make it static since it doesn't change any state. However, this may not always be the case. Sometimes in order to perform the separation, we need to actually cut the cord.

Suppose that our validation now includes comparison to the current address' street:


public void UpdateStreet(string newStreet) { if(string.IsNullOrEmpty(street) && !street.StartsWith(" ") && !street.StartsWith("@")) && currentAddress.GetStreet().CompareTo(street) == 0) { Address address = new Address(this); address.SetStreet(newStreet); } }
        
currentAddress is a field in our class, so it's easy to extract it into a private method:

private bool ValidateStreet(string street) { return string.IsNullOrEmpty(street) && !street.StartsWith(" ") && !street.StartsWith("@") && currentAddress.GetStreet().CompareTo(street) == 0; }
       
However, extracting this into separate class, requires us to pass the currentAddress as a parameter. We can do this in two steps. First, we change the signature of the method, and add a parameter with the same name as the field:

private bool ValidateStreet(string street, Address currentAddress) { return string.IsNullOrEmpty(street) && !street.StartsWith(" ") && !street.StartsWith("@") && currentAddress.GetStreet().CompareTo(street) == 0; }
Now that we “shadowed” the field, we decoupled the method from its class. The method can now be extracted to a separate class.

I find people accept extract class (if it's safe and easy) more than exposing methods, or creating accessors. The effect is the same (and the risk that someone will call it is the same), but the simplicity makes it more bearable, I guess.

Extracting a class reduces the complexity of the code and testing. Instead of the combinatorial order code paths that need testing, we lowered the test cases into linear order. Testing is not only possible, but also more probable - we are always likely to test more when the there is less to test.

That trick we did when we added the parameter? We’ll discuss it with more details next.

“The New Agile” Slides

I had a great fun presenting at the Agile Practitioners meet-up today. Good feedback, and feels like a keynote.

Here are the slides.

Legacy Code to Testable Code #4: More Accessors!

This post is part of the “Legacy Code to Testable Code” series. In the series we’ll talk about making refactoring steps before writing tests for legacy code, and how they make our life easier. It continues the last post on accessors.

We talked about “setter” accessors as a mean to inject values. The other side of the coin is when we want to know something has happened inside our object. For example, if internal state has changed, or a non-public method was called.

In fact, this is like the "if a tree fell in the woods" question: Why do we care if internal state changed?

In legacy code, there are many cases where a class grows large and does many things. If we had separated responsibilities, the result would be possible to assert on another class. Alas, with god objects, things are a bit of a mess. When that happens, our acceptance criteria may move inside: We can either check internal state or internal method calls for its impact.

To check internal state, we can add a “getter” method. Adding a "getter" function is easy, and if it doesn't have logic (and it shouldn’t), it can expose the information without any harm done. If the refactoring tool begs you to add a "setter" you can set it to be private, so no one else uses it.

Role reversal

In a funny way, "getter" methods can reverse roles: We can use a "getter" method to inject a value by mocking it.
So in our getAccount example:
protected Bank getBank() { return new Bank(); }public void getAccount() { Bank tempBank = getBank(); ...

By mocking the getBank method we can return a mockBank (according to our tools of choice):

when(testedObject.getBank()).thenReturn(mockBank);

On the other hand, we can verify a call on a "setter" instead of exposing a value. So if our Account object has an internal state called balance, instead of exposing it and checking it after the tested operation, we can add a "setter" method, and see if it was called.

verify(account).setBalance(3);

In contrast to injection, when we probe we don't want to expose an object on the stack. It's in the middle of an operation, and therefore not interesting (and hard to examine). If there's an actual case for that, we can use the "setter" method verification option.

In this example, the addMoney function calculates the interimBalance before setting the value back to currentBalance.

public void addMoney(int amount) { int interimBalance = currentBalance; interimBalance += amount; currentBalance = interimBalance; }

If we want to check the `currentBalance` before the calculation, we can modify the method to:

public void addMoney(int amount) { int interimBalance = setInterim(currentBalance); interimBalance += amount; currentBalance = interimBalance; }protected void setInterim (int balance){ return balance; }

Then in our test we can use verification as a precondition:

verify(account).setInterim(100);

Adding accessors is a solution for a problem that was created before we thought about tests: The design is not modular enough and has many responsibilities. It holds information inside it, and tests (and future clients) cannot access it. If we wrote it "right" the first time, the god class would probably have been written as a set of classes. With our tests in place, we want to get to a modular design.

Tests give us the safety to change the code. So are the automated refactoring tools. We can start the separation even before our tests using the Extract Method refactoring pattern. 

We’re going to discuss it next.

Legacy Code to Testable Code #3: Adding Setter Accessors

This post is part of the “Legacy Code to Testable Code” series. In the series we’ll talk about making refactoring steps before writing tests for legacy code, and how they make our life easier.
Irish Setter. He's a go-getter.
Irish Setter. He's a go-getter.

Adding accessors to private state data is an admission that either our design is wrong, or that we're adding the accessors purely for testing. If that state was private before, how come are we exposing it? And if so, maybe tests are not the only clients of this data.

We need an accessor in two cases:
  • When we need to inject values that are otherwise hard to inject
  • When we need to probe the internal state to understand the impact of our operation
In both cases, we're adding functions that were not needed before. Meaning, the caller did not need to either inject or probe the data. Now our tests, another client, demand an additional entry point. In this post, I’ll concentrate on the Set accessors for injecting values.

Injecting values

If we have other methods, we might not need an accessor. For example, we have a private field that's initialized in the constructor:

public Account(Bank bank) { this.bank = bank; ...

If we pass the new bank value as a parameter, we can set it in our tests easily to whatever we need. However, if the constructor looks like this:

public Account() { this.bank = new Bank(); ...

The seam for inserting our bank is no longer there, and we need another way to inject it. In another case, the new bank can come from an external source:

public Account() { this.bank = BankFactory.getBank(); ...

In both of these cases, we can mock the creation or the static call with power tools. If we can't use those, we can introduce a "setter" method. This "setter" can be public or at least accessible to our test.

public setBank(Bank bank) { this.bank = bank; }

When we call this method, it overrides the value that the constructor initialized. This doesn't work if our constructor already did something with the original value. Usually it's not a problem, because we're testing code in another method, so whatever the constructor did, the tested method will be under our influence. In other cases, we can mock the constructor using partial-mocking for eliminating code that runs in the constructor, or use power tools to mock the factory method.

From here it gets complicated. What if our `bank` is static? Consider a private static singleton:

private static Bank theBank = new Bank():
Once initialized, it cannot be replaced through regular means. If our test calls for it, replacing it can be easier by injecting our own mockBank. So we can add a static "setter":

public static void setBank(Bank bank) { theBank = bank; }

As before adding an external "setter" is a risk: What if someone else decides to call it? We can reduce accessibility, but the risk is still there.

In both former cases, we could store the overriding mockBank. It maybe that the value is not stored anywhere, only created on the stack, in the tested method:

public void getAccount() { Bank tempBank = new Bank(); ...

Here, we cannot access tempBank, because it's on the stack. In this case, we can do a bit of refactoring in order to allow injection. We'll first extract the creation into a private method called getBank:

private Bank getBank() { return new Bank(); } public void getAccount() { Bank tempBank = getBank(); ...

Using refactoring tools, we can reuse this method for every creation. We can then introduce a field and a "setter", and modify the getAccount method:

private Bank bank = Null; public void setBank(Bank bank) { this.bank = bank; } private Bank getBank() { if (this.bank == Null) return new Bank(); else return this.bank; }

Using this method we skip mocking (and reduce coupling) at the expense of design changes. We can't escape trade-offs.

There's a final option for injecting values, if the language supports it: Reflection.  If we use reflection in the test, there is no need for modification in the tested code. However, tests using reflection are as fragile as mocking tests, sometime even more so. Reflection access is fragile, and not refactorable. Therefore, this method should be considered last in the solutions offered.

That’s it for now. Next time we’ll continue the discussion about accessors with “getters”.

Image source: http://en.wikipedia.org/wiki/Setter

Legacy Code To Testable Code #2: Extract Method

This post is part of the “Legacy Code to Testable Code” series. In the series we’ll talk about making refactoring steps before writing tests for legacy code, and how they make our life easier.

As with renaming, extracting a method helps us understand the code better. If you find it easy to name the method, it makes sense. Otherwise, you just enclosed code that does a lot of things. It can be useful sometimes, although not as extracting small methods that make sense.

Extracting a method also introduces a seam. This method can now be mocked, and can now affect the code as it being tested. One of the tricks when not using power-tools is wrapping a static method with an instance method.

In our Person class, we have the GetZipCode method:
public class Person { String street; public String getZipCode() { Directory directory = Directory.getInstance(); return directory.getZipCodeFromStreet(street); } }

The Directory.getInstance() method is static. If we extract it to a getDirectory method (in the Person class) and make this method accessible, we now can mock it.

public class Person { String street; public String getZipCode() { Directory directory = getDirectory(); return directory.getZipCodeFromStreet(street); } protected Directory getDirectory() { return Directory.getInstance(); } }

While it's now very easy to mock the getDirectory method using Mockito, it was also easy to mock the Directory.getInstance if we used PowerMockito. Is there an additional reason to introduce a new method?

If it’s just for the sake of testing - there's no need to do the extraction. Sometimes, however mocking things with power-tools is not easy. Problems appearing in static constructors may require more handling on the test side. It may be easier to wrap in a separate method.

There are times when extracting helps us regardless of the mocking tool. We can use method extraction to simplify the test, even before we've written it. It's simpler and safer to mock one method, rather than 3 calls.

If our getZipCode method looked like this:

public String getZipCode() { Address address = new Address(); address.setStreet(street); address.setCountry(country); address.setState(state); address.setCity(city); Directory directory = Directory.getInstance(address); return directory.GetZipCode(); }

Even with power-tools, faking the Address instance and setting the rest of the behavior settings just for retrieving the directory is quite a lot of work, which means a longer test with a long setup. If we extract a getDirectoryFromAddress method:

public String getZipCode() { Directory directory = getDirectoryFromAddress(); return directory.GetZipCode(); }

We get more readable code, and we'll need to mock only one line.

While extracting has its up side, making a method a seam comes with the baggage. If the method is private, and we use power tools to mock it, coupling between test and code is increased. If we make it public, someone can call it. If it's protected, a derived class can call it. Changes for testability is a change of design, for better or worse.

Up next: Creating accessors.

Image source:http://commons.wikimedia.org/wiki/File:Dentist_extracting_tooth..JPG

Upcoming Events

It’s that time again. It’s going to be busy so try to keep up.

image On Oct 21st, I’m giving a presentation called “The New Agile” at the Agile Practitioners Meetup. The talk is about what’s hot and making the waves in the agile world.
image On Nov 13th, I’ll do the “TDD Patterns” talk at Agile Testing Days. This is interesting for me, not just because going back to #AgileTD, but also because of the topic. Patterns AND TDD? This is awesome.
image On Dec 3rd, I’ll be at NDC London, talking about “Testing Economics 101”. Very exciting conference, and lots of insights that you may get a glimpse from my book “Everyday Unit Testing”.
image For starting 2015, on Jan 28th, I’ll do the “To Estimate or #NoEstimate” talk, as part of the Agile Practitioners 2015 conference. This is shaping up to be a great conference, and I say that not just because I’m part of the organizing committee.

These are all new talks, so I’ve got my plate full, but the way things are shaping up, I’m going to enjoy doing them, and I hope you’ll like them too. And this is not all folks, other events are already in discussion, so expect more.

Now, excuse me, I’ve got a few talks to prepare.

Legacy Code To Testable Code #1: Renaming

This post is part of the “Legacy Code to Testable Code” series. In the series we’ll talk about making refactoring steps before writing tests for legacy code, and how they make our life easier.

Renaming is easy and is usually safe. Most IDEs have the functionality, and most languages (I’m not talking about you, C++) lend themselves to safe renaming.

Why start at renaming? It helps make the code more understandable. When we can understand the code better, the tests we write will be more effective. In other words: don’t write tests for code you don’t understand.

Renaming is an easy win on the way there.

Naming things is maybe the hardest thing in programming. Lots of code grows complex because we slap "Manager" as a class suffix. It's like giving ourselves permission to make room for code we don't know where to put. It doesn't end with one class, though. Once there's an AccountManager class, soon BankManager and CustomerManager will appear. The same goes for a getValidCustomer method that should really be a void method, but instead returning a success code. When we’re sloppy, we allow for generalized, confusing names to flourish in the code. When names are vague, all kinds of implementation pours in.

It doesn't matter if I wrote the code, or somebody who doesn't work here anymore did. Legacy code can always do with improvement.

One of our main goals in writing tests is to improve code. But improving code safely without effort is a bonus. Risk is key here. If the IDE can do the renaming safely, we are more likely to do it. If, on the other hand, we need to rely on manual changes, chances are we won't.

Mostly, when we're doing pre-test renaming, we'll concentrate more on method names, and maybe variables in the code. These are usually small enough for picking good names (and if not enough, can be extracted). Renaming classes is usually harder, because they generally cover more ground (remember how that happened, kids?).

Renaming is a part of our familiarization with the code, before testing it. Even if I don't know what to test, making the code readable helps me not only understand what it does, but also how to test it.

Renaming variables

We usually name by scope, if at all. Making the distinction helps in making sense. If there’s already a convention in the code (like m_ prefix for fields), make sure that the convention is followed in the code you’re observing. If there isn’t a convention, start one.

Compare the type of the variable to the method and to their type. If it can be improved, rename it.
For example:
Acct a = BankManager.getAccount();

We can rename a to account, and the we wouldn’t need to remember what a is in the next 500 lines of our method. If the method returning the value seems confusing, its type can help you rename it.

Don’t skimp on vowels! It’s starts as a clever way to save screen space, but after the vowels go, we think about other options, and soon we’re left with: acct. Not only less readable, but also annoying. Make the code readable.

Apart from renaming, if you can tidy up the code, put the declarations into one area, the beginning of class or methods. If you find declaration spread around, clean it up.

Renaming methods

Method are harder to rename because they tend to do more, because of the aforementioned sloppiness. We usually start with a simple name, and then find the method the best place to do a couple things more . We soon have a big method, with a name reminding us of what the method was back then.

This is confusing to the reader, and of course makes it harder to test. But we’re not there yet. For now, make the renaming simple: If the method returns something prefix it with get. If it does something, make sure it start with a verb like set or make or call.

Check out the last lines of the method, or the exit points. Usually (not always), from the structure you’ll see what is the purpose of the function. Try to make the name fit the purpose. It may not be always the case though, so be careful.

Don’t skimp on vowels! And don’t worry about the screen space. Make method names convey their purpose, and use the entire alphabet for it.

Renaming classes

These are the hard ones to rename, and I usually recommend not to (at least not until you cut them down to size). We only see the types at declaration or creation time, so renaming them won’t bring us much benefit in terms of understanding.

It might be beneficial to identify a base or derived class in its name. Mostly it won’t, and lends itself to get nasty later, when adding a third hierarchy layer, for example.  I still like an I prefix to an interface, although it may get me killed in some communities. And always remember kids:

Don’t skimp on vowels! Applies to classes too.

Now that we’re done with renaming, it’s extraction time. Up next.
Related Posts Plugin for WordPress, Blogger...
Twitter Delicious Facebook Digg Stumbleupon More