Refactoring for the refactoring's sake
Imagine for a minute that you have inherited a legacy code base (According to Michael Feathers, a legacy code base is one that is largely untested, if at all) but also have a whole list of new features that you need to build upon the existing code with a deadline. What would you do?
Available options:
1. Rip the code apart and start the cleaning process before adding new features.
2. Pile the code for the new features on top of the existing ball of mud.
3. Is there a 3rd option?
The issue with Option 1 is that it might be a while before we can deem the code base ready for development of new features, and what is the proper acceptance criteria for refactoring stories anyway? I don't think Option 2 should be an option at all, because that would just mean we're shooting ourselves in the foot, screaming in pain, and doing it over and over again. I learned recently that another thing we could do is to tack refactoring on top of new feature stories, which means whatever existing code that you touch to make things work for the new stuff that you're writing, you should stop and clean it up and write tests for it. The problem, as with all refactoring tasks, is when do we stop? I don't think I've found a good answer to that question just yet, but just that it should be decided by the team.
Why do I write tests?
It seems like I've been a testing nazi on the loose lately. I can't help it. It stresses me out when people say things like, "We've made a conscious decision not to write tests" or "Writing tests just takes too long. I don't see the benefit of spending my precious time doing that". So why do I do it? I'm sure I've spoken about one or more of the following points on previous posts but I shall do it again in (maybe vain) attempts to emphasize their importance.
1. Writing tests ensures me that what I'm currently developing works as expected.
2. Writing tests gives me the confidence to make changes in the code with little to no fear of breaking existing functionality. Change is inevitable in any software project and it's not fair to blame the appearance of defects on changing requirements. It's how we deal with change that matters. I think testing is one of the many ways of dealing with the risks and disruption that comes with change.
3. Tests (especially unit tests that run really fast) give me quick feedback. I don't want to have to write code, check it in, wait for a build to the QA environment and have someone else test it to verify that it works correctly. I wrote the code, I should be the one to test it.
It might seem longer to finish a story or functionality but in the long run, we'd be spending less time fixing defects, regression and otherwise, (and acquiring less heartache too) and more time building new functionality. It takes time to build up a comprehensive suite of tests and a lot of discipline amongst developers to keep up the practice but I believe it is worth it and I can't really work any other way.
Your testing tendencies: Survey results
I promised a few people that I would share the results of the survey I posted about a week ago. A big thank you to all who participated! I really appreciate your time and honesty.
Total number of responses: 118
Question 1: Do you write automated tests? (i.e. unit, integration, functional tests, etc)
Yes - 106 - 90%
No - 12 - 10%
Question 2: Does your project have an automated build?
Yes - 101 - 86%
No - 17 - 14%
Question 3: Do you run the build before checking in?
Yes - 75 - 64%
No - 43 - 36%
Question 4: What types of tests do you think developers should be responsible for writing? (Since people are allowed to check more than one option, the numbers will add up to be > 100%)
Unit tests - 114
Integration tests - 98
Functional tests - 69
None. Developers shouldn't be writing tests. That's the job of a QA - 0
None. It takes too long to write tests. I should focus on writing production code - 1
Other - 121
Question 5: How do you verify that new code that was written behaves as it should?
I write tests - 74 - 62%
I do a quick smoke run through the application and make sure that it works from the browser - 22 - 19%
Other - 22 - 19%
Question 6: How do you make sure functionality that has been completed still works correctly?
I write tests that run periodically - 86 - 73%
I deal with regression defects as they appear and fix them as soon as possible - 19 - 16%
I don't know of a way. I just hope that nothing breaks as I add new code - 5 - 4%
Other - 8 - 7%
A few observations:
Overall, most of the people who responded do write tests.
Some developers write tests but don't run the build before checking in.
Developers have some way of running a build before checking in, but that build does not belong to the team as a whole.
Developers don't write tests, but do run a build before checking in, and that build belongs to the team.
I get the impression that some people associate writing tests to practicing test-driven-development.
So, what, you would wonder, would I do with the results of this survey. Initially it was plainly a selfish action on my part to try to gauge how different teams and people approach software development and testing practices because I haven't been very encouraged with what I've seen on many of the projects I've been on. Some people mentioned that they do not know of a way to reduce regression defects and effectively be sure that what was developed in the past still works in the present. I do not profess to know the answers to all your questions, but I am willing to take a stab at it. Feel free to post a comment here if you'd like to discuss testing options and strategies for your project. If I do not know the answer up front, I will definitely find someone who does.
Your testing tendencies
In the enterprise world, it is usually safe to assume that our code-bases are relatively large and involve many moving pieces. Different teams (and individuals) have different beliefs about the practice of writing automated tests to verify the correctness of code. I want to learn and understand those beliefs and the reasons behind them.
Please visit this link and give me your honest feedback. Feel free to also comment on this post. I'd like to hear from as many of you as possible.
Thanks!
[Edited April 8 2010]
I'm taking down the survey and am currently working on posting the results.
Book Review: Working Effectively with Legacy Code – Chapter 6
Working Effectively with Legacy Code by Michael Feathers
I've been carrying around this book with me for the past few weeks and I find myself going back and reading parts of a chapter again and again, trying to soak in the concepts. This particular chapter, titled: I Don't Have Much Time And I Have To Change It, struck me specifically when Feathers starts out talking about change and how often it happens in code bases and that steps should be taken to make every consequent change easier than the one before. When teams make it a point to only introduce changes to the code only if they have tests to cover that change, they find that their velocity slows down and people feel like they aren't getting as much done as they need to. Feathers emphasizes that this is normal but if they persevere, they slowly realize that they are revisiting better code and changes get easier and easier to make.
I think that it's important for teams to understand the benefits of testable code and how it eases the process of change so that they will invest the time and energy needed to get the code to that point. And I quote:
Ultimately, testing makes your work go faster, and that's important in nearly every development organization.
Having code that is testable is only possible if we do one of the following (If I remember correctly, I think this is according to Uncle Bob):
- Write the tests before writing the code
- Design for testability
Doing the latter isn't easy, and in fact it is so hard to achieve that people end up not doing it at all. So that leaves us with Option 1. But sometimes you come across code that doesn't have any tests AND doesn't look like it is testable, but you have to make some changes in a short amount of time. So what do you do? Michael Feathers suggests 4 approaches, and my colleague Mark documents it in a short and sweet way (basically he beat me to writing the post first).
- Sprout Method
Advantages: Separating new code from old code. New code is testable.
Disadvantages: You're giving up on the old code for now. - Sprout Class
Advantages: Allows you to move forward with more confidence.
Disadvantages: Conceptual complexity - moving new code into a separate class disrupts the flow of how key classes in the code base work together. - Wrap Method
Advantages: A great way to introduce seams (this is a concept that Feathers talks about in a previous chapter, which we will visit in a later post) while adding new features.
Disadvantages: This method requires you to rename the method that you want to change, and create a new method with the same name as the previous method before the rename was done. Sometimes this could introduce sloppy method names because we needed to do some renaming to make way for the new method. - Wrap Class
This method is pretty much the same as Wrap Method except you're extracting new behavior out into a new class. Feathers mentioned that 2 things push him towards choosing to use Wrap Classes and one of them is when a class has grown so large that he cannot stand to make it worse. Pushing behavior into a new class when the existing class is unmanageably large could also indicate that the original class might have too many responsibilities and thus needs to be gutted.
I like this chapter because it formalizes a lot of things that I try to do very often because of the large range of code bases that I work with, but find it difficult to nail it down to something that could be explained to someone else.
Testing: Unit testing on the UI in Flex (Part 3)
So how does the view model for the test in my previous post look like? Judging from the test cases written, we would have 3 public methods:
- viewModel.valid
- viewModel.hasDuplicates
- viewModel.save which depends on the first two
public
{
public var dispatcher:IEventDispatcher;
[Bindable]
public var subscriptions:ArrayCollection;
private var subscriptionsFromServer:ArrayCollection;
public {}
public :void
{
if(subscriptions == null)
subscriptions = new ArrayCollection();
subscriptions.addItem(SomeViewModel.createNewViewModel());
}
public function get valid():Boolean
{
if(subscriptions == null)
return false;
for each(var subscription:SomeViewModel in subscriptions)
{
if(!subscription.valid)
return false;
}
return true;
}
public function get hasDuplicates():Boolean
{
for each(var subscription:SomeViewModel
in subscriptions)
{
var remainingSubscriptions:ArrayCollection =
removeFirstOccurenceOf(subscription);
if(containsDuplicateValues(remainingSubscriptions, subscription))
return true;
}
return false;
}
public :void
{
if(hasChanges && valid)
{
var subscriptions:ArrayCollection =
SomeViewModel.convertToDomainModels(subscriptions);
var event:SomeEvent =
SomeEvent.createSaveSubscriptionsEvent(subscriptions);
dispatcher.dispatchEvent(event);
}
}
}
Since Mate is event-based, the act of clicking on the 'Save' button causes the dispatching of an event, and the dispatcher is always the view, which is injected to the View Model via the Controller.
At the end of this, we have the functionality that pertains to the 'Save' button, but the view doesn't need know about these complications. The controller will wire up the 'Save' button to invoke the save method on the view model, which in turn will only dispatch the event back to the controller if there is a need.
Testing: Unit testing on the UI in Flex (Part 2)
Continuing on my previous post, say we have the following view:
In this screen, a user is allowed to add a new subscription in the system that s/he cares about, view existing subscriptions, remove existing subscriptions and subsequently save those subscriptions. Let's focus on a small portion of the functionality that is outlined in this view. When the user adds new subscriptions, saving is only allowed if:
- All the required fields have been selected
- There are no duplicates.
So how do we verify that behavior? Forgetting about the view for a moment, let's think about the presentation model (I will call it view model) class, which holds the state and logic behind the view. There will be a 'save' method on that class which will only perform its magic when the view model itself is valid, which means all required fields have been selected, and there aren't any duplicate subscriptions. So let's write the tests for that behavior:
public
{
[Test]
public :void
{
//given there are no subscriptions
//then
assertFalse(viewModel.valid);
}
[Test]
public :void
{
//given
viewModel.addNewSubscription();
viewModel.addNewSubscription();
//when
for each(var subscription:SomeViewModel in viewModel.subscriptions)
{
subscription.type = TestHelper.SUBSCRIPTION_TYPE_A;
subscription.group = TestHelper.GROUP_2;
subscription.priority = TestHelper.ACTIONABLE_PRIORITY;
}
//then
assertTrue(viewModel.valid);
}
[Test]
public :void
{
//given
viewModel.addNewSubscription();
viewModel.addNewSubscription();
//when
viewModel.subscriptions[0].type = TestHelper.SUBSCRIPTION_TYPE_A;
viewModel.subscriptions[0].group = TestHelper.GROUP_1;
viewModel.subscriptions[0].priority = TestHelper.ACTIONABLE_PRIORITY;
//then
assertFalse(viewModel.valid);
}
[Test]
public :void
{
//given
var subscriptionsFromServer:ArrayCollection = new ArrayCollection();
subscriptionsFromServer.addItem(helper.
createSubscriptionTypeADomainModelWithInformationalPriority());
subscriptionsFromServer.addItem(helper.
createSubscriptionTypeBDomainModelWithActionablePriority(TestHelper.GROUP_1));
viewModel.populateSubscriptionsFromServer(subscriptionsFromServer);
//when
viewModel.addNewSubscription();
viewModel.subscriptions.setItemAt(helper.
createSubscriptionTypeADomainModelWithInformationalPriority(), 2);
//then
assertTrue(viewModel.hasDuplicates);
}
[Test]
public :void
{
//given
var subscriptionsFromServer:ArrayCollection = new ArrayCollection();
subscriptionsFromServer.addItem(helper.
createSubscriptionTypeADomainModelWithInformationalPriority());
subscriptionsFromServer.addItem(helper.
createSubscriptionTypeBDomainModelWithActionablePriority(TestHelper.GROUP_1));
viewModel.populateSubscriptionsFromServer(subscriptionsFromServer);
//when
viewModel.addNewSubscription();
viewModel.subscriptions.setItemAt(helper.
createSubscriptionTypeBDomainModelWithActionablePriority(TestHelper.GROUP_1), 2);
//then
assertTrue(viewModel.hasDuplicates);
}
[Test]
public :void
{
//given
var subscriptionsFromServer:ArrayCollection = new ArrayCollection();
subscriptionsFromServer.addItem(helper.
createSubscriptionTypeADomainModelWithInformationalPriority());
subscriptionsFromServer.addItem(helper.
createSubscriptionTypeBDomainModelWithActionablePriority(TestHelper.GROUP_1));
viewModel.populateSubscriptionsFromServer(subscriptionsFromServer);
//then
assertFalse(viewModel.hasDuplicates);
}
}
}
The view model has 2 attributes that are used to determine whether subscriptions can be saved or not: valid and hasDuplicates. Next we'll take a look at the view model itself and how it ties back to the view.
Testing: Unit testing on the UI in Flex (Part 1)
I seem to be obsessed with testing lately and after watching J.B. Rainsberger's video, I feel even more convinced that good developer testing plays a big part in producing code that is maintainable and not adverse to change in addition to providing the assurance to the developer that a functionality is done when they say it is.
Testing UI code is often a challenge and most projects I've been on defer that to acceptance tests that are driven from the browser. I'm not disregarding browser tests entirely but like I've mentioned in my previous post, that could be a suboptimal testing option.
Mate (pronounced Mah-tay), which is a tag-based Flex framework, provides the ability to define a separation between the state and behavior of the presentation of a view and the UI controls of the view itself, thus allowing for extensive unit testing of the UI. Before I delve deeper into how that works, let me talk a little bit about the Presentation Model pattern, advocated by Martin Fowler. Given a Model-View-Controller situation, the idea of the pattern is to remove all logic from the view into the presentation model, which is a model class that is part of the presentation (or view). This gives us a few advantages:
- The view becomes dumb and simple. No logic resides here, just UI components.
- The view could potentially be reused to display other data by just switching out the model.
- And more importantly, we are now able to test the behavior of the view (i.e. what happens when a button is clicked, are multiple drop-down menus dependent on each other, and if so, how?)
We will take a look at how to test the behavior and state of the view in my subsequent posts.
Testing: Take that log out of your eye before you can spot the speck in your neighbor's eye
On one of my previous projects, we started on a story (or functionality) by first writing the acceptance test for it. We had a custom-written BDD (Behavior-Driven-Development)-like tool that drove the test from the browser. I initially found it quite cool that it was set up in a way that it made it easy for developers to write acceptance tests. One of the downsides of writing these kinds of tests is the slow feedback loop. We of course also wrote unit tests but to verify a particular functionality, we ran the acceptance tests. Over time, the team built up a sizable suite of browser tests and the build started to become so brittle that it was red about 70% of the time. Another downside that we had with those tests was the fact that there were a lot of duplicates. Certain parts of the application were accessed with the same initial flow and each of those tests repeated them, hence slowing down the build even more.
I've recently learned that relying primarily on acceptance tests (or integration tests) isn't such a good idea after all, in fact it's actually quite a bad idea. In an application with many layers, like most applications are, we have to make sure that every single layer has its own suite of tests that verify behavior regardless of its external collaborators. J.B. Rainsberger has an excellent presentation called 'Integration Tests Are A Scam' in which he explains why integration tests can cause a project more harm than good. He defines integration tests to be tests that pass or fail depending on the correctness of multiple objects with multiple unrelated methods that have multiple pieces of interesting behavior where interesting is complex enough to be tested on its own. Acceptance tests are a level of abstraction higher than integration tests, therefore increasing in complexity and number of points of possible failure.
Besides slow feedback, why else would you NOT want to write too many acceptance/integration tests? Say you have an application that is structured in the following way:
This is a typical set up of most projects that I've been on. Say for example that you have tests that run on the repository layer but depend heavily on data being in the database, and if the data changes or if the structure of the database changes, you will definitely have broken tests. But you say, "Each test that runs against the database sets up and tears down data that's pertinent to that test only. And we recreate the database at every run of the build. So the database could be empty for all we care about." Okay, that's all good and fair, but are you going to write ALL your tests for the repository against the database? How many different permutations of those tests can you come up with that would test everything exhaustively? And if you are able to list all those permutations, how long do you think those tests will take to run?
Let's take this scenario further. You have tests at the repository layer that depend on the database, and then tests at the service layer that depend on the repository layer. Your development team does not have complete control over the database and its data. It changes under your feet a lot and the build is broken almost all the time and the team does not have enough bandwidth and energy to keep fixing them. Then someone says, "We need to drive tests from the UI as well!". Let's take a look at how that would look:
As my colleague Prem has said many times, "How much hope do the tests driven from the UI will have of passing if everything underneath it is failing?". Not only are you introducing tests that take longer to run, you're also increasing the number of tests that will continue to fail thus making the lives of developers even harder.
I've learned that developers need to focus more on tests that are:
- Fast, lightning fast.
- Independent of external collaborators, and thus less brittle and prone to breakage because of something other than behavior change in the code immediately under test.
- Exhaustive. Since they are fast to begin with, we can write as many tests for as many given scenarios as we can think of.
Change is inevitable. And to support change, developers need to be able to move nimbly. Writing tests in a focused manner at the unit level reinforces that nimbleness and allow developers to work more effectively.
Why one should not live without tests
I find it hard to imagine myself writing any sort of code without a test to back it up. But I've learned that not everyone shares that sentiment. So here's an attempt to explain why developer tests are extremely vital to reliable and working software.
[Disclaimer]: These are not my thoughts alone. They have been collated from a number of books and blogs that I've read over the months. A few of which are Clean Code by Robert Martin and Working Effectively with Legacy Code by Michael Feathers.
[Yet another disclaimer]: My colleague Pat Kua reminded me that I should avoid blanket statements that say developers should write tests all the time. Thanks Pat! He writes about when tests should and should not be written here. So I should say that I'm talking specifically about developers writing production code in a focused mode with a good idea of how the resulting code should behave. And I'm not even bringing up Test Driven Development, but just the fact that having tests as an assurance that what you've written behaves in the way you expect and having that assurance continuously going forward.
1. Getting fast feedback
When you write a piece of functionality on a web application for example, arguably you could launch the application, drill down to where the code that you wrote is executed and test whether it behaves the way you expect it to. One of the problems with that approach is that the feedback loop is large. Depending on how your development environment is set up, it might take a whole 10 minutes (or more) to bring up the application on the browser, login (if you have to), drill down to the part of the application that you're interested in and finally test its functionality. With a test, you could run it, and it would probably take 5% (or much less) of that 10 minutes to execute and tell you whether your code behaves the way you think it should. Different types of tests have different levels of feedback loops.
The lowest level of tests you can write are unit tests - these test the behavior of the lowest unit of code that you can have, which is usually a class. These tests should run extremely fast i.e. you could execute a few hundred of them in a matter of seconds. Then there are integration tests that test interaction between two or more components, these could involve database or file system access. These could potentially take longer to run since they interact with agents outside your application and should be written with care as to not increase the build time unnecessarily. Functional or acceptance tests come next. These are usually written against your application as a whole (i.e. through the browser) and should test features of the application on a higher level and mimic the interaction of a user with your application. These tests typically take the longest to run.
2. Tests document your code
This brings up yet another debate about how the functionality of an application should be documented for the users or future consumers of the code. As a developer, the best documentation of an application is always the tests. If I want to know what a class is supposed to do and how it's supposed to behave, if there was a test, I could take a look at that and in theory, understand what I need to know in a relatively short time as opposed to trying to understand the code. Tests acting as documentation also beats external documentation because if a functionality changes, Word or PDF documents describing that change don't always get updated and with time developers will lose visibility into how exactly the code should behave.
3. Gives you confidence to change your code
I often hear developers say things like, "I'm afraid to make that change there because I don't know what will break after that". That doesn't mean that the particular developer is incompetent of understanding what the code is supposed to but it's a symptom of a bigger problem which is the fact that there is no assurance whether a change we make in one portion of the application will not break a functionality in another portion. In his book, Working Effectively With Legacy Code, Michael Feathers points out that tests are "kind of like a cloak we put over code we are working on to make sure that bad changes don't leak out and infect the rest of our software. When we have a good set of tests around a piece of code, we can make changes and find out quickly whether the effects were good or bad".
A lot of times there is a steep ramp up for us to be able to get to the point where we can write any kind of test. For example if our code base was in a condition where there is tight coupling everywhere, or there aren't clear-cut components that are testable. These impediments would seem to indicate that writing tests would be a waste of time and that we should focus our energies on producing new functionality. Then what I would suggest you do is take a step back and evaluate your priorities.
Would you rather:
- Spend a chunk of time now reorganizing your code and making it testable, so that you can reap the benefits of having the assurance that whatever you have built still works and will continue to work, and know that whatever that you are currently building or will build in the future will fail faster so that you can fix it and move on.
- OR, having to go back and forth every time new functionality is built or when changes are made in existing functionality to make sure that they all still work as they should.
Contact Me
Archives
- January 2012 (2)
- December 2011 (1)
- October 2011 (1)
- August 2011 (3)
- June 2011 (2)
- March 2011 (1)
- February 2011 (1)
- November 2010 (3)
- September 2010 (1)
- August 2010 (1)
- July 2010 (2)
- June 2010 (1)
- May 2010 (2)
- April 2010 (2)
- March 2010 (4)
- February 2010 (1)
- November 2009 (5)
- October 2009 (4)
- September 2009 (2)
- August 2009 (3)
- July 2009 (2)
- May 2009 (4)
- April 2009 (9)
- March 2009 (1)
- February 2009 (5)
- January 2009 (4)
- December 2008 (6)
- November 2008 (5)
- October 2008 (3)
- September 2008 (1)
- August 2008 (1)
Categories
- .net (3)
- active record (1)
- agile (9)
- band (1)
- books (13)
- c# (3)
- clojure (2)
- coding (2)
- coding practices (8)
- conferences (1)
- consulting (3)
- continuous intergration (2)
- error handling (1)
- flex (3)
- functional programming (2)
- git (1)
- grails (2)
- hibernate (1)
- integration (2)
- java (2)
- lean (2)
- logging (1)
- mocking (1)
- music (1)
- oop (1)
- osx (2)
- pairing (6)
- philosophy (2)
- photography (1)
- rails (1)
- ramble (10)
- ruby (2)
- Sydney (3)
- tdd (2)
- teamwork (5)
- testing (17)
- ThoughtWorks (19)
- twu (2)
- UI (3)
- web (1)