0

I am regularly given code with bugs in it and told to fix the bugs without introducing any more. My approach is usually as follows:

  1. Identify the bug's issue, e.g. method is returning an empty list and shouldn't be.
  2. Write a test that verifies that the returned list is of size==1.
  3. Run the test, watching it fail.
  4. Write as little code as possible to get the test to pass.
  5. Repeat 1-4 so that all known bugs are covered by a test and fixed.
  6. With a green test suite, have the confidence to only then refactor some code to improve it.

Would this be the best approach? I think my goal should be 1. fix the bug, and 2. leave the code a bit better off than when I found it.

TheCoder
  • 179
  • 2
  • Possible duplicate of [What is the most effective way to add functionality to unfamiliar, structurally unsound code?](http://softwareengineering.stackexchange.com/questions/135311/what-is-the-most-effective-way-to-add-functionality-to-unfamiliar-structurally) – gnat Feb 28 '17 at 20:42
  • 1
    You are simply describing standard [TDD](https://en.wikipedia.org/wiki/Test-driven_development). So are you really asking us if TDD is a good method? Though most experts here (including me) will probably say yes, you will find others who do not like TDD and say no. In the end, this is mostly an opinion. – Doc Brown Feb 28 '17 at 21:24

3 Answers3

4

While this method is better than just starting to code, it does not at all prevent you from breaking existing functionality. And that's normally the greatest danger when fixing a bug.

If you really want to be safe, there is no way around adding all tests required for the module you are working on. However it is very unlikely that you have the resources for doing so. Therefore some kind of compromise is required. There is no simple recipe. The less you understand the existing code, the more complex it is and the higher the quality standards to be fulfilled, the more effort is required to implement tests.

Aside from that, I would try to do some refactoring of the existing code before modifying its functionality.

Frank Puffer
  • 6,411
  • 5
  • 21
  • 38
  • 3
    Adding all tests required usually isn't an option. Understanding the code before you fix the bug usually is an option. You might want to mention this, as regression bugs occur when someone does not understand the code they are working on. – Frank Hileman Feb 28 '17 at 19:34
0

It depends on the type of code.

If the function is tightly scoped and has no side effects, you can write comprehensive tests for just that function and be confident you haven't broken anything.

If the function accesses global variables or otherwise changes state, then it is much more difficult. You will either need to carefully review the code to determine what could be impacted. You can also use tools to map dependencies to get a visual view of how your changes may propagate logic changes throughout the code base. You may then have to test functions that would otherwise seem unrelated to your changes.

John Wu
  • 26,032
  • 10
  • 63
  • 84
0

Yes, this is a great approach when fixing bugs in maintenance mode. That way, you at least can demonstrate that the bug was fixed, and end up with a “free” regression test – as long as you have this test, you'll never have to fix the same bug twice.

Writing a suitable test can however get quite difficult when the system wasn't designed for testability, e.g. if dependency injection wasn't used. This doesn't necessarily make tests impossible, it just makes them more expensive and more fragile. In such cases, it might be beneficial to do a very careful refactoring to introduce seams into the design at which you can apply your tests.

In the absence of a complete test suite, good documentation, and good domain knowledge, it is impossible to ensure that your bugfix doesn't break something else. It's good to be aware of this risk, but it can't really be mitigated. Since you are aware of this risk, you will try to confine your bug fixes to the smallest possible area of code. Trying to leave the code better than you've found it is a noble cause, but it's risky if you don't fully understand all implications of this code.

Ideally, you would also run the fixed version alongside the broken version in order to detect changes in the output or behaviour that you didn't intend, but that's probably not feasible for complex systems.

It's also worth pointing out that bugs tend to cluster. If you have the time, ask: why was this bug introduced? What is the root cause? Might it have caused similar problems around here?

With sufficient understanding, it can be better to undertake a more complete refactoring of a module. E.g. one time I was bringing a small parsing function under test. The root cause wasn't that the parsing algorithm was flawed, but the data model of the output. Now that I understood how not to represent this kind of data, I could completely rewrite that part of the code and eliminate a whole class of possible problems. Another time, it turned out that a very faulty module was only present for historical reasons, and wasn't really used any more. Deleting it was far more productive than trying to correct each individual problem.

amon
  • 132,749
  • 27
  • 279
  • 375