1 / 68

Working Effectively with Legacy Code

Working Effectively with Legacy Code. Michael Feathers mfeathers@objectmentor.com. Background. Want to do Extreme Programming …but transitioning from a dirty code base. if (m_nCriticalError) return; m_nCriticalError=nErrorCode; switch (nEvent) { case FD_READ:

susane
Download Presentation

Working Effectively with Legacy Code

An Image/Link below is provided (as is) to download presentation Download Policy: Content on the Website is provided to you AS IS for your information and personal use and may not be sold / licensed / shared on other websites without getting consent from its author. Content is provided to you AS IS for your information and personal use only. Download presentation by click this link. While downloading, if for some reason you are not able to download a presentation, the publisher may have deleted the file from their server. During download, if you can't get a presentation, the file might be deleted by the publisher.

E N D

Presentation Transcript

  1. Working Effectively with Legacy Code Michael Feathers mfeathers@objectmentor.com

  2. Background • Want to do Extreme Programming …but transitioning from a dirty code base if (m_nCriticalError) return; m_nCriticalError=nErrorCode; switch (nEvent) { case FD_READ: case FD_FORCEREAD: if (GetLayerState()==connecting && !nErrorCode)

  3. First Reaction • Pretend the code isn’t there • Use TDD to create new classes, make direct changes to the old ones, hope that they are correct • We’d actually like to be even more conservative. We’d like to avoid having to go back to that code ever again. • Is this viable?

  4. Let’s Try It

  5. PageGenerator::generate() std::string PageGenerator::generate() { std::vector<Result> results = database.queryResults(beginDate, endDate); std::string pageText; pageText += "<html>"; pageText += "<head>"; pageText += "<title>"; pageText += "Quarterly Report"; pageText += "</title>"; pageText += "</head>"; pageText += "<body>"; pageText += "<h1>"; pageText += "Quarterly Report"; pageText += "</h1><table>"; …

  6. Continued.. … if (results.size() != 0) { pageText += "<table border=\"1\">"; for (std::vector<Result>::iterator it = results.begin(); it != results.end(); ++it) { pageText += "<tr border=\"1\">"; pageText += "<td>" + it->department + "</td>"; pageText += "<td>" + it->manager + "</td>"; char buffer [128]; sprintf(buffer, "<td>$%d</td>", it->netProfit / 100); pageText += std::string(buffer); sprintf(buffer, "<td>$%d</td>", it->operatingExpense / 100); pageText += std::string(buffer); pageText += "</tr>"; } pageText += "</table>"; } else { …

  7. Continued.. … pageText += "<p>No results for this period</p>"; } pageText += "</body>"; pageText += "</html>"; return pageText; } // Argh!!

  8. Changes • Making changes inline makes methods longer • Changes are untested, the code never has a chance to get better • We could add code to new methods, and delegate from here • Upside: • This method won’t get longer • We can make progress • Downsides: • this method won’t be tested • Sometimes, it doesn’t work

  9. Chia Pet Pattern When you have legacy code, try to not to add inline code to it. Instead, write tested code in new classes and methods, and delegate to them. The techniques for doing this are called Sprout Method and Sprout Class

  10. Why are we so Conservative? When we don’t have tests it is easy to make unintentional errors when we change code Most of development is about preserving behavior. If it wasn’t we would be able to go much faster

  11. The Refactoring Dilemma When we refactor, we should have tests. To put tests in place, we often have to refactor

  12. Most applications are glued together • We don’t know this until we try to test pieces in isolation

  13. Kinds of Glue • Singletons (a.k.a. global variables) • If there can only be one of an object you’d better hope that it is good for your tests too • Internal instantiation • When a class creates an instance of a hard coded class, you’d better hope that class runs well in tests • Concrete Dependency • When a class uses a concrete class, you’d better hope that class lets you know what is happening to it

  14. Breaking Dependencies The way that we break dependencies depends upon the tools available to us. If we have safe extract method and safe extract interface in an IDE, We can do a lot If we don’t, we have to refactor manually, and very conservatively. We have to break dependencies as directly and simply as possible. Sometimes breaking dependencies is ugly

  15. ‘The Undetectable Side-Effect’ • An ugly Java class (it’s uglier inside, trust me!)

  16. (told you) public class AccountDetailFrame extends Frame implements ActionListener, WindowListener { private TextField display = new TextField(10); … private AccountDetailFrame(…) { … } public void actionPerformed(ActionEvent event) { String source = (String)event.getActionCommand(); if (source.equals(“project activity”)) { DetailFrame detailDisplay = new DetailFrame(); detailDisplay.setDescription( getDetailText() + “ “ + getProjectText()); detailDisplay.show(); String accountDescription = detailDisplay.getAccountSymbol(); accountDescription += “: “; … display.setText(accountDescription); … } }

  17. Separate a dependency public class AccountDetailFrame extends Frame implements ActionListener, WindowListener { private TextField display = new TextField(10); … private AccountDetailFrame(…) { … } public void actionPerformed(ActionEvent event) { String source = (String)event.getActionCommand(); if (source.equals(“project activity”)) { DetailFrame detailDisplay = new DetailFrame(); detailDisplay.setDescription( getDetailText() + “ “ + getProjectText()); detailDisplay.show(); String accountDescription = detailDisplay.getAccountSymbol(); accountDescription += “: “; … display.setText(accountDescription); … } }

  18. After a method extraction.. public class AccountDetailFrame extends Frame implements ActionListener, WindowListener { … public void actionPerformed(ActionEvent event) { performCommand((String)event.getActionCommand()); } void performCommand(String source); if (source.equals(“project activity”)) { DetailFrame detailDisplay = new DetailFrame(); detailDisplay.setDescription( getDetailText() + “ “ + getProjectText()); detailDisplay.show(); String accountDescription = detailDisplay.getAccountSymbol(); accountDescription += “: “; … display.setText(accountDescription); … } }

  19. More offensive dependencies.. public class AccountDetailFrame extends Frame implements ActionListener, WindowListener { … void performCommand(String source); if (source.equals(“project activity”)) { DetailFrame detailDisplay = new DetailFrame(); detailDisplay.setDescription( getDetailText() + “ “ + getProjectText()); detailDisplay.show(); String accountDescription = detailDisplay.getAccountSymbol(); accountDescription += “: “; … display.setText(accountDescription); … } }

  20. More refactoring.. public class AccountDetailFrame extends Frame implements ActionListener, WindowListener { private TextField display = new TextField(10); private DetailFrame detailDisplay; … void performCommand(String source); if (source.equals(“project activity”)) { setDescription(getDetailText() + “” + getProjectText()); … String accountDescripton = getAccountSymbol(); accountDescription += “: “; … setDisplayText(accountDescription); … } } …

  21. The aftermath.. We can subclass and override getAccountSymbol, setDisplayText, and setDescription to sense our work

  22. A Test.. public void testPerformCommand() { TestingAccountDetailFrame frame = new TestingAccountDetailFrame(); frame.accountSymbol = “SYM”; frame.performCommand(“project activity”); assertEquals(“Magma!”, frame.getDisplayText()); }

  23. Classes are like cells.. Most designs need some mitosis

  24. Making it Better • We can make the design better by extracting classes for those separate responsibilities

  25. Better vs. Best “Best is the enemy of good” – Voltaire (paraphrased)

  26. Automated Tool Moves • If you have a safe refactoring tool and you are going to use it to break dependencies without tests, do a refactoring series: a series of automated refactorings with no other edits. • When tool operations are safe, all that does is move the risk to your edits. Minimize them by leaning on the tool

  27. Manual Moves • Lean on the Compiler • Preserve Signatures • Single Goal Editing

  28. Leaning on the Compiler • You can use the compiler to navigate to needed points of change by deliberately producing errors • Most common case: • Change a declaration • Compile to find the references that need changes

  29. Signature Preservation • When you are trying to get tests in place favor refactorings which allow you to cut/copy and paste signatures without modification • Less error prone

  30. Single Goal Editing • Do one thing at a time

  31. The Legacy Code Change Algorithm • Identify Change Points • Identify Test Points • Break Dependencies • Write Tests • Refactor or Change

  32. Development Speed Mars Spirit Rover “What can you do in 14 minutes?”

  33. Breaking Dependencies 0 Extract Interface Extract Implementor

  34. Extract Interface • A key refactoring for legacy code • Safe • With a few rules in mind you can do this without a chance of breaking your software • Can take a little time

  35. Extract Interface • Interface • A class which contains only pure virtual member-functions • Can safely inherit more than one them to present different faces to users of your class

  36. Extract Interface Steps • Find the member-functions that your class uses from the target class • Think of an interface name for the responsibility of those methods • Verify that no subclass of target class defines those functions non-virtually • Create an empty interface class with that name • Make the target class inherit from the interface class • Replace all target class references in the client class with the name of the interface • Lean on the Compiler to find the methods the interface needs • Copy function signatures for all unfound functions to the new interface. Make them pure virtual

  37. The Non-Virtual Override Problem • In C++, there is a subtle bug that can hit you when you do an extract interface class EventProcessor { public: void handle(Event *event); }; class MultiplexProcessor : public EventProcessor { public: void handle(Event *event); };

  38. The Non-Virtual Override Problem • When you make a function virtual, every function in a subclass with the same signature becomes virtual too • In the code on the previous slide, when someone sends the handle message through an EventProcessor reference to a MultiplexProcessor, EventProcessor’s handle method will be executed • This can happen when people assign derived objects to base objects. Generally this is bad. • Something to watch out for

  39. The Non-Virtual Override Problem • So… • Before you extract an interface, check to see if the subject class has derived classes • Make sure the functions you want to make virtual are not non-virtual in the derived class • If they are introduce a new virtual method and replace calls to use it

  40. Extract Implementor • Same as Extract Interface only we push down instead of pull up.

  41. Breaking Dependencies 1 Parameterize Method Extract and Override Call

  42. Parameterize Method • If a method has a hidden dependency on a class because it instantiates it, make a new method that accepts an object of that class as an argument • Call it from the other method void TestCase::run() { m_result = new TestResult; runTest(m_result); } void TestCase::run() { run(new TestResult); } void TestCase::run( TestResult *result) { m_result = result; runTest(m_result); }

  43. Steps – Parameterize Method • Create a new method with the internally created object as an argument • Copy the code from the original method into the old method, deleting the creation code • Cut the code from the original method and replace it with a call to the new method, using a “new expression” in the argument list

  44. Extract and Override Call • When we have a bad dependency in a method and it is represented as a call. We can extract the call to a method and then override it in a testing subclass.

  45. Steps – Extract and Override Call • Extract a method for the call (remember to preserve signatures) • Make the method virtual • Create a testing subclass that overrides that method

  46. Breaking Dependencies 2 Link-Time Polymorphism

  47. Link-Time Polymorphism void account_deposit(int amount) { struct Call *call = (struct Call *) calloc(1, sizeof (struct Call)); call->type = ACC_DEPOSIT; call->arg0 = amount; append(g_calls, call); }

  48. Steps – Link-Time Polymorphism • Identify the functions or classes you want to fake • Produce alternative definitions for them • Adjust your build so that the alternative definitions are included rather than the production versions.

  49. Breaking Dependencies 3 Expose Static Method

  50. Expose Static Method • Here is a method we have to modify • How do we get it under test if we can’t instantiate the class? class RSCWorkflow { public void validate(Packet& packet) { if (packet.getOriginator() == “MIA” || !packet.hasValidCheckSum()) { throw new InvalidFlowException; } … } }

More Related