Confident Refactoring

Refactoring is something every programmer claims to do, but how often do we mistake a rewrite for a refactor?

Or are we too scared to even start refactoring for fear of breaking stuff. How do you overcome refactophobia?

By applying a surgical approach to refactoring, we can safely and confidently make changes to improve our codebases

Transcript

1. Confident Refactoring Matt Swanson Sunday, January 20, 13
2. Refactoring is ubiquitous Sunday, January 20, 13
3. “Refactoring is the process of changing a software system in such a way that it does not alter the external behavior of the code yet improves its internal structure.” Martin Fowler, Refactoring Sunday, January 20, 13
4. “This code is awful — I’m rewriting it. I’ll be back in a week with a version that doesn’t suck.” A foolhardy programmer Sunday, January 20, 13
5. Refactoring Rewriting Sunday, January 20, 13
6. “The person that wrote this doesn’t even work here anymore. There are no tests and I have no idea what it’s even supposed to be doing. I’m afraid to even touch the code.” A fearful programmer Sunday, January 20, 13
7. Refactoring Refactophobia Sunday, January 20, 13
8. Surgical Approach Post-Op Check Diagnose Operate Sunday, January 20, 13
9. Let’s talk about some legacy code Sunday, January 20, 13
10. Flight Processor (ala Black Box) Read some data, do some work EHMS, ETS Get up to speed Sunday, January 20, 13
11. “We need to add a new rule to the processor. If we create a new aircraft, we want to infer some details from the filename.” Your assignment Sunday, January 20, 13
12. VerifyAircraft SerialNumber Decode Unzip File Fun Fact: there are over 35 commands in total! Sunday, January 20, 13
13. Sunday, January 20, 13
14. Doesn’t work here anymore Sunday, January 20, 13
15. Doesn’t work here anymore Me...as an intern... Sunday, January 20, 13
16. Doesn’t work here anymore Me...as an intern... My boss Sunday, January 20, 13
17. Doesn’t work here anymore Me...as an intern... My boss On different project now Sunday, January 20, 13
18. Doesn’t work here anymore Me...as an intern... My boss On different project now Intern...doesn’t work here Sunday, January 20, 13
19. Sunday, January 20, 13
20. Sunday, January 20, 13
21. using SEP.EHMS.EHMSDataProviders.Services; namespace SEP.EHMS.FlightProcessingService.Commands.RawEngineDataCommands { [FileProcessingStep( CurrentStep = EngineFileProcessingSteps.ValidateAc, NextStepOnFailure = EngineFileProcessingSteps.End, NextStepOnSuccess = EngineFileProcessingSteps.DecodeFile)] public class ValidateAcSnCommand : ProcessingCommandBase<EhmsRawEngineDataFile> { private EhmsProcessedDataFile _dataFile; private IEhmsAircraftDataProvider _provider; private IEhmsPartDataProvider _partProvider; private const int ValidAcsnLengthMax = 20; public IEhmsAircraftDataProvider Provider { get { return _provider ?? (_provider = new EhmsAircraftDataProvider()); } set { _provider = value; } } public IEhmsAircraftDataProvider PartProvider { get { return _partProvider ?? (_partProvider = new EhmsPartDataProvider()); } set { _partProvider = value; } } protected override bool ExecuteInternal(EhmsProcessedDataFile<EhmsRawEngineDataFile> passThr { #if DEBUG Debugger.Log(0, "Test", "Validating ACSN.\n"); #endif _dataFile = passThrough; var acsn = passThrough.FilenameRegexResult.Groups[FileNameSettings.AirCraftSerialNumber] if (acsn.Length > ValidAcsnLengthMax && _dataFile.Id != null) { Sunday, January 20, 13
22. } } //Email notification Stub _dataFile.State = ProcessingStates.ACSNInvalid; _dataFile.Update(); #if DEBUG Debugger.Log(0, "Test", "Invalid ACSN.\n"); #endif return false; } private void StoreAircraftData(EhmsPart result) { _dataFile.Aircraft.Id = result.Id; _dataFile.Aircraft.Owner = result.Owner; _dataFile.Aircraft.EngineProgram = result.EngineProgram; _dataFile.Aircraft.PartVersion = result.PartVersion; _dataFile.Aircraft.SerialNumber = result.SerialNumber; _dataFile.Aircraft.TailNumber = result.TailNumber; _dataFile.Aircraft.Location = result.Location; } public override void HandleError(EhmsProcessedDataFile<EhmsRawEngineDataFile> file) { file.State = ProcessingStates.ACSNUnexpected; file.Update(); } } } Sunday, January 20, 13
23. The code works (because no one has yelled at us about it) There are no tests (and changes need to be made to it) Sunday, January 20, 13
24. Why refactor now? Can’t we just tack it on and then go play ping-pong? Sunday, January 20, 13
25. What is this code doing? Where are the problem areas? Diagnose Sunday, January 20, 13
26. Sunday, January 20, 13
27. using Microsoft.VisualStudio.TestTools.UnitTesting; using SEP.EHMS.Common.EhmsObjects; using SEP.EHMS.FlightProcessingService.Commands.RawEngineDataCommands; namespace SEP.EHMS.FlightProcessingService.Tests.Commands.RawEngineDataCommands { [TestClass] public class ValidateAcSnCommandFixture { [TestMethod] public void ShouldDoSomething() { var command = new TestValidateAcSnCommand(); var file = new EhmsProcessedDataFile<EhmsRawEngineDataFile>(); var result = command.Execute(file); Assert.IsTrue(result, "One small step for MDS..."); } private class TestValidateAcSnCommand : ValidateAcSnCommand { public bool Execute(EhmsProcessedDataFile<EhmsRawEngineDataFile> file) { return ExecuteInternal(file); } } } } Characterization Test Sunday, January 20, 13
28. _dataFile = passThrough; var acsn = passThrough.FilenameRegexResult.Groups[FileNameSettings.AirCraftSeria if (acsn.Length > ValidAcsnLengthMax && _dataFile.Id != null) { ProcessedFilesDataProvider.AddProcessingError(_dataFile.Id.Value, null, null _dataFile.State = ProcessingStates.ACSNInvalid; _dataFile.Update(); return false; } } Test method SEP.EHMS.FlightProcessingService.Tests.Commands.RawEngineDataCommands.Va lidateAcSnCommandFixture.ShouldDoSomething threw exception: System.NullReferenceException: Object reference not set to an instance of an object. at SEP.EHMS.FlightProcessingService.Commands.RawEngineDataCommands.Validate AcSnCommand.ExecuteInternal(EhmsProcessedDataFile`1 passThrough) in ValidateAcSnCommand.cs: line 38 Explore with Failures Sunday, January 20, 13
29. [TestMethod] public void ShouldDoSomething() { var command = new TestValidateAcSnCommand(); var file = new EhmsProcessedDataFile<EhmsRawEngineDataFile>(); file.FilenameRegexResult = Regex.Match("file.123", TestRegex, RegexOptions.IgnoreCase); var result = command.Execute(file); Assert.IsTrue(result, "One small step for MDS..."); } private const string TestRegex = @"(^file.?<ACSN>[0-9A-Za-z\-]+)$"; private class TestValidateAcSnCommand : ValidateAcSnCommand { public bool Execute(EhmsProcessedDataFile<EhmsRawEngineDataFile> file) { return ExecuteInternal(file); } } Sunday, January 20, 13
30. Test method SEP.EHMS.FlightProcessingService.Tests.Commands.RawEngineDataCommands.Va lidateAcSnCommandFixture.ShouldDoSomething threw exception: System.NullReferenceException: Object reference not set to an instance of an object. at SEP.EHMS.FlightProcessingService.Commands.RawEngineDataCommands.Validate AcSnCommand.ExecuteInternal(EhmsProcessedDataFile`1 passThrough) in ValidateAcSnCommand.cs: line 46 _dataFile.Update(); return false; } var userId = _dataFile.DataFile.File.UploadingUser.Id ?? 0; var result = Provider.GetACLocalBySerialNumber(acsn, userId); if (result != null) { Sunday, January 20, 13
31. [TestMethod] public void ShouldDoSomething() { var command = new TestValidateAcSnCommand(); var file = new EhmsProcessedDataFile<EhmsRawEngineDataFile>(); file.FilenameRegexResult = Regex.Match("file.123", TestRegex, RegexOptions.IgnoreCase); file.DataFile = new EhmsRawEngineDataFile { File = new EhmsFile { UploadingUser = new EtsUser {Id = -1} } }; var result = command.Execute(file); Assert.IsTrue(result, "One small step for MDS..."); } private const string TestRegex = @"(^file.?<ACSN>[0-9A-Za-z\-]+)$"; private class TestValidateAcSnCommand : ValidateAcSnCommand { public bool Execute(EhmsProcessedDataFile<EhmsRawEngineDataFile> file) { return ExecuteInternal(file); } } Sunday, January 20, 13
32. Test method SEP.EHMS.FlightProcessingService.Tests.Commands.RawEngineDataCommands.Va lidateAcSnCommandFixture.ShouldDoSomething threw exception: System.NullReferenceException: Object reference not set to an instance of an object. at SEP.EHMS.FlightProcessingService.Commands.RawEngineDataCommands.Validate AcSnCommand.ExecuteInternal(EhmsProcessedDataFile`1 passThrough) in ValidateAcSnCommand.cs: line 48 var userId = _dataFile.DataFile.File.UploadingUser.Id ?? 0; var result = Provider.GetACLocalBySerialNumber(acsn, userId); if (result != null) { StoreAircraftData(result); ... // Meanwhile at the top of the file... public IEhmsAircraftDataProvider Provider { get { return _provider ?? (_provider = new EhmsAircraftDataProvider()); } set { _provider = value; } } Sunday, January 20, 13
33. [TestMethod] public void ShouldDoSomething() { var provider = MockRepository.GenerateStub<IEhmsAircraftDataProvider>(); provider.Stub(p => p.GetACLocalBySerialNumber(Arg<string>.Is.Anything, Arg<long>.Is.Anything)) .Return(null); var command = new TestValidateAcSnCommand {Provider = provider}; var file = new EhmsProcessedDataFile<EhmsRawEngineDataFile>(); file.FilenameRegexResult = Regex.Match("file.123", TestRegex, RegexOptions.Ignor file.DataFile = new EhmsRawEngineDataFile { File = new EhmsFile { UploadingUser = new EtsUser {Id = -1} } }; var result = command.Execute(file); Assert.IsTrue(result, "One small step for MDS..."); } Sunday, January 20, 13
34. Test method SEP.EHMS.FlightProcessingService.Tests.Commands.RawEngineDataCommands.Va lidateAcSnCommandFixture.ShouldDoSomething threw exception: System.NullReferenceException: Object reference not set to an instance of an object. at SEP.EHMS.FlightProcessingService.Commands.RawEngineDataCommands.Validate AcSnCommand.ExecuteInternal(EhmsProcessedDataFile`1 passThrough) in ValidateAcSnCommand.cs: line 60 _dataFile.Update(); return true; } if (!PartProvider.DoesExist(acsn,Provider.GetAircraftPartTypeId())) { long engineProgramId = _dataFile.DataFile.EngineProgram.Id ?? 0; Sunday, January 20, 13
35. [TestMethod] public void ShouldDoSomething() { var provider = MockRepository.GenerateStub<IEhmsAircraftDataProvider>(); provider.Stub(p => p.GetACLocalBySerialNumber(Arg<string>.Is.Anything, Arg<l .Return(null); var partProvider = MockRepository.GenerateStub<IEhmsPartDataProvider>(); partProvider.Stub(p => p.DoesExist(Arg<string>.Is.Anything, Arg<long>.Is.Any .Return(true); var command = new TestValidateAcSnCommand {Provider = provider, PartProvider var file = new EhmsProcessedDataFile<EhmsRawEngineDataFile>(); file.FilenameRegexResult = Regex.Match("file.123", TestRegex, RegexOptions.I file.DataFile = new EhmsRawEngineDataFile { File = new EhmsFile { UploadingUser = new EtsUser {Id = -1} } }; var result = command.Execute(file); Assert.IsTrue(result, "One small step for MDS..."); } Sunday, January 20, 13
36. Assert.IsTrue failed. One small step for MDS... Sunday, January 20, 13
37. [TestMethod] public void ShouldDoSomething() { var provider = MockRepository.GenerateStub<IEhmsAircraftDataProvider>(); provider.Stub(p => p.GetACLocalBySerialNumber(Arg<string>.Is.Anything, Arg<long>.Is.Anything)) .Return(null); var partProvider = MockRepository.GenerateStub<IEhmsPartDataProvider>(); partProvider.Stub(p => p.DoesExist(Arg<string>.Is.Anything, Arg<long>.Is.Anything)) .Return(true); var command = new TestValidateAcSnCommand {Provider = provider, PartProvider = p var file = new EhmsProcessedDataFile<EhmsRawEngineDataFile>(); file.FilenameRegexResult = Regex.Match("file.123", TestRegex, RegexOptions.IgnoreCase); file.DataFile = new EhmsRawEngineDataFile { File = new EhmsFile { UploadingUser = new EtsUser {Id = -1} } }; var result = command.Execute(file); Assert.IsFalse(result, "One small step for MDS..."); } private const string TestRegex = @"(^file.?<ACSN>[0-9A-Za-z\-]+)$"; Sunday, January 20, 13
38. [TestMethod] public void ShouldDoSomething() { var provider = MockRepository.GenerateStub<IEhmsAircraftDataProvider>(); provider.Stub(p => p.GetACLocalBySerialNumber(Arg<string>.Is.Anything, Arg<long>.Is.Anything)) .Return(null); var partProvider = MockRepository.GenerateStub<IEhmsPartDataProvider>(); partProvider.Stub(p => p.DoesExist(Arg<string>.Is.Anything, Arg<long>.Is.Anything)) .Return(true); var command = new TestValidateAcSnCommand {Provider = provider, PartProvider = p var file = new EhmsProcessedDataFile<EhmsRawEngineDataFile>(); file.FilenameRegexResult = Regex.Match("file.123", TestRegex, RegexOptions.IgnoreCase); file.DataFile = new EhmsRawEngineDataFile { File = new EhmsFile { UploadingUser = new EtsUser {Id = -1} } }; var result = command.Execute(file); Assert.IsFalse(result, "One small step for MDS..."); } private const string TestRegex = @"(^file.?<ACSN>[0-9A-Za-z\-]+)$"; Sunday, January 20, 13
39. ShouldFailProcessingWhenAircraftSerialNumberIn FileNameIsLessThanMaxLength AndAircraftDoesNotExistInEhms GivenThisUsersVisibility AndDoesNotNeedToBeCreated Descriptive Test Names Sunday, January 20, 13
40. [TestInitialize] public void TestInitialize() { _provider = MockRepository.GenerateStub<IEhmsAircraftDataProvider>(); _partProvider = MockRepository.GenerateStub<IEhmsPartDataProvider>(); _file = new EhmsProcessedDataFile<EhmsRawEngineDataFile>(); ... more _file stuff... } private static void AssertProcessingFailed(bool result, EhmsProcessedDataFile file) { Assert.AreEqual(false, result); Assert.AreEqual(ProcessingStates.ACSNInvalid, file.State); } Readable tests Sunday, January 20, 13
41. var acsn = //SOME JACKED UP REGEX STUFF if (acsn.Length > ValidAcsnLengthMax && _dataFile.Id != null) { ProcessedFilesDataProvider.AddProcessingError(_dataFile.Id.Value, null, null, nu _dataFile.State = ProcessingStates.ACSNInvalid; _dataFile.Update(); return false; } Explore Conditionals Sunday, January 20, 13
42. [TestMethod] public void ShouldFailProcessingWhenAircraftSerialNumberInFileNameIsTooLong() { var command = new TestValidateAcSnCommand { Provider = _provider }; _file.FilenameRegexResult = Regex.Match("file.123567890123456789012", TestRegex, RegexOptions.IgnoreCase); _file.Id = 123; var result = command.Execute(_file); AssertProcessingFailed(result, _file); } Explore Conditionals Sunday, January 20, 13
43. var acsn = //SOME JACKED UP REGEX STUFF if (acsn.Length > ValidAcsnLengthMax && _dataFile.Id != null) { ProcessedFilesDataProvider.AddProcessingError(_dataFile.Id.Value, null, null, nu _dataFile.State = ProcessingStates.ACSNInvalid; _dataFile.Update(); return false; } Are processing errors important? Update hits the database :( Sunday, January 20, 13
44. { Provider = _provider, PartProvider = _partProvider, ProcessedFilesDataProvider = processedFileDataProvider }; _file.FilenameRegexResult = Regex.Match("file.123567890123456789012", TestRegex, _file.Id = 123; processedFileDataProvider.Expect(p => p.AddProcessingError(1, 1, "", 1, "", 1)) .IgnoreArguments(); var result = command.Execute(_file); processedFileDataProvider.VerifyAllExpectations(); Assert.IsTrue(_file.UpdateWasCalled, "Update was not called on the file."); AssertProcessingFailed(result, _file); } private class FakeEhmsFile : EhmsProcessedDataFile<EhmsRawEngineDataFile> { public bool UpdateWasCalled { get; set; } public override void Update() { UpdateWasCalled = true; } } Sunday, January 20, 13
45. var command = new TestValidateAcSnCommand { Provider = _provider, PartProvider = _partProvider, ProcessedFilesDataProvider = processedFileDataProvider }; _file.FilenameRegexResult = Regex.Match("file.123567890123456789012", TestRegex, _file.Id = 123; processedFileDataProvider.Expect(p => p.AddProcessingError(1, 1, "", 1, "", 1)) .IgnoreArguments(); var result = command.Execute(_file); processedFileDataProvider.VerifyAllExpectations(); Assert.IsTrue(_file.UpdateWasCalled, "Update was not called on the file."); AssertProcessingFailed(result, _file); } private class FakeEhmsFile : EhmsProcessedDataFile<EhmsRawEngineDataFile> { public bool UpdateWasCalled { get; set; } public override void Update() { UpdateWasCalled = true; } } Sunday, January 20, 13
46. var command = new TestValidateAcSnCommand { Provider = _provider, PartProvider = _partProvider, ProcessedFilesDataProvider = processedFileDataProvider }; _file.FilenameRegexResult = Regex.Match("file.123567890123456789012", TestRegex, _file.Id = 123; processedFileDataProvider.Expect(p => p.AddProcessingError(1, 1, "", 1, "", 1)) .IgnoreArguments(); var result = command.Execute(_file); processedFileDataProvider.VerifyAllExpectations(); Assert.IsTrue(_file.UpdateWasCalled, "Update was not called on the file."); AssertProcessingFailed(result, _file); } private class FakeEhmsFile : EhmsProcessedDataFile<EhmsRawEngineDataFile> { public bool UpdateWasCalled { get; set; } public override void Update() { UpdateWasCalled = true; } } Repeat the same process for the rest of the conditionals in the code Sunday, January 20, 13
47. [TestMethod] public void ShouldPassProcessingWhenAircraftExistsInEhms() { var fakePart = BuildFakePart(); _provider.Stub(p => p.GetACLocalBySerialNumber(Arg<string>.Is.Anything, Arg<long .Return(fakePart); var result = _command.Execute(_file); AssertProcessingPassed(result, _file); AssertAircraftDataStoredOnFile(_file, fakePart); } [TestMethod] public void ShouldFailProcessingWhenAircraftDoesNotExistGivenThisUsersVisibilityAndC { _provider.Stub(p => p.GetACLocalBySerialNumber(Arg<string>.Is.Anything, Arg<long .Return(null); _partProvider.Stub(p => p.DoesExist(Arg<string>.Is.Anything, Arg<long>.Is.Anythi .Return(false); _provider.Stub( p => p.AttemptRemoteFetchOrCreate(Arg<string>.Is.Anything, Arg<long>.Is.Anyt .Return(null); var result = _command.Execute(_file); AssertProcessingFailed(result, _file); } [TestMethod] Sunday, January 20, 13
48. _provider.Stub( p => p.AttemptRemoteFetchOrCreate(Arg<string>.Is.Anything, Arg<long>.Is.Anyt .Return(null); var result = _command.Execute(_file); AssertProcessingFailed(result, _file); } [TestMethod] public void ShouldFailProcessingWhenAircraftDoesNotExistGivenThisUsersVisibilityAndI { _provider.Stub(p => p.GetACLocalBySerialNumber(Arg<string>.Is.Anything, Arg<long .Return(null); _partProvider.Stub(p => p.DoesExist(Arg<string>.Is.Anything, Arg<long>.Is.Anythi .Return(false); var fakePart = BuildFakePart(); _provider.Stub( p => p.AttemptRemoteFetchOrCreate(Arg<string>.Is.Anything, Arg<long>.Is.Anyt .Return(fakePart); var result = _command.Execute(_file); AssertProcessingPassed(result, _file); AssertAircraftDataStoredOnFile(_file, fakePart); } Sunday, January 20, 13
49. _provider.Stub( p => p.AttemptRemoteFetchOrCreate(Arg<string>.Is.Anything, Arg<long>.Is.Anyt .Return(null); var result = _command.Execute(_file); AssertProcessingFailed(result, _file); } [TestMethod] public void ShouldFailProcessingWhenAircraftDoesNotExistGivenThisUsersVisibilityAndI { _provider.Stub(p => p.GetACLocalBySerialNumber(Arg<string>.Is.Anything, Arg<long .Return(null); _partProvider.Stub(p => p.DoesExist(Arg<string>.Is.Anything, Arg<long>.Is.Anythi .Return(false); var fakePart = BuildFakePart(); _provider.Stub( p => p.AttemptRemoteFetchOrCreate(Arg<string>.Is.Anything, Arg<long>.Is.Anyt .Return(fakePart); var result = _command.Execute(_file); AssertProcessingPassed(result, _file); AssertAircraftDataStoredOnFile(_file, fakePart); } Sunday, January 20, 13
50. Tools to make changes Tests to make them safely Operate Sunday, January 20, 13
51. } //Email notification Stub _dataFile.State = ProcessingStates.ACSNInvalid; _dataFile.Update(); Kill the cruft Sunday, January 20, 13
52. #if DEBUG Debugger.Log(0, "Test", "Validating ACSN.\n"); #endif Trace.WriteLine("Validating ACSN.\n"); Kill the cruft Sunday, January 20, 13
53. protected bool ExecuteInternal(... passThrough) { _dataFile = passThrough; Kill the cruft Sunday, January 20, 13
54. var acsn = passThrough.FilenameRegexResult .Groups[FileNameSettings.AirCraftSerialNumber] .ToString(); public string ExtractSerialNumberFromFilename() { return FilenameRegexResult.Groups["ACSN"].ToString(); } var acsn = passThrough.ExtractSerialNumberFromFilename(); Property chain to method Sunday, January 20, 13
55. var acsn = passThrough.FilenameRegexResult .Groups[FileNameSettings.AirCraftSerialNumber] .ToString(); public string ExtractSerialNumberFromFilename() { return FilenameRegexResult.Groups["ACSN"].ToString(); } var acsn = passThrough.ExtractSerialNumberFromFilename(); “Omega Mess”-Sandi Metz Property chain to method Sunday, January 20, 13
56. protected override bool ExecuteInternal(EhmsProcessedDataFile<EhmsRawEngineDataFile> { var acsn = dataFile.ExtractSerialNumberFromFilename(); if (acsn.Length > ValidAcsnLengthMax && dataFile.Id != null) { ProcessedFilesDataProvider.AddProcessingError(dataFile.Id.Value, null, null, dataFile.State = ProcessingStates.ACSNInvalid; dataFile.Update(); return false; } var userId = dataFile.DataFile.File.UploadingUser.Id ?? 0; if (FindAircraftInEhms(dataFile, acsn, userId)) return true; if (CheckIfPartShouldBeCreated(dataFile, userId, acsn)) return true; dataFile.State = ProcessingStates.ACSNInvalid; dataFile.Update(); return false; } Extract method Sunday, January 20, 13
57. Extract class var verifyCommand = new VerifyAircraftExistsInEhmsCommand(_provider); if (verifyCommand.Execute(dataFile)) return true; public class VerifyAircraftExistsInEhmsCommand: ProcessingCommandBase<EhmsRawEngineD { public IEhmsAircraftDataProvider Provider { get; set; } public VerifyAircraftExistsInEhmsCommand(IEhmsAircraftDataProvider provider) { Provider = provider; } protected override bool ExecuteInternal(EhmsProcessedDataFile<EhmsRawEngineDataF { var acsn = dataFile.ExtractSerialNumberFromFilename(); var userId = dataFile.DataFile.File.UploadingUser.Id ?? 0; var result = Provider.GetACLocalBySerialNumber(acsn, userId); if (result != null) { StoreAircraftData(dataFile, result); dataFile.State = ProcessingStates.ACSNValidated; dataFile.Update(); return true; Sunday, January 20, 13
58. var checkCommand = new FindOrCreatePartInEhmsCommand(_provider, _partProvider); if (checkCommand.Execute(dataFile)) return true; public class FindOrCreatePartInEhmsCommand : ProcessingCommandBase<EhmsRawEngine { ... SNIP ... protected override bool ExecuteInternal(EhmsProcessedDataFile<EhmsRawEngineD { var acsn = dataFile.ExtractSerialNumberFromFilename(); var userId = dataFile.DataFile.File.UploadingUser.Id ?? 0; if (!PartProvider.DoesExist(acsn, Provider.GetAircraftPartTypeId())) { long engineProgramId = dataFile.DataFile.EngineProgram.Id ?? 0; var result = Provider.AttemptRemoteFetchOrCreate(acsn, userId, engin if (result != null) { StoreAircraftData(dataFile, result); dataFile.State = ProcessingStates.ACSNValidated; dataFile.Update(); return true; } } Extract class Sunday, January 20, 13
59. [TestClass] public class VerifyAircraftExistsInEhmsCommandFixture { } [TestClass] public class FindOrCreatePartInEhmsCommandFixture { } Extract tests Sunday, January 20, 13
60. [TestClass] public class VerifyAircraftExistsInEhmsCommandFixture { [TestMethod] public void ShouldFailProcessingWhenAircraftDoesNotExist() {} [TestMethod] public void ShouldPassProcessingWhenAircraftExists() {} } [TestClass] public class FindOrCreatePartInEhmsCommandFixture { [TestMethod] public void ShouldFailProcessingWhenAircraftDoesNotExistGiven ThisUsersVisibilityAndCannotBeFoundOrCreated() {} [TestMethod] public void ShouldPassProcessingWhenAircraftDoesNotExistGiven ThisUsersVisibilityIsFoundOrCreated() {} } Sunday, January 20, 13
61. [TestClass] public class VerifyAircraftExistsInEhmsCommandFixture { [TestMethod] public void ShouldFailProcessingWhenAircraftDoesNotExist() {} [TestMethod] public void ShouldPassProcessingWhenAircraftExists() {} } [TestClass] public class FindOrCreatePartInEhmsCommandFixture { [TestMethod] public void ShouldFailProcessingWhenAircraftDoesNotExistGiven ThisUsersVisibilityAndCannotBeFoundOrCreated() {} [TestMethod] public void ShouldPassProcessingWhenAircraftDoesNotExistGiven ThisUsersVisibilityIsFoundOrCreated() {} } Sunday, January 20, 13
62. var verifyCommand = new VerifyAircraftExistsInEhmsCommand(_provider); if (verifyCommand.Execute(dataFile)) return true; var checkCommand = new FindOrCreatePartInEhmsCommand(_provider, _partProvider); if (checkCommand.Execute(dataFile)) return true; dataFile.State = ProcessingStates.ACSNInvalid; dataFile.Update(); return false; [FileProcessingStep( CurrentStep = EngineFileProcessingSteps.ValidateAc, NextStepOnFailure = EngineFileProcessingSteps.End, NextStepOnSuccess = EngineFileProcessingSteps.DecodeFile)] Refactor to pattern Sunday, January 20, 13
63. Refactor to pattern [FileProcessingStep( CurrentStep = EngineFileProcessingSteps.ValidateAc, NextStepOnFailure = EngineFileProcessingSteps.End, NextStepOnSuccess = EngineFileProcessingSteps.VerifyAircraftExistsInEhmsCommand)] [FileProcessingStep( CurrentStep = EngineFileProcessingSteps.VerifyAircraftExistsInEhmsCommand, NextStepOnFailure = EngineFileProcessingSteps.End, NextStepOnSuccess = EngineFileProcessingSteps.FindOrCreatePartInEhmsCommand)] [FileProcessingStep( CurrentStep = EngineFileProcessingSteps.FindOrCreatePartInEhmsCommand, NextStepOnFailure = EngineFileProcessingSteps.End, NextStepOnSuccess = EngineFileProcessingSteps.DecodeFile)] Sunday, January 20, 13
64. Refactor to pattern [FileProcessingStep( CurrentStep = EngineFileProcessingSteps.ValidateAc, NextStepOnFailure = EngineFileProcessingSteps.End, NextStepOnSuccess = EngineFileProcessingSteps.VerifyAircraftExistsInEhmsCommand)] [FileProcessingStep( CurrentStep = EngineFileProcessingSteps.VerifyAircraftExistsInEhmsCommand, NextStepOnFailure = EngineFileProcessingSteps.End, NextStepOnSuccess = EngineFileProcessingSteps.FindOrCreatePartInEhmsCommand)] [FileProcessingStep( CurrentStep = EngineFileProcessingSteps.FindOrCreatePartInEhmsCommand, NextStepOnFailure = EngineFileProcessingSteps.End, NextStepOnSuccess = EngineFileProcessingSteps.DecodeFile)] Integration tests will break if we somehow messed this up. Like I did when I first did this refactor :( Sunday, January 20, 13
65. Run your tests one last time Evaluate your work Post-Op Check Sunday, January 20, 13
66. namespace SEP.EHMS.FlightProcessingService.Commands.RawEngineDataCommands { [FileProcessingStep( CurrentStep = EngineFileProcessingSteps.ValidateAc, NextStepOnFailure = EngineFileProcessingSteps.End, NextStepOnSuccess = EngineFileProcessingSteps.VerifyAircraftExistsInEhmsComma public class ValidateAircraftSerialNumberLengthCommand : ProcessingCommandBase<E { private const int ValidAcsnLengthMax = 20; protected override bool ExecuteInternal(EhmsProcessedDataFile<EhmsRawEngineD { var acsn = dataFile.ExtractSerialNumberFromFilename(); if (acsn.Length > ValidAcsnLengthMax && dataFile.Id != null) { ProcessedFilesDataProvider.AddProcessingError(dataFile.Id.Value, nul dataFile.State = ProcessingStates.ACSNInvalid; dataFile.Update(); return false; } return true; } public override void HandleError(EhmsProcessedDataFile<EhmsRawEngineDataFile { file.State = ProcessingStates.ACSNUnexpected; file.Update(); } } } Sunday, January 20, 13
67. namespace SEP.EHMS.FlightProcessingService.Commands.RawEngineDataCommands { [FileProcessingStep( CurrentStep = EngineFileProcessingSteps.ValidateAc, NextStepOnFailure = EngineFileProcessingSteps.End, NextStepOnSuccess = EngineFileProcessingSteps.VerifyAircraftExistsInEhmsComma public class ValidateAircraftSerialNumberLengthCommand : ProcessingCommandBase<E { private const int ValidAcsnLengthMax = 20; protected override bool ExecuteInternal(EhmsProcessedDataFile<EhmsRawEngineD { var acsn = dataFile.ExtractSerialNumberFromFilename(); if (acsn.Length > ValidAcsnLengthMax && dataFile.Id != null) { ProcessedFilesDataProvider.AddProcessingError(dataFile.Id.Value, nul dataFile.State = ProcessingStates.ACSNInvalid; dataFile.Update(); return false; } return true; } public override void HandleError(EhmsProcessedDataFile<EhmsRawEngineDataFile { file.State = ProcessingStates.ACSNUnexpected; file.Update(); } } } Sunday, January 20, 13
68. [TestMethod] public void ShouldPassProcessingWhenAircraftSerialNumberInFileNameIsLessThanMaxLengt { var file = new FakeEhmsFile {SerialNumberFromFilename = "123"}; var result = _command.Execute(file); AssertProcessingPassed(result, file); } [TestMethod] public void ShouldFailProcessingWhenAircraftSerialNumberInFileNameIsTooLong() { var file = new FakeEhmsFile {SerialNumberFromFilename = new string('5', 21)}; var result = _command.Execute(file); AssertProcessingFailed(result, file); } [TestMethod] public void ShouldAddProcessingErrorWhenAircraftSerialNumberInFileNameIsTooLong() { var file = new FakeEhmsFile { SerialNumberFromFilename = new string('5', 21) }; _processedFileDataProvider.Expect(p => p.AddProcessingError(1, 1, "", 1, "", 1)) .IgnoreArguments(); _command.Execute(file); _processedFileDataProvider.VerifyAllExpectations(); } Sunday, January 20, 13
69. using System.Diagnostics; using SEP.EHMS.Common.DataProviders.Interfaces; using SEP.EHMS.Common.EhmsObjects; using SEP.EHMS.EHMSDataProviders.Services; namespace SEP.EHMS.FlightProcessingService.Commands.RawEngineDataCommands { [FileProcessingStep( CurrentStep = EngineFileProcessingSteps.ValidateAc, NextStepOnFailure = EngineFileProcessingSteps.End, NextStepOnSuccess = EngineFileProcessingSteps.DecodeFile)] public class ValidateAcSnCommand : ProcessingCommandBase<EhmsRawEngineDataFile> { private EhmsProcessedDataFile _dataFile; private IEhmsAircraftDataProvider _provider; private const int ValidAcsnLengthMax = 20; public IEhmsAircraftDataProvider Provider { get { return _provider ?? (_provider = new EhmsAircraftDataProvider()); } set { _provider = value; } } protected override bool ExecuteInternal(EhmsProcessedDataFile<EhmsRawEngineDataFile> passThr { #if DEBUG Debugger.Log(0, "Test", "Validating ACSN.\n"); #endif _dataFile = passThrough; var acsn = passThrough.FilenameRegexResult.Groups[FileNameSettings.AirCraftSerialNumber] if (acsn.Length > ValidAcsnLengthMax && _dataFile.Id != null) { ProcessedFilesDataProvider.AddProcessingError(_dataFile.Id.Value, null, null, null, _dataFile.State = ProcessingStates.ACSNInvalid; _dataFile.Update(); Sunday, January 20, 13
70. //Email notification Stub _dataFile.State = ProcessingStates.ACSNInvalid; _dataFile.Update(); #if DEBUG Debugger.Log(0, "Test", "Invalid ACSN.\n"); #endif return false; } private void StoreAircraftData(EhmsPart result) { _dataFile.Aircraft.Id = result.Id; _dataFile.Aircraft.Owner = result.Owner; _dataFile.Aircraft.EngineProgram = result.EngineProgram; _dataFile.Aircraft.PartVersion = result.PartVersion; _dataFile.Aircraft.SerialNumber = result.SerialNumber; _dataFile.Aircraft.TailNumber = result.TailNumber; _dataFile.Aircraft.Location = result.Location; } public override void HandleError(EhmsProcessedDataFile<EhmsRawEngineDataFile> file) { file.State = ProcessingStates.ACSNUnexpected; file.Update(); } } } Sunday, January 20, 13
71. We just refactored 4 year old legacy code written by developers that don’t work here anymore that, at first, we didn’t understand and went from zero to 100% test coverage Sunday, January 20, 13
72. Refactoring doesn’t have to be scary or require genius domain experts Sunday, January 20, 13
73. All you need is Patience, Courage, Tests, and a Plan Sunday, January 20, 13
74. Thank you. Let’s have questions. Matt Swanson @_swanson // swanson.github.com Photo credit: http://www.flickr.com/photos/aluc71/2310144939/ Sunday, January 20, 13