At work, as part of the project I’m on, I’ve been working with a third-party open-source application for the last few months.

It is a Python application that requires customising in order to fit the way it needs to work. Most of the functionality stays the same, except in cases where that functionality needs to be extended or modified to fit with the needs of the application I am working on.

I am fairly new to the team here, so have been going along with their processes for the last six months, but recently I’ve started to become more vocal on changes¹ that would benefit us all.

The problem lies around the maintenance of this app, which I’ll call MapApp as that’s kind of what it is. The development team really do not like to make changes to this MapApp as they believe they will run into trouble when upgrading to a newer version of it later on. Any changes that are made are done so by adding the custom code between comment blocks. Some pretend function examples are shown thusly:

class MapAppThing:
    def getLayer(self, id: int) -> Data:
        data = get_data(id)
        return data

    def updateLayer(self, id: int, update: str) -> None:
        data = self.getLayer(id)
        data.updated_prop = update
        data.save()

It would get called thisly:

component = MapAppThing()
data = component.updateLayer(1, "update text")

If the team decided that, actually, they wanted to update a timestamp property when the data is saved in the updateLayer. The code would be altered to look like this:

class MapAppThing:
    def getLayer(self, id: int) -> Data:
        data = get_data(id)
        return data

    def updateLayer(self, id: int, update: str) -> None:
        data = self.getLayer(id)
        #data.updated_prop = update DEV: No longer updating this
        data.different_prop = update #DEV: Updating this property instead
        data.timestamp = datetime.now() #DEV: Add timestamp
        data.save()

Apparently this is so when we take the next release of the MapApp code it will be easier to see what updates have been made and to merge those in.

I don’t think this is a sensible solution as we have to maintain a custom fork rather than just using the actual released version of MapApp. It doesn’t help with merging either as we still have to go through the aggro of merging in the changes of our custom form on top of the latest version. If we have fundamentally changes a function that has also been significantly altering between releases, that will require a total re-work back into the custom fork.

Imagine having to manually resolve hundreds of merge conflicts every single upgrade which might be as frequent as every few months. It is also a problem that can only become more time consuming every release, as you will always have at least as many conflicts as last time.

Instead I believe that applying some of those popular design patterns that every senior software engineer knows but seem unable to describe or implement² is a better solution. Namely the decorator and the adapter patterns.

I propose we add an adapter class for this MapAppThing, which will be called MapAppThingAdapter³:

class MapAppThingAdapter:
    def __init__(self, adaptee: MapAppThing) -> None:
      self._adaptee = adaptee

    def getLayer(self, id: int) -> Data:
        data = self._adaptee.getLayer(id)
        return data

    def updateLayer(self, id: int, update: str) -> None:
        data = self._adaptee.getLayer(id)
        data.different_prop = update
        data.timestamp = datetime.now()
        data.save()

Now we can use this in code like this:

component = MapAppThing()
adapter = MapAppThingAdapter(component)
data = adapter.updateLayer(1, "update text")

This would mean only two lines needed to be changed, the one where the adapter is initialised, and the one where the adapter is called rather than the original object.

Obviously any references to MapAppThing would now need to use the adapter, but that’s ok, I mean, that’s what was happening with the custom fork anyway. Now it’s just a bit more explicit⁴.

However this should free the developers from the worry of updating this MapApp code, as all the behavioural changes will now take place in an adapter class. In fact, we would know what code has been customised, because it will all be in this adapter layer. If it is not, then it’s the original code.

It would now be possible to swap out MapApp v2.12 (or whatever) with MapApp v3.0⁵, and the only merge conflicts that should arise are where we are plugging our adapters in. Once we have upgraded MapApp we can run their provided suite of units tests which will work, unless their code is poor. Then we can run our adapter unit tests⁶ and see whether any fail. If some do, then we need to fix up our adapters, and we will know which ones are problems and why due to the test outcomes.

Perhaps we prefer the updates that the MapApp community have rolled out as part of their upgrade. Maybe they are adding timestamps to their layer data updates now, and our adapter is no longer required. Well, that’s fine, we can either remove our adapter entirely if it provides no benefit any longer, or just call the original code:

class MapAppThingAdapter:
    def __init__(self, adaptee: MapAppThing) -> None:
      self._adaptee = adaptee

    def getLayer(self, id: int) -> Data:
        data = self._adaptee.getLayer(id)
        return data

    def updateLayer(self, id: int, update: str) -> None:        
        self._adaptee.updateLayer(id, update)

Well, anyway, let’s see how that flies over the next few weeks. My Python isn’t great, and nor is my grasp on these design patterns, so convincing a team that my way is better would be quite the feat. At the very least it shows that I am getting involved in making decisions, even if they are wrong. It took me a long time to work out where I wanted to use a Decorator or an Adapter pattern. I think I am correct that the Adapter is the more suitable one, though I am sure the Decorator would also handle this requirement⁷.

« Back to Posts

17th May 2024