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.

From Legacy Code To Testable Code–Introduction

The word "legacy" has a lot of connotations. Mostly bad ones. We seem to forget that our beautiful code gets to “legacy“ status three days after writing it. Michael Feathers, in his wonderful book “Working Effectively With Legacy Code” defined legacy code as code that doesn't have tests, and there is truth in that, although it took me a while to fully understand it.

Code that doesn't have tests rots. It rots because we don't feel confident to touch it, we're afraid to break the "working" parts. Code rotting means that it doesn't change, staying the way we first wrote it. I'll be the first to admit that whenever I write code, it comes in its ugly form. It may not look ugly immediately after I wrote it, but if I wait a couple of days (or a couple of hours), I know I will find many ways to improve it. Without tests I can rely either on the automatic capabilities of refactoring tools, or pure guts (read: stupidity).

Most code doesn't look nice after writing it. But nice doesn't matter.

Because code costs, we'd like it to help us understand it, and minimize debugging time. Refactoring is essential to lower maintenance costs, and therefore tests are essentials.

And this is where you start paying

The problem of course, is that writing tests for legacy code is hard. Code is convoluted, full of dependencies both near and far, and without proper tests its risky to modify. On the other hand, legacy code is the one that needs tests the most. It is also the most common code out there - most of time we don't write new code, we add it to an existing code base.

We will need to change the code to test it, in most cases. Here are some examples why:

  • We can't create an instance of the tested object.
  • We can't decouple it from its dependencies
  • Singletons that are created once, and impact the different scenarios
  • Algorithms that are not exposed through public interface
  • Dependencies in base classes of the tested code.

Some tools, like PowerMockito in Java, or Typemock Isolator for C# allow us to bypass some of these problems, although they too come with a price: lower speed and code lock-down. The lower speed come from the way these tools work, which makes them slower compared to other mocking tools. The code lock-down comes as a side effect of extra coupling to the tested code - the more we use the power tools' capabilities, they know more about the implementation. This leads to coupling between the tests and the code, and therefore make the tests more fragile. Fragile tests carry a bigger maintenance cost, and therefore people try not to change them, and the code. While this looks like a technology barrier, it manifests itself, and therefore can be overcome, by procedure and leadership (e.g., once we have the tests, encourage the developers to improve the code and the tests).

Even with the power tools, we'll be left with some work. We might even want to do some work up front. Some tweaks to the tested code before we write the tests (as long as they are not risky), can simplify the tests. Unless the code was written simple and readable the first time. Yeah, right.

We’ll need to do some of the following:

  • Expose interfaces
  • Derive and implement classes
  • Change accessibility
  • Use dependency injection
  • Add accessors
  • Renaming
  • Extract method
  • Extract class

Some of these changes to the code is introducing "seams" into it. Through these seams, we can enter probes to check the impact of the code changes.  Other changes just help us make sense of it. We can if these things are refactoring patterns or not. If we apply them wisely, and more important SAFELY, we can prepare the code to be tested more easily and make the tests more robust.

In the upcoming posts I’ll look into these with more details.

Image source: http://memecrunch.com/meme/19PWL/exploring-legacy-code

Testability != Good Design

It's a funny thing, testability. It's not really defined, or rather, it is defined poorly.

If testable code is code we can test, that means all code is like that. We can test it through unit tests. If it's hard we can move to functional tests. And if all fails, we can do manual testing. Even performance testing exercises the code. There might be code that tests cannot exercise, but then why did we write it in the first place?

When we talk about testability we usually mean "hard to test". That is a whole discussion by itself, because "hard to test" is also subjective. If we follow the theme of testing as an investment to minimize future maintenance costs, then "hard to test" translates to  "Costly to test" or "risky to test".

So here's another fun fact: When we have a well-factored code, it requires minimal changes, if any, for it to be tested. There’s no surprise that focusing on SRP makes code testable.

Because well-designed code is testable, we tend to correlate the two. Hard to test code is usually not factored well, and the two seem to go together. Moreover, we may infer that testable code leads to good design. It can in many cases, but not always.

Here's an example. We have a Customer class with a static method that gets the balance of an Account from a Bank:

public bool isOverdrawn(String name, int limit) { return (Bank.getAccount(name).getBalance() > limit); }

This is a very straightforward, readable method. Its use of a static method (getAccount) makes it "untestable" in Java and other languages. Again, by "untestable" we really mean "hard to test", which then translates to "hard to mock". In our case, using regular methods, it will be hard to mock the static method and control the input.

If we rule out use of PowerMockito, we need to modify our code to make it ”testable”. We can refactor it to pass the Account as a parameter. Once the Account is passed as an argument (really as an interface), we can mock the IAccount interface and pass it in. We now have testable code.

bool isOverdrawn(int limit, IAccount account) { return (account.getBalance() > limit); }

But has the design improved?

The method is readable as before. We exposed the type of account, although I’m not sure we even needed to know about it. We no longer have the name parameter, but instead, we needed the caller to extract the account before the method call, while originally it did not need to bother with the Bank at all.

The design has changed, but maybe not for the better. It definitely complicated the calling code.

Now let's try another design change for the sake of testability. This time instead of extracting a parameter, we’ll inject it with a dependency injection container (I’ll use Geuce). For that we need to modify the Customer class, and add:

private IAccount account; @Injectpublic void setAccount(IAccount account) { this.account = account; } bool isOverdrawn(int limit) { return (account.getBalance() > limit); }

Has its design improved now?

For the Customer class, we’ve added an unnecessary public setter, and a field we didn’t need before. If the calling code just used the setter, we’ll be in a similar condition to the last example, but using a DI framework makes the calling code, including wiring and configuration again, more complicated. (By the way, in .net it looks a bit better, but not by much.)

You may argue that sacrificing the simplicity of the calling code in order to make the design of the tested object is ok. But if you’re going to test the calling code, you’ll need to use the same tricks, and if you’re not, well, you just made it more complex and susceptible to bugs. You should test it.

Testable code is not inherently designed better

Sometimes the changes are risky and costly. We need to balance the need for testing with the risk, and how the tools we use impact the design.

And let’s remember: this code was not “untestable”. We set a constraint to not use PowerMockito, and tried to work around it. We could easily tested that code as-is.

For years I've heard that tools like PowerMockito and Typemock Isolator, encourage bad design, because they allow to test badly designed code. It sounds bad, but it maybe a better solution than making risky changes so you can just test. Sometimes the changes are not even risky, but will create a more complex code, where it should be simple.

Testing and design are broad skills every developer should have.

As long as you’re making a knowledgeable decision, not based on popular slogans, you’ll be fine.

Image source: https://www.flickr.com/photos/microassist/7268711202/

I Did Scrum and All I Got Was This Lousy Shirt

So this came up a few days ago: “Why Scrum Should Basically Just Die In A Fire”.

My first thought was: “Of course scrum is failing him, he’s not actually doing scrum.”

Second thought was: “He doesn’t understand agile at all, does he?”

Third thought was: “Gil, you’re an idiot.”.

Because I have been saying this was going to happen. Last time was on the “why agile is declining” sequel.

No Surprises

We shouldn’t be surprised that people are struggling with scrum or agile, because change is hard.

Bad experiences, coached or self-inflicted, guide our decisions. We connect the results to the experience, because if there’s a name on it, even better. Replace scrum, with kanban, SAFe or XP and I’m sure you’ll find people with similar experiences, who won’t touch agile again.

We know they are “doing it wrong”. That if only had the right coaching, everything would be better. It doesn’t matter.

Here’s a shocker: scrum never succeeds.

When you hear about successful scrum implementation from the people who went through it, they are no longer talk about bare-bone scrum. They talk about a working process that has similarities to scrum. The process is working for the team, and they are using “scrum” as the name they know. Scrum is a toolbox, and a small one at that. It can’t do the work, and it needs to be adopted to the team, the organization. To the people.

So regardless to what coaches may tell you, scrum can’t work, because it’s only a starting point. And we don’t declare wins and losses at the beginning.

The Conflict

On one hand, we deserve this. If scrum doesn’t deliver, and blows up in the face, regardless of where the fault lies, it’s because we, the agile community are responsible to explain scrum. If we communicated expectations better, and did not make it seem so simple, we wouldn’t be in this mess.

On the other hand, we, or should I say I, know that it works. Not as prescribed, but adapted specifically, gradually to how a team works.

It’s easy selling scrum as a silver bullet, because everybody is doing it. Just not the same.

For some it’s a miserable experience, and they call us on it.

The last thing we should do, is blame them for “not understanding agile”.



Image source: http://agile.logihelgu.com/category/agile/scrum/page/2/

Related Posts Plugin for WordPress, Blogger...
Twitter Delicious Facebook Digg Stumbleupon More