独家优惠奖金 100% 高达 1 BTC + 180 免费旋转

Refactoring a legacy app into an exciting opportunity

It is common in software to see a struggle between product managers and developers. Product managers will say “I just want to add X and Y features to the existing application, the client is expecting them soon, so this is the only priority for the project” with developers answering “Existing code is a monstrosity, it will be better to throw it away and develop it with those features in mind from the beginning”. I’ll try to show that the much less treaded path of refactoring is, in many cases, the way forward.

In my experience, product managers tend to win the discussion. The winning argument being that the code won’t be modified again after said features have been implemented, so it is wasteful to add more flexibility or make it more readable. This tends to be false, as new requirements will be found along the way and the customer will require new functionalities, repeating the original discussion with an even worse starting point.

The few times developers win the battle, things are even worse. The code will indeed accommodate the new features like a glove, and the most prominent functionality will be properly handled and tested. Alas, less salient requirements like internationalization or integration to different platforms will remain unnoticed and unprepared for, muddying the envisioned architecture and at times leaving it as bad as the previous iteration.

I tend to argue, and thankfully I am not alone in this, that in most cases the right answer is to refactor the existing code. When refactoring you are not changing the external behavior of the system, so you’ll never end up in a state that does less than before. Also, you don’t need input from management or customers: the requirements are perfectly stated in the current software incarnation. Even if the code does not get extended anymore, as the product owner expected, the new features will already be easier to include and test, reducing the total time for development.

In this article I’ll be sharing my experience when dealing with such a situation, where I defined a process that helped the team understand the vision and systematically improve each of the software’s subsystems. Of course the steps I took will not apply to all or most cases, but will hopefully serve as inspiration when getting into a similar discussion or guidance designing other refactoring plans.

A while ago I was working on a team that was adapting an internal application for a specific customer. For the sake of the article, let’s imagine I was working on a company that produces TV software. Separate teams would produce customized version of the base software for Televisions from Sony or Samsung and even DVD players or Set Top Boxes. This base software was created a long time ago and was not planned to be the future of the company; it had been in the state of adding “only one quick last feature for this specific new customer” for a few years already. No one was comfortable changing any important part of it, and there was no one who would fully understand its inner workings.

The biggest indication that refactoring was absolutely needed is that each customization team would need to make a full copy of the project. It was usually needed to change code all over the place, instead of inside a supposed customer dependent area. This was specially worrying since most of the customer related changes only had to do with a set of subsystems that dealt with the hardware platform: video playback, sound, internet connection…

These customer specific copies were a massive waste of time. If the base software got an update, it was a monumental and error prone task to apply it to all the different versions. Developers working on customer teams were forced to deal with all the code, without proper documentation or stable interfaces. The core team would not know what parts were customized in downstream teams, so they were always stepping on each other’s toes.

I started working in one of such customization teams, suffering all the problems with the software. Even so, it was clear that the application had lots of nice features and clients were usually happy about it.

I became increasingly convinced that clearly separating the customer dependent code and creating some nice interfaces would save months of boring work to all the customer teams (even make them redundant). What is more important, all this could be accomplished in an incremental way, making slow steps towards a seemingly far away goal, getting a better code base after each. In my spare time I did a proof of concept of this idea, proposing a sort of internal SDK.

As soon as I heard the company was setting up a team to improve the code of the base software I jumped at the possibility of joining, bringing the ideas I gathered the months before. I must say I did not come up with a firm plan from the beginning, instead I identified the biggest code smells and set in motion a procedure for fixing them one by one. This was more of an spiritual trip, impossible to fully plan from the get go, but where each step got us into better shape and gave clues about what to do next.

One of the new requirements was to rearrange and improve the TV’s Quick Menu, which included information about the current channel and subtitle and audio options and was shown on top of the video content.

The class QuickMenuScreen looked like the right place to edit this part of the user interface, and its declaration had very few dependencies, with no indication of its real complexity. Once we got our hands dirty with the code, the real fun began. The class contained hundreds of lines of code, most of which were spent setting up the video playback, dealing with subtitle edge cases or with internet connection problems, responsibilities no one would expect from this user interface related class. The code strictly related to drawing the user interface was sprinkled here and there, hard to identify and reason about. Changing any line of the screen class with any level of confidence meant understanding all the edge cases of the other systems touched here.

If any part of the code, in this case QuickMenuScreen wanted to just read the current subtitle menu, it would need a reference to AppManager and from it get the subtitles class. From this point onwards, QuickMenuScreen would have unrestricted access to all the software functionality. On such a messy code base, a developer quickly fixing a bug related with, for example, video playback when showing the Quick Menu would not identify the source of it and fix it where needed, would just introduce a dirty hack in QuickMenuScreen using the already available interfaces. Of course, since the bug was not fixed at its root, it would appear weeks later somewhere else, which would be fixed in a similarly wrong way.

It was our decision to make each module declare the different dependencies it had, instead of just referencing one of the “God classes”. Looking at each class declaration, you should see what other parts it is talking to and what needs to be properly understood before making changes. It would also provide a way of restricting the access to unrelated parts of the system, limiting the introduction of wrong hacks. By the end of this change, thinking about the future, complexity could be managed by slowly removing the declared references in places where they didn’t belong.

The naïve way of accomplishing this is to change all constructors, forcing the callers to pass required objects. This was not really feasible or desirable in our project. Classes were nested too deeply and without much structure, forcing callers to have access to all the modules its children would use making the existing structure harder to change, which is contrary to the purpose.

It is a cumbersome process, where the developers need to change how everything gets accessed from all over the code. Lots of lines were changed when removing each of the modules from the God Class. It was not, however, a risky change; the kind of errors that could be made were automatically spotted by the compiler. Given how systematic this approach is, all the team could collaborate efficiently and introduce the changes in parallel.

At first the team was reluctant to implement this step. It was not immediately obvious how such an abstract idea would affect the real issues we had when coding new features or doing maintenance tasks. However, as more and more classes were moved to the dependency injection system we started to see may improvements:

Now that God Classes were no longer needed for accessing dependencies, it felt appropriate to remove them all together. This was no easy feat, though. Because of the already mentioned problem of everything accessing everything else, the start up process had gotten messier and messier. Services were not initialized atomically, or even from one place, initialization happened from all sorts of unconnected places at unpredictable points in the application start up.

Each of the few God classes started initializing some of their submodules, and then calling another God Class. The ones down under would expect some stuff to be already present, creating dependencies. Since there was no clear reasoning behind what was contained on each God Class, the dependencies created made no sense, they were just based on the existing assumptions. Moreover, the responsibility of a given submodule initialization would be shared across several classes, making the code flow even harder to reason about.

Since we had separate implementations depending on platform/customer, some of those would have new specific dependencies. It was common that those didn’t match the existing flow, forcing all sorts of hacks in the way.

This prevents the software from having good component tests. It was impossible to isolate the video playback functionality, for example, since it was so intertwined with the rest of the software. It also prevented us from having “conformance tests” that customer teams could execute to make sure their custom implementations matched what is expected from them.

The life cycle of the software is a fundamental piece of it, but that does not mean it needs to be complex. In our case, it differed from the rest of the code flow in the sense that until all the subsystems have been created, nothing would be shown on screen and no subsystem would be destroyed until the software started shutting down.

This logical se[aration made it possible to move the start up process to the beginning of the program execution and make it independent from the rest of the business logic and user interface, just a precondition to it. This independence would eventually allow the customer teams to write the code for initializing their subsystems without even looking at the rest of the software.

As opposed to the rest of the code, the new position for the initialization code was final. It was clear there was no need for the flexibility offered by the dependency injection framework, so it was decided to disable it during start up; all dependencies the subsytems had would be passed through constructors. Once created, they would get registered onto the Dependency Injection framework and only by the end of the start up it was allowed to use it.

All basic systems were now independent from the rest of the software. Could be created separately and had a proper interface. However, not all of them made sense. This step requires more thought than the previous two, analyzing what is the purpose of each system and how informative and useful is its interface.

Some of the subsystems were duplicated, segmented or completely useless. In some cases completely new ones were created to replace several half baked elements. Some others were deleted completely. There were at least 4 different ways of communicating across the system, which introduced hard to track dependencies and problems.

A specific source of pain was settings. There were a couple subsystems used for storing settings. On top of that, they contained signals notify others as soon as anything was changed. Lazy developers started using this setting system to communicate across the application.

In the current example, the internet connection subsystem would store connection properties through the IUserSettings module. That information didn’t really fit in the user settings, it was something users didn’t really control or see. When the internet connection was established, its current speed category was stored as a user setting. The video player would register itself as listener to the user settings and get notified when the ConnectionCurSpeed key was updated, and modify the quality of the video stream accordingly.

One by one, we identified this kind of back channels where information was passed, and removed them. For this concrete case, we just needed to expose the information in the IInternetConnection interface in the easiest way to use externally. It would be possible to return the type of connection using an enum, instead of storing an unsafe string as a key in a database.

Developers used to work with this system would not notice how harmful these indirect dependencies used to be. They got accustomed to the existing way of getting this info, and even argued it was better. The info would be saved across reboots and the interfaces would be kept smaller. These arguments were easily disarmed just by looking at existing bugs: it was easy for the info not to be correctly updated, or kept invalid from previous connection, and it was common for the key or values to be mistyped as the compiler would not be able to check it.

Up until this point, making a standalone player using an IVideoPlayer implementation required a fake IUserSettings that would match the existing not documented interfaces. Having this added to the IInternetConnection interface made it much easier to mock, provided an easy to parse definition and allowed the compiler to pick up any typo in the type or return values.

Now there were no more god classes, initialization was independent and easily customizable, and subsystems had clean and meaningful interfaces. We were almost at the end of the road. The biggest of the remaining problems is that the source files were not properly organized, and matched in no way how the code itself was organized.

At the end of this process, we started to really understand how different parts of the code interacted with each other and were at a point that could decide the proper way of organizing the sources. This is not only an aesthetical change, it indeed reduces the cognitive load of the programmer, but on top of that, it allowed makefiles to more accurately define the boundaries of the code they are related to.

As any experienced programmer assumed by this point, the system we were working on contained much more complexity than shown in this text or diagrams. Here only two layers were mentioned: Model and View, classes that provide the basic functionality and classes that make use of it to show the user interface. It is important to have at least one middle layer that would contain the business logic (MVC or MVVM patterns being good examples of this). The boundaries we can now introduce make it possible to implement this separation and be sure it cannot be broken.

After carefully moving the source files around, the possibility of creating a separate library with the base software that could be linked by any of the customers’ teams opened up. Customer teams now knew the places they were allowed to make code changes, and had stable interfaces to adhere to. Tests in the base code wouldn’t need to be repeated since the base library can be provided already compiled, and proper documentation could be shipped alongside.

This was a lengthy process, but it took less than a year to finish, even whilst developing new features at the same time. Developers working for years in the code, and who initially opposed to these changes, got much happier about their jobs, being able to spend their efforts in fixing real issues and adding features instead of dealing with useless integrations and code they didn’t understand.

Starting with a supposedly doomed project that nobody wanted to deal with, after all the described changes this it became an example of how to do good development in the company. Moreover, since the foundations were now so clear, and the adaptations for multiple customers were already in place, it was even considered to base a future development in parts of the refactored software.

Add a comment

What is your view?

Send

Related posts:

Reliability of a TMR system

Reliability is the probability of no failure within a given operating period. The simplest system reliability model is to assume that in a system with n modules, all the modules must work. If the…

Six Ways Management Can Increase Team Member Productivity

According to research by Atlassian, the average employee has over 31 hours of meetings each month. In that study, most employees felt these work meetings were wasted time. Meetings are great if they…

Yous

Different stokes for different folk. “Yous” is a common pronoun in Australia. But it’s rare to hear it applied selectively. Cafe Acropole, Bathurst. These days, there are better places to eat in…