I am happy to be part of an amazing team that adopted a series of best practices in the software development.
One best practice that I like the most is “Improve code on the fly.” When we see code that can be improved, we take the opportunity to improve it.
This approach of improving the code quality is known as Robert Martin’s “The Boy Scout Rule” or Martin Fowler’s “Opportunistic Refactoring”. It’s all about being good code stewards, leaving code cleaner than when you found it.
A few months ago one of my colleagues worked on a feature and stumbled across some code that was in pretty bad shape: a lot of code duplication and a few methods that did too much. So we worked together to improve it.
What we started with
In production we have two desktop applications. These desktop applications are made up of controls (buttons, lists, labels, menu etc). Some of the controls (like lists, textboxes etc) support editing like copy/pasting. These controls are always put in a container of controls. So in order to get access to those editable controls, we need to detect the active control — this is the control that has focus, the control that the user works with — within the container. In order to implement the copy/paste functionality, we initially created those duplicated lines, but that makes for poor code.
But before starting to discuss our topic, let’s have a quick look at what we had initially:
private static void SelectAll(Control control) { // Initially here there were about 10 lines of code that determined the active control. // In the production code, the control object passed as parameter is a container of controls. // But since determining the active control is out of scope, in order to keep things simple, we'll ignore those lines. Control activeControl = control; if (activeControl is CertainFrameworkControl) ((CertainFrameworkControl)activeControl).SelectAll(); else if (activeControl is IEditableControl) ((IEditableControl)activeControl).SelectAll(); else if (activeControl is CertainThirdPartyControl) ((CertainThirdPartyControl)activeControl).Select(); }
So we had four methods, all looking the same as above — SelectAll(), Copy(), Paste() and Cut() — multiplying about ten lines of code for detecting the active control and three lines of code to detect the type of the object activeControl. Because we had two applications with the same functionality, these four methods were also duplicated, ending up with about 100 duplicate lines.
While we have two applications in reality, we simplified it to one for how we explained our solution on Github. For this clarity on Github, only the problem and the solution (adapter pattern in real world) is presented. I will explain everything in further detail here now.
Our step-by-step solution to simpler code
So the first step was pretty simple: we had to move the detection of the user’s active control in one method. That leads us to the following intermediary stage (also here on Github):
private static void SelectAll(Control control) { Control activeControl = GetActiveControl(control); if (activeControl is CertainFrameworkControl) ((CertainFrameworkControl)activeControl).SelectAll(); else if (activeControl is IEditableControl) ((IEditableControl)activeControl).SelectAll(); else if (activeControl is CertainThirdPartyControl) ((CertainThirdPartyControl)activeControl).Select(); }
We improved a lot already by getting rid of a lot of duplication. What was left was three checks multiplied four times, once for each method.
Add to this, the methods did too much: they knew about too many objects and they have an unnecessarily high level of cyclomatic complexity, which is a high level of complexity that can keep looping. This complexity makes for a solid refactoring candidate.
But since they don’t share the same interface, was it possible to remove the duplication in a nice way?
How the Adapter Pattern Translates
So here’s the challenge: how do we remove the duplication of the checks by keeping the code easy to read? The best solution that came to mind was to make those three types speak the same language. Basically we wanted to have a single interface to communicate with all three types.
Is there anything out there that can help us with this? The answer is yes, the Adapter Pattern can help us achieve this goal. But what is the adapter pattern? What problems does it try to resolve?
A simple definition we found for the adapter pattern is: “Convert the interface of a class into another interface clients expect. Adapter lets classes work together that couldn’t otherwise because of incompatible interfaces.” It looked like exactly what we were looking for!
But before trying to implement the adapter pattern, let’s have a second look at what we have — CertainFrameworkControl, a CertainThirdPartyControl, and the interface IEditableControl. I like the interface IEditableControl because it has everything we needed, so there was no need to create another interface. Instead, we just tried to adapt the other two types to this interface.
The adapter:
public class CertainThirdPartyControlAdapter : IEditableControl { CertainThirdPartyControl control; public CertainThirdPartyControlAdapter(CertainThirdPartyControl control) { this.control = control; } public void Copy() => control.Copy(); public void Cut() => control.Cut(); public void Paste() => control.Paste(); // Note that we call Select() because this is what CertainThirdPartyControl exposes for this functionality public void SelectAll() => control.Select(); }
In Utils class, implement the method GetControl() to return an IEditableControl:
public static IEditableControl GetControl(Control control) { if (control is IEditableControl) return (IEditableControl)control; else if (control is CertainFrameworkControl) return new CertainFrameworkControlAdapter((CertainFrameworkControl)control); else if (control is CertainThirdPartyControl) return new CertainThirdPartyControlAdapter((CertainThirdPartyControl)control); else return null; }
Instead of returning a Control, now GetActiveControl() returns an IEditableControl:
private static IEditableControl GetActiveControl(Control control) { // Initially here there were about 10 lines of code that determined the active control. // In the production code, the control object passed as parameter is a container of controls. // But since determining the active control is out of scope, in order to keep things simple, we'll ignore those lines. return Utils.GetControl(control); }
The SelectAll() method becomes:
private static void SelectAll(Control control) { IEditableControl activeControl = GetActiveControl(control); activeControl?.SelectAll(); }
Our Conclusions
The Adapter Pattern can help us to build more robust and maintainable code. What we gained with this solution:
- A la Uncle Bob’s Principles, the architecture is more SOLID now.
- The classes and methods are smaller and easier to read.
- It has a lower cyclomatic complexity.
- The code is now less fragile.
- The code is more abstract and therefore easier to extend. Take, for example, the method SelectAll() — the only thing that we know and care about is that we have an active control that knows how to select all its content; we don’t care which is the control that does X and we don’t care how does that control do X.
- If we need to add a new control that doesn’t implement IEditableControl, then we need to do two things:
- implement an adapter for that control
- change the method GetControl() to return that adapter
In the absence of the adapter pattern, we would have to modify the code in four places — all four methods: SelectAll(), Copy(), Paste() and Cut(). In our production code, without all this refactoring we would need to change the code in eight places.
If you forget to change the code in one place and that part of the code is not tested, chances are that you’ll release a bug into production because the code is fragile.
I cannot think about any disadvantage that this particular solution has, but clearly almost everything is debatable especially in software design.
Is it desirable to have so many classes for such a simple problem? Is it desirable to allocate the time for these benefits? My answers for these questions are: Yes, the solution is worth applying for the simple fact that it cost us almost nothing and we gain a lot of benefits from it.
But isn’t the last benefit a theoretical one, the one related to the new control that we might introduce later in the application? Of course, that benefit is a theoretical benefit and we might actually never get the benefit. But even if we don’t get it, we still have the other benefits that justify the changes.
Plus, when we design the software, aren’t some of the benefits always going to be theoretical? Of course they are, yet we still try to stick to the best practices because we know that it’s a matter of probability and the probability to gain benefits from best practices is huge.
TL;DR: The Adapter Pattern helps us to build more maintainable code at a low cost.