An exercise improving legacy code There are many articles explaining how to make a good design and what rules to follow. In this note we will see a concrete example on how to convert a legacy design into a better one. The problem Many existing systems have coupling problems. Therefore, their maintainability is reduced. Making a change in this type of system brings a large ripple effect. Coupling: The One and Only Software Designing Problem Let's assume we have an existing process. The system applies various algorithms to deduce the of a supervised learning model. hyper-parameters A new requirement is requested: To be able to see, in production, data on the performance of each strategy in real time. Decoupling the system Let’s see the process entry point: StrategySupervisedHelper::getInstance()->optimize($processId); <? … the supervised learning class: { } class StrategySupervisedHelper extends Singleton //.. and the method invoked: ; <? private function optimize ($processId) In the case of a productive system, the first thing we must do is identify its current coverage. The system has a series of automated unit and functional tests. To measure coverage we will use the . Mutation testing technique { ( );
} <? private function optimize ($processId) throw new Exception 'Testing coverage' Unfortunately a single test fails, so we discovered that the process is not covered and we see that the Michael Feathers maxim is sadly applied: “An inherited system is one that has no tests” The strategy to refactor an inherited system is to the existing functionality before making any changes. cover 1—  Creating deferred tests. Writing tests reveals good design interfaces among objects. Due to the current solution and the coupling it has incorporated, it is very difficult to write tests. However, we cannot refactor to write the tests without writing the tests previously. It seems that we are facing a vicious circle. Photo by Justin Chen on Unsplash The possible solution to this deadlock is to write the tests , thus generating better interfaces. declaratively We will run them manually until the coupling is resolved. 2 —  We write tests to cover pre-existing functionality. Tests can be written with a tool from the family with a false assertion (they always fail). xUnit { ->assertTrue( );
} { ->assertTrue( );
}

... function testOptimizationIsGoodEnough () $this false function testOptimizationBelowThreshold () $this false After having covered (for now manually) the necessary cases we can start with the refactor. 3 —  The class name does not represent a real name in the bijection. Helpers do not exist in the real world, nor should they exist in any computable model. Let’s think about the responsibilities to choose the name in . MAPPER What is (wrong with) software? {
} <? class SupervisedLearningAlgorithm extends Singleton For now the name is good enough, and it gives us an idea of the responsibilities of your instances in the real world. 4 —  The class is a singleton. There are no valid reasons to use singletons. This fact, in addition to generating all the problems described here: Singleton Pattern: The Root of All Evil yields a very implemental invocation (coupled to ) and not very declarative... getInstance() SupervisedLearningAlgorithm::getInstance()->optimize($processId); <? which we will change to: ( SupervisedLearningAlgorithm())->optimize($processId); ?> new leaving the class definition as follows: {
	
} <? class SupervisedLearningAlgorithm An important design rule is: Do not subclass concrete classes. If the language allows this, we explicitly declare it: {
} <? final class SupervisedLearningAlgorithm 5 —  The same parameter in all methods. The object is created and then it gets a magic parameter setting the identifier of the process to be optimized. This argument travels by all methods. This is a suggesting us to check the between this parameter and the process. code smell cohesion { {} {} {} } <? final class SupervisedLearningAlgorithm public function calculate ($processId) private function analize ($processId) private function executeAndGetData ($processId, bool $isUsingFastMethod = null) //... etc etc etc Looking at we conclude there can be no algorithm without a process. We don’t want to have a class with to mutate it: bijection setters Nude Models - Part I : Setters Therefore we will pass all the attributes during construction. essential The way to know if an attribute is is to take away all the responsibilities associated with that object. If it can no longer carry out its responsibilities, it is because the attribute belongs to the . essential minimal attribute set { {}
} ?> final class SupervisedLearningAlgorithm public function __construct ($processId) In this way, the strategy is in its essence, with all the benefits it brings us. immutable Is it Crystal Clear for Everybody That a Date Should Not Mutate? 6 —  We find a design pattern. The process, according to bijection, models a real world process. This seems to fit the pattern. Command However, we believe that it is closer to a where there is an ordered sequence of executions, modeling the different steps of an algorithm. method object 7 —  Interchangeable behavior resembles yet another pattern. As the name we assigned to the object according to its responsibilities suggests, this process models an execution strategy that will compete with other polymorphic strategies. This is the intention of the . Strategy pattern Photo by Nicolas Hoizey on Unsplash {		
} ?> final class SupervisedLearningStrategy 8 —  We eliminate nulls. There is never a valid reason to use . does not exist in real life. null Null It violates the principle of and generates coupling between the function caller and the argument. Also, it generates unnecessary as is not polymorphic with any other object. bijection ifs null {

} {

} <? private function executeAndGetData ($processId, $isUsingFastMethod = null) private function executeAndGetData ($processId, bool $isUsingFastMethod = false) We change the absence of the argument to a boolean truth value. NULL: The Billion Dollar Mistake 9 —  We remove the default parameters. The private function in the previous example has a default parameter. The default parameters produce and ripple effect. They are available for the programmer laziness. Since it is a private function the replacement scope is the same class. We make it explicit, replacing all invocations: coupling {

} {

} <? private function executeAndGetData ($processId, bool $isUsingFastMethod = false) private function executeAndGetData ($processId, bool $isUsingFastMethod) 10—  We remove hardcoded constants. { CONFIDENDE_INTERVAL_THRESHOLD= ; { ($estimatedError <= ::CONFIDENDE_INTERVAL_THRESHOLD) {} }
} <? final class SupervisedLearningStrategy const 0.9 private function executeAndGetData ($processId, bool $isUsingFastMethod) //... if self //.. These constants coupled within the code will not allow us to make good tests “manipulating time”. Remember that the tests have to and the time is and to match the tests. be in control of the entire environment global fragile From now on, it will be an essential parameter of object creation (Refactoring by is a safe task, which can be done by any modern IDE. adding parameters 11 — We decouple the log. The log stores relevant information in production about the executions of the strategy. As usual, using a as a global reference. Singleton This new bond prevents us from being able to test it. This is in another module over which we have no control, so we are going to use a wrapping technique. Singleton {
  SingletonLogger::info($infoToLog);
} function logInfo (array $infoToLog) Besides from being a , the log uses class messages. Singleton static SingletonLogger::info($infoToLog); <? Let’s remember that: The only protocol that a class should contain is the one related to its single responsibility (the S for Solid): creating instances. Since the reference is to a static method, we cannot replace the class call with a polymorphic method. Instead, we will use an anonymous function. {
 
  $loggingFunction = {
    SingletonLogger::info($infoToLog);
  };

  $loggingFunction($infoToLog);
} <? function logInfo (array $infoToLog) function () use ($infoToLog) Then, we can decouple the reference to the log and extract it from the class by reducing the coupling, generating better cohesion from the strategy and favoring its testability. { {
  }
} <? final class SupervisedLearningAlgorithm public function __construct ($processId, closure $logging function) With the call from the productive code: $loggingFunction = {
        SingletonLogger::info($infoToLog);
}; SupervisedLearningAlgorithm{$processId, $loggingFunction); <? function () use ($infoToLog) new And the call from the tests: $loggingFunction = { ->loggedData[] = $infoToLog;
}; SupervisedLearningAlgorithm{$processId,  $loggingFunction); ->assertEquals([...], ->loggedData); <? function () use ($infoToLog) $this new $this $this 12 — We reify objects. On the way of our refactoring we find some fixes with persistent data. Such data travel cohesively, so it makes sense to think of it as an object with real-world responsibilities: { [ => ->processId, => Timestamp(), => $runTime, => $isClustering
  ];
} <? private function getDataToPersist ($runTime, $isClustering) return 'processId' $this 'date' new 'runTime' 'isClustering' By creating the new concept, we are in danger of building an . Let’s see what hidden responsibilities you have: anemic model { } ?> final class LearningAlgorithmRunData ////look for responsabilities related to the cohesive data 13 — We complete the coverage. We did not forget to program the tests that we could not write at the beginning. As we have a much less coupled design it is now very easy to do. { ->assertEquals($expected, $real);	
} { ->assertEquals($expected, $real);		
} <? function testOptimizationIsGoodEnough () /// $this function testOptimizationBelowThreshold () /// $this And our system is much less “legacy” compared to when we found it. Photo by Kelly Sikkema on Unsplash Summary After hard iterative and incremental work, through short steps, we have achieved a better solution in the following aspects: Less Coupling. Immutability. Best Names. No Setters / Getters. No Ifs. Without Null. Without Singletons. No default parameters. Better test coverage. Following the Open/Closed principle (Solid’s O) to be able to add new polymorphic algorithms. Following the principle of Single Responsibility (The S for Solid). Without overloading the classes with protocol Photo by Zac Farmer on Unsplash Conclusions Modifying an existing system by improving its design is possible, taking into account clear design rules and taking small steps. We must have professional responsibility and courage to make the relevant changes, leaving a much better solution than when we found it. Part of the objective of this series of articles is to generate spaces for debate and discussion on software design. We look forward to comments and suggestions on this article.

Different

Winner of Noonies 2020 for the award Hacker Noon Contributor of the Year - REFACTORING.

How to Decouple a Legacy System

Too Long; Didn't Read

Company Mentioned

@mcsee

RELATED STORIES