Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

I usually try to follow the advice of the book Working Effectively with Legacy Code. I break dependencies, move parts of the code to @VisibleForTesting public static methods and to new classes to make the code (or at least some part of it) testable. And I write tests to make sure that I don't break anything when I'm modifying or adding new functions.

A colleague says that I shouldn't do this. His reasoning:

  • The original code might not work properly in the first place. And writing tests for it makes future fixes and modifications harder since devs have to understand and modify the tests too.
  • If it's GUI code with some logic (~12 lines, 2-3 if/else block, for example), a test isn't worth the trouble since the code is too trivial to begin with.
  • Similar bad patterns could exist in other parts of the codebase, too (which I haven't seen yet, I'm rather new); it will be easier to clean them all up in one big refactoring. Extracting out logic could undermine this future possibility.

Should I avoid extracting out testable parts and writing tests if we don't have time for complete refactoring? Is there any disadvantage to this that I should consider?

share|improve this question
26  
Seems your colleague is just presenting excuses because he does not work that way. People sometimes behave like this because of beeing too tenacious to change their adopted way of doing things. –  Doc Brown Feb 6 at 7:20
3  
what should be classed as a bug may be relied on by other parts of the code turning it into a feature –  ratchet freak Feb 6 at 15:42
1  
The only good argument against that I can think of is that your refactoring itself could introduce new bugs if you've misread/miscopied something. For that reason I'm free to refactor and fix to my heart's content on the version currently under development -- but any fixes on past versions face a much higher hurdle and may not be approved if they are "only" cosmetic/structural cleanup since the risk is deemed to exceed the potential gain. Know your local culture -- not just one cow-orker's idea of it -- and have EXTREMELY strong reasons ready before doing anything else. –  keshlam Feb 6 at 23:28
5  
The first point is kind of hilarious – “Don’t test it, it might be buggy.” Well, yes? Then it’s good to know that – either we want to fix that or we do not want anyone to change the actual behavior to what some design spec said. Either way, testing (and running the tests in an automated system) is beneficial. –  Christopher Creutzig Feb 7 at 6:17
3  
Too often the "one big refactoring" that's about to happen and that will cure all ills is a myth, concocted by those who simply want to push things they consider boring (writing tests) into the distant future. And if it ever does become real, they will seriously regret having let it become so big! –  Julia Hayward Feb 7 at 10:01

10 Answers 10

Here's my personal unscientific impression: all three reasons sound like widespread but false cognitive illusions.

  1. Sure, the existing code might be wrong. It might also be right. Since the application as a whole seems to have value to you (otherwise you'd simply discard it), in the absence of more specific information you should assume that it is predominantly right. "Writing tests makes things harder because there's more code involved overall" is a simplistic, and very wrong, attitude.
  2. By all means expend your refactoring, testing and improvement efforts in the places where they add the most value with the least effort. Value-formatting GUI subroutines are often not the first priority. But not testing something because "it's simple" is also a very wrong attitude. Virtually all severe errors are committed because people thought they understood something better than they actually did.
  3. "We will do it all in one big swoop in the future" is a nice thought. Usually the big swoop stays firmly in the future, while in the present nothing happens. Me, I'm firmly of the "slow and steady wins the race" conviction.
share|improve this answer
21  
+1 for "Virtually all severe errors are committed because people thought they understood something better than they actually did." –  rem Feb 6 at 9:09
    
Re point 1 - with BDD, the tests are self documenting... –  Robbie Dee Feb 6 at 9:34
2  
As @guillaume31 points out, part of the value of writing tests is demonstrating how the code actually works -- which may or may not be in accordance with the specification(s). But it could be the spec that's "wrong": business needs may have changed and the code reflects the new requirements but the spec doesn't. Simply assuming the code is "wrong" is overly simplistic (see point 1). And again the tests will tell you what the code actually does, not what someone thinks/says it does (see point 2). –  David Feb 7 at 13:20

A few thoughts :

When you're refactoring legacy code, it doesn't matter if some of the tests you write happen to contradict ideal specifications. What matters is that they test the program's current behavior. Refactoring is about taking tiny iso-functional steps to make the code cleaner; you don't want to engage in bugfixing while you're refactoring. Besides, if you spot a blatant bug, it won't be lost. You can always write a regression test for it and temporarily disable it, or insert a bugfix task in your backlog for later. One thing at a time.

I'd agree that pure GUI code is hard to test and perhaps not a good fit for "Working Effectively ..." -style refactoring. However, this doesn't mean you shouldn't extract behavior that has nothing to do in the GUI layer and test the extracted code. And "12 lines, 2-3 if/else block" is not trivial. All code with at least a bit of conditional logic should be tested.

In my experience, big refactorings are not easy and they rarely work. If you don't set yourself precise, tiny goals, there's a high risk that you embark on a never-ending, hair-pulling rework where you'll never land on your feet in the end. The bigger the change, the more you risk breaking something and the more trouble you will have finding out where you failed.

Making things progressively better with ad hoc small refactorings isn't "undermining future possibilities", it's enabling them - solidifying the swampy ground where your application lies. You should definitely do it.

share|improve this answer
5  
+1 for "tests you write test the program's current behavior" –  David Feb 6 at 13:03

Also re: "The original code might not work properly" - that doesn't mean you just change the code's behaviour without worrying about the impact. Other code may rely on what appears to be broken behaviour, or side-effects of the current implementation. Test coverage of the existing application ought to make it easier to refactor later, because it'll help you find out when you've accidentally broken something. You should test the most important parts first.

share|improve this answer
    
Sadly true. We have a couple of obvious bugs that manifest in edge cases that we can't fix because our client prefers consistency over correctness. (They're caused due to the data-gathering code allowing things the reporting code doesn't take into account, like leaving one field in a series of fields blank) –  Izkata Feb 6 at 16:48

Kilian's answer covers the most important aspects, but I want to expand on points 1 and 3.

If a developer wants to change (refactor, extend, debug) code, she has to understand it. She has to make sure her changes affect exactly the behavior she wants (nothing in the case of refactoring), and nothing else.

If there are tests, then she has to understand the tests as well, sure. At the same time, the tests should help her understand the main code, and tests are far easier to understand than functional code anyway (unless they're bad tests). And the tests help show what changed in the behavior of the old code. Even if the original code is wrong, and the test tests for that wrong behavior, that is still an advantage.

However, this requires that the tests are documented as testing preexisting behavior, not a specification.

Some thoughts on point 3, too: in addition to the fact that the "big swoop" rarely ever actually happens, there's also another thing: it isn't actually easier. To be easier, several conditions would have to apply:

  • The antipattern to be refactored needs to be easily found. Are all your singletons named XYZSingleton? Is their instance getter always called getInstance()? And how do you find your overly deep hierarchies? How do you search for your god objects? These require code metrics analysis and then manually inspecting the metrics. Or you just stumble over them as you work, as you did.
  • The refactoring needs to be mechanical. In most cases, the hard part of refactoring is understanding the existing code well enough to know how to change it. Singletons again: if the singleton is gone, how do you get the required information to its users? It often means understanding the local callgraph so that you know where to get the information from. Now what is easier: searching out the ten singletons in your app, understanding the uses of each (which leads to needing to understand 60% of the codebase), and ripping them out? Or taking the code that you already understand (because you're working on it right now) and ripping the singletons being used there out? If the refactoring isn't so mechanical that it requires little to no knowledge of the surrounding code, there's not use in bunching it up.
  • The refactoring needs to be automated. This is somewhat opinion-based, but here goes. A bit of refactoring is fun and satisfying. A lot of refactoring is tedious and boring. Leaving the piece of code you just worked on in a better state gives you a nice, warm feeling, before you move on to more interesting stuff. Trying to refactor an entire code base will leave you frustrated and angry at the idiot programmers who wrote it. If you want to do a big swoop refactoring, then it needs to be largely automated so as to minimize the frustration. This is, in a way, a meld of the first two points: you can only automate the refactoring if you can automate finding the bad code (i.e. easily found), and automate changing it (i.e. mechanical).
  • Gradual improvement makes for a better business case. The big swoop refactoring is incredibly disruptive. If you refactor a piece of code, you invariably get into merge conflicts with other people working on it, because you just split the method they were changing into five parts. When you refactor a reasonably-sized piece of code, you get conflicts with a few people (1-2 when splitting up the 600-line megafunction, 2-4 when breaking up the god object, 5 when ripping out the singleton from a module), but you would have had those conflicts anyway because of your main edits. When you do a codebase-wide refactoring, you conflict with everyone. Not to mention that it ties up a few developers for days. Gradual improvement causes each code modification to take a little longer. This makes it more predictable, and there isn't such a visible time period when nothing happens except cleanup.
share|improve this answer

There is a culture in some companies where they're reticent to allow developers any time to enhance code that doesn't directly deliver additional value e.g. new functionality.

I'm probably preaching to the converted here, but that is clearly false economy. Clean and concise code benefits subsequent developers. It is just that the payback isn't immediately evident.

I personally subscribe to the Boy Scout Principle but others (as you've seen) don't.

That said, software suffers from entropy and builds up technical debt. Previous developers short on time (or perhaps just lazy or inexperienced) may have implemented sub-optimal buggy solutions over well designed ones. Whilst it may seem desirable to refactor these, you risk introducing new bugs to what is (to the users anyway) working code.

Some changes are lower risk than others. For example, where I work there tends to be a lot of duplicated code that can safely be farmed out to a subroutine with minimal impact.

Ultimately, you have to make a judgement call as to how far you take the refactoring but there is undeniably value in adding automated tests if they don't exist already.

share|improve this answer
2  
I totally agree in principle, but in many companies it comes down to time and money. If the "tidy up" part only takes a few minutes then that's fine, but once the estimate for the tidy up starts to get bigger (for some definition of big), you, the person coding need to delegate that decision to your boss or project manager. It is not your place to decide the value of that time spent. Working on bug fix X, or new feature Y might have a much higher value to the project/company/customer. –  jmo21 Feb 6 at 11:13
2  
You also may not be aware of bigger issues like the project being scrapped in 6 months time, or simply that the company values your time more (eg. you do something they deem more important, and someone else gets to do the refeactoring work). Refactoring work can also have an affect on testing. Will a large refactoring trigger a full test regression? Does the company have resources it can deploy to do this? –  jmo21 Feb 6 at 11:16
    
Yes, as you've touched on there are myriad reasons why major code surgery may or may not be a good idea: other development priorities, the lifetime of the software, testing resource, developer experience, coupling, release cycle, familiarity with the code base, documentation, mission criticality, company culture etc etc etc. It is a judgement call –  Robbie Dee Feb 6 at 11:29
    
+1 for citing Uncle Bob's book. –  Scotty C. Feb 7 at 3:32

In my experience a characterisation test of some sort works well. It gives you a broad but not very specific test coverage relatively quickly, but can be tricky to implement for GUI applications.

I would then write unit tests for parts that you want to change and do so every time you want to make a change thereby increasing your unit test coverage over time.

This approach gives you a good idea if changes are affecting other parts of the system and let's you get into a position to make any required changes sooner.

share|improve this answer

Re: "The original code might not work properly":

Tests are not written in stone. They can be changed. And if you tested for a feature that was wrong, it should be easy to rewrite the test more correctly. Only the expected result of the tested function should have changed, after all.

share|improve this answer
1  
IMO, individual tests should be written in stone, at least til the feature they're testing is dead and gone. They are what verify the existing system's behavior, and help assure maintainers that their changes won't break legacy code that may already rely on that behavior. Change the tests for a live feature, and you are removing those assurances. –  cHao Feb 6 at 18:12

Well, yes. Answering as a software test engineer. Firstly you should test everything you ever do anyway. Because if you don't, you don't know whether it works or not. This may seem obvious to us but I have colleagues who see it differently. Even if your project is a little one that may never be delivered, you have to look the user in the face and say you know it works because you tested it.

Non-trivial code always contains bugs (quoting a guy from uni; and if there are no bugs in it, it's trivial) and our job is to find them before the customer does. Legacy code has legacy bugs. If the original code doesn't work the way it should, you want to know about it, believe me. Bugs are ok if you know about them, don't be afraid to find them, that's what release notes are for.

If I remember rightly the Refactoring book says to test constantly anyway., so it's part of the process.

share|improve this answer

Do the automated test coverage.

Beware wishful thinking, both your own and by your customers and bosses. Much as I would love to believe that my changes will be correct the first time and I'll only have to test once, I've learned to treat that kind of thinking the same way I treat Nigerian scam emails. Well, mostly; I've never gone for a scam email but recently (when yelled at) I gave in on not using best practices. It was a painful experience that dragged (expensively) on and on. Never again!

I have a favorite quote from the Freefall web comic: "Have you ever worked in a complex field where the supervisor has only a rough idea of the technical details? ... Then you know the surest way to cause your supervisor to fail is to follow his every order without question."

It's probably appropriate to limit the amount of time you invest.

share|improve this answer

If you're dealing with large amounts of legacy code that isn't currently under test, getting test coverage now instead of waiting for a hypothetical big rewrite in the future is the right move. Starting by writing unit tests is not.

Without automated testing, after making any changes to the code you need to do some manual end to end testing of the app to make sure it's working. Start by writing high level integration tests to replace that. If your app reads files in, validates them, processes the data in some fashion, and displays the results you want tests that capture all of that.

Ideally you'll either have data from a manual test plan or be able to get a sample of actual production data to use. If not, since the app's in production, in most cases it's doing what it should be, so just make up data that will hit all the high points and assume the output is correct for now. It's no worse than taking a small function, assuming it's doing what it's name or any comments suggest it should be doing, and writing tests assuming it's working correctly.

IntegrationTestCase1()
{
    var input = ReadDataFile("path\to\test\data\case1in.ext");
    bool validInput = ValidateData(input);
    Assert.IsTrue(validInput);

    var processedData = ProcessData(input);
    Assert.AreEqual(0, processedData.Errors.Count);

    bool writeError = WriteFile(processedData, "temp\file.ext");
    Assert.IsFalse(writeError);

    bool filesAreEqual = CompareFiles("temp\file.ext", "path\to\test\data\case1out.ext");
    Assert.IsTrue(filesAreEqual);
}

Once you've got enough of these high level tests written to capture the apps normal operation and most common error cases the amount of time you'll need to spend pounding on the keyboard to try and catch errors from the code doing something other than what you thought it was supposed to do will go down significantly making future refactoring (or even a big rewrite) much easier.

As you're able to expand unit test coverage you can pare down or even retire most of the integration tests. If your app's reading/writing files or accessing a DB, testing those parts in isolation and either mocking them out or having your tests begin by creating the data structures read from the file/database are an obvious place to start. Actually creating that testing infrastructure will take a lot longer than writing a set of quick and dirty tests; and every time you run a 2 minute set of integration tests instead of spending 30 minutes manually testing a fraction of what the integration tests covered you're already making a big win.

share|improve this answer

protected by gnat Jun 7 at 17:38

Thank you for your interest in this question. Because it has attracted low-quality answers, posting an answer now requires 10 reputation on this site.

Would you like to answer one of these unanswered questions instead?

Not the answer you're looking for? Browse other questions tagged or ask your own question.