Get to Green: How to safely refactor legacy code

© COPYRIGHT 2019 COVEROS, INC. ALL RIGHTS RESERVED. 1 @CoverosGene
#techbash Agility. Security. Delivered. Get to Green: How to safely refactor legacy code Gene Gotimer [email protected] @CoverosGene @CoverosGene #techbash © COPYRIGHT 2019 COVEROS, INC. ALL RIGHTS RESERVED.

#techbash About Coveros • Coveros helps companies accelerate the delivery of secure, reliable software using agile methods • Agile & DevOps Services • DevOps Implementation • DevSecOps Integration • Agile Transformations & Coaching • Agile Software Development • Agile Testing & Automation • Agile, DevOps, Testing, Security Training • Open Source Products • SecureCI – Secure DevOps toolchain • Selenified – Agile test framework Development Platforms

#techbash Selected Commercial Clients

#techbash Selected Federal Clients

#techbash Refactoring Refactoring is a disciplined technique for restructuring an existing body of code, altering its internal structure without changing its external behavior. Martin Fowler, https://refactoring.com

#techbash Legacy Code Legacy code is simply code without tests. Code without tests is bad code. Michael C. Feathers, Working Effectively with Legacy Code

#techbash Addressing Legacy Code The temptation is to get rid of it and start over from scratch. But even if legacy code is bad code: • it works (maybe, sort of) • it is a known quantity • there may be a reason the solution isn’t as simple as you think it could or should be

#techbash Why Refactor At All? If it works, why change it? Because bad code carries cost and risk. • What if it needs to be updated? • What if it has a security issue? • What if it is incompatible with an update that fixes a security problem? • Reduce technical debt • Be proactive, not reactive. • Don't wait until it has to be changed right now.

#techbash Example Refactored Code public boolean isUserAllowed(User someUser) { boolean isAllowed = false; for (String role : someUser.userRoles()) { if (this.currentService.allowedRoles().contains(role)) { isAllowed = true; break; } } return isAllowed; } public boolean isUserAllowed(User someUser) { return this.currentService.allowedRoles().stream() .anyMatch(someUser.userRoles()::contains); }

#techbash Refactoring

#techbash Catalog of Refactorings • Change Function Declaration • Change Reference to Value • Change Value to Reference • Collapse Hierarchy • Combine Functions into Class • Combine Functions into Transform • Consolidate Conditional Expression • Decompose Conditional • Encapsulate Collection • Encapsulate Record • Encapsulate Variable • Extract Class • Extract Function • Extract Superclass • Extract Variable • Hide Delegate • Inline Class • Inline Function • Inline Variable • Introduce Assertion • Introduce Parameter Object • Introduce Special Case • Move Field • Move Function • Move Statements into Function • Move Statements to Callers • Parameterize Function • Preserve Whole Object • Pull Up Constructor Body • Pull Up Field • Pull Up Method • Push Down Field • Push Down Method • Remove Dead Code • Remove Flag Argument • Remove Middle Man • Remove Setting Method • Remove Subclass • Rename Field • Rename Variable • Replace Command with Function • Replace Conditional with Polymorphism • Replace Constructor with Factory Function • Replace Control Flag with Break • Replace Derived Variable with Query • Replace Error Code with Exception • Replace Exception with Precheck • Replace Function with Command • Replace Inline Code with Function Call • Replace Loop with Pipeline • Replace Magic Literal • Replace Nested Conditional with Guard Clauses • Replace Parameter with Query • Replace Primitive with Object • Replace Query with Parameter • Replace Subclass with Delegate • Replace Superclass with Delegate • Replace Temp with Query • Replace Type Code with Subclasses • Return Modified Value • Separate Query from Modifier • Slide Statements • Split Loop • Split Phase • Split Variable • Substitute Algorithm https://refactoring.com/catalog/

#techbash First Step Whenever I do refactoring, the first step is always the same. I need to build a solid set of tests for that section of code. Martin Fowler, Refactoring: Improving the Design of Existing Code

#techbash Unit Testing Unit testing is a way for developers to document the behavior of code. Unit tests • are automated, • are independent, • usually test individual methods or smaller, • have no external dependencies, and • become our safety net for fearless refactoring.

#techbash Unit Testing Enables Change • The challenge of how to refactor legacy code becomes a challenge of how to unit test legacy code. • Once it has tests, it isn’t legacy code anymore. • We have a safety net. • We can fearlessly refactor. • We can just follow the catalog of refactorings. • And/or we can modify the behavior safely in the future.

#techbash Testing a Private Method private double getPerimeter() { double perimeter = 0.0d; for (double length : lengths) { perimeter += length; } return perimeter; } @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) double getPerimeter() { double perimeter = 0.0d; for (double length : lengths) { perimeter += length; } return perimeter; }

#techbash Key Takeaways • Safety is the goal. Strive for fearless refactoring. • Don't over-complicate the solution. • Unit tests will almost always be the safety net, but it doesn't have to be unit tests. Simple code reviews can provide a safety net.

#techbash Code Coverage • Measures code executed when unit tests run • NOT amount of code tested • Good tool to find untested code • Not covered == not tested • Covered == possibly tested

#techbash Mutation Testing Mutation testing public int foo(int i) { i--; return i; } public int foo(int i) { i++; return i; } public String bar(String s) { if (s == null) { // do something public String bar(String s) { if (s != null) { // do something

#techbash PIT Report Example Light green shows line coverage, dark green shows mutation coverage. Light pink show lack of line coverage, dark pink shows lack of mutation coverage.

#techbash Key Takeaways • Code coverage tells you what code is executed during tests. • Mutation testing tells you what code is tested. • You don't need 100% tested, but at least the pieces that you want to change need a safety net.

#techbash Testing a God Method • God method = “The Method That Does Everything” • For example: 2,000-line Java method, private static void • Nothing but side effects • Modifies parameters • Calls external services • Implements time travel, I assume • Small, incremental changes • Need a safety net • Unit testing will be necessary, but difficult

#techbash “Untestable” Code We want to add tests, but the code isn’t making that easy. • Important methods are void, rely on side effects, etc. • Long methods do too much to comprehend, or require complicated and specific setup before the test can be executed. • Other code or services are invoked from within, so the unit of code can’t be easily isolated.

#techbash Mock Frameworks • Allow you to create objects that have predetermined responses to specific requests. • Rely on interfaces to replace “real” instances. • Replacement for external services in unit tests, • or sometimes when it is just tedious to create a graph of objects. • Dependency injection makes testing with mocks easy. when(mockService.get(anyInt()).thenReturn("foo"); when(mockService.get(-1).thenThrow(new IllegalArgumentException());

#techbash Using mocks public ZapSensor(ZapSensorConfiguration configuration, FileSystem fileSystem, PathResolver pathResolver, Rules rules) { this.rules = rules; this.report = new XmlReportFile(configuration, fileSystem, pathResolver); } @Before public void setUp() { final ZapSensorConfiguration configuration = mock(ZapSensorConfiguration.class); final FileSystem fileSystem = mock(FileSystem.class); final PathResolver pathResolver = mock(PathResolver.class); final Rules rules = mock(Rules.class); this.zapSensor = new ZapSensor(configuration, fileSystem, pathResolver, rules); }

#techbash PowerMock • Useful when you don't have interfaces or dependency injection • Extends Mockito and EasyMock • Uses reflection to mock • constructors • final methods • static methods • private methods • Make PowerMock use temporary. The goal is to use it to refactor so that you do not need it. “Please note that PowerMock is mainly intended for people with expert knowledge in unit testing. Putting it in the hands of junior developers may cause more harm than good.”

#techbash Using PowerMock … void addIssue(SensorContext context, AlertItem alert) { Severity severity = ZapUtils.riskCodeToSonarQubeSeverity(alert.getRiskcode()); … mockStatic(ZapUtils.class); … when(ZapUtils.riskCodeToSonarQubeSeverity(3)) .thenReturn(Severity.CRITICAL);

#techbash Removing PowerMock … void addIssue(SensorContext context, AlertItem alert) { Severity severity = ZapUtils.riskCodeToSonarQubeSeverity(alert.getRiskcode()); … void addIssue(SensorContext context, Severity severity) {

#techbash Removing PowerMock public static Severity riskCodeToSonarQubeSeverity(int riskcode) { if (riskcode == 3) { return Severity.CRITICAL; } else if (riskcode == 2) { return Severity.MAJOR; } else if (riskcode == 1) { return Severity.MINOR; } else { return Severity.INFO; } }

#techbash Removing PowerMock @Deprecated public static Severity riskCodeToSonarQubeSeverity(int riskcode) { return new ZapUtils().riskCodeToSonarQubeSeverity(riskcode); } public Severity riskCodeToSonarQubeSeverity(int riskcode) { if (riskcode == 3) { return Severity.CRITICAL; } else if (riskcode == 2) { return Severity.MAJOR; } else if (riskcode == 1) { return Severity.MINOR; } else { return Severity.INFO; } }

#techbash Key Takeaways • Use mocking frameworks to isolate units from external dependencies. • Mocks can also simplify tests by making it easier to set up objects that you need but aren't directly involved in the test. • Frameworks like PowerMock can be used as temporary solutions. The goal is to use PowerMock to safely refactor until you don't need PowerMock anymore.

#techbash Static Code Analysis • Inspects source code for • code coverage • duplicated code • complex code • tightly coupled code • security issues • Helps identify riskiest code • Refactoring opportunities • Also shows • unused variables • confusing code • best practices • coding standards

#techbash

#techbash IDE Code Inspections • IDEs have some static analysis built-in (red, squiggly lines) • Other static analysis tools can be integrated • e.g., SonarLint • Quicker feedback loop, could show some low-hanging fruit

#techbash IDE Refactoring • IDEs have built-in refactoring support • You still need unit tests as the safety net

#techbash Key Takeaways • Static code analysis tools can identify riskiest code, which are prime candidates for refactoring. • IDEs often have static code analysis built in. • IDEs often have refactoring tools built in. You still need unit tests.

#techbash General Plan of Attack For example, for our 2,000-line Java method, private static void 1. Unit test with mocks to isolate units. 2. Use PowerMock when necessary. 3. Use mutation testing to make sure code being touched is tested. 4. Replace chunks with package-private methods, @VisibleForTesting. 5. Refactor to eliminate the need for PowerMock. 6. Let static analysis tools identify refactoring opportunities. 7. Use IDE tools to perform individual refactorings. One change at a time. Refactor – Test – Repeat.

#techbash #Coveros5 • Safety is the goal. Strive for fearless refactoring. • Unit tests are the best safety net for making code changes. • Use mutation testing to make sure your unit tests are actually covering what you need covered. • Use a combination of mocking tools and mocks to isolate your units to test. • Make small, incremental, safe changes.

#techbash Tools Required • Unit testing framework • Code coverage tool • Mutation testing tool • Mock frameworks • Static analysis tools • IDE refactoring tools • Lots of patience

#techbash Java Tools • JUnit https://junit.org • JaCoCo https://www.eclemma.org/jacoco • PIT http://pitest.org • Mockito https://github.com/mockito/mockito • EasyMock http://easymock.org • PowerMock https://github.com/powermock/powermock • PMD CPD https://pmd.github.io • SonarQube https://www.sonarqube.org

#techbash C#/.NET Tools • NUnit https://nunit.org • NCover (commercial) https://www.ncover.com • OpenCover https://github.com/OpenCover/opencover • Stryker.NET https://stryker-mutator.io • Moq https://github.com/moq/moq4 • Microsoft Fakes https://docs.microsoft.com/en- us/visualstudio/test/isolating-code-under-test-with-microsoft-fakes • PMD CPD https://pmd.github.io • SonarQube https://www.sonarqube.org

#techbash Reading List • Refactoring: Improving the Design of Existing Code, by Martin Fowler https://amzn.com/0134757599 • Working Effectively with Legacy Code, by Michael Feathers https://amzn.com/0131177052 • Clean Code: A Handbook of Agile Software Craftsmanship, by Robert C. Martin https://amzn.com/0132350882 • Pragmatic Unit Testing in Java 8 with JUnit, by Jeff Langr https://amzn.com/1941222595 • The Mikado Method, by Ola Ellnestam and Daniel Brolund https://amzn.com/1617291218

#techbash Questions? https://hub.techwell.com/ @Gene Gotimer [email protected] @CoverosGene

Get to Green: How to safely refactor legacy code

Get to Green: How to safely refactor legacy code

More Decks by Gene Gotimer

Other Decks in Programming

Featured

Transcript