Last week I was doing a quick refactoring of our virtualization module at work. We are about to introduce some new concepts in it and a reorganization of the directory structure was due.
However, this being a quite mature project it was started before the dawn of namespaces in PHP and the KVM module was also written in that fashion. As we thrive for perfectness whenever we can, moving the module to namespaces while changing the directory structure was the obvious choice.
Introducing namespacing into a module of about 20 or so classes was not a big deal. Tests covered most of it, find and replace worked like a charm and the PHP analyzer in PHPStorm highlighted a the few spots missed by the former two actions.
In less then a full work day the whole module, at the business logic level, was up and running using namespaces and the new directory structure. Long live TDD and good test coverage, it would have taken several days or maybe weeks to change all that without tests, but that's another story, for another time.
Then my colleague +Vadim Comanescu offered to help with the update of the web interface. As our business logic is totally separated from our user interface, the changes should have been quite localized. And they were, for the most of it. The change went on smoothly, except one little mistake - or laziness for that matter - we made a few months ago.
A rogue method in a view helper implemented something like "isCurrentPathForVM($path)" At first sight it was acceptable. It accessed the virtual machine inventory class Kvm_VmInventory, retrieved all the virtual machines with its "findAll()" method and did a quick foreach() on it asking each virtual machine if it belongs or not to the provided path.
Helper -> Inventory -> Virtual Machine. A simple dependency chain involving only 3 classes. Shouldn't be a problem, right?
Then it struck us! Why modifying the public interface of the Inventory class affects a view helper? Why the internal class structure of the KVM module is leaked to the UI? Why a helper attached to a view must know the working details of an class buried deep inside the module? Don't we have a Facade already written for that?
So many question had been risen in an instance. Clearly something was wrong.
A view helper should use only information available to the view and it should only do operations related to presentation. Yes, it should contain logic, but only presentation logic. Was finding out that the current path belongs or not to a virtual machine presentation logic? It turns out it was not.
To start answering our so many questions we decided the helper must not know about the inner classes of the KVM module. However it needs to ask the module for the information it requires: Does the current path belong or not to a virtual machine. If yes, the helper will instruct the view to draw a little computer instead of the folder icon at the left of the path.
To do so the helper need access to the facade. But should it create an instance of the facade? No, it shouldn't. There is already one available created by the Kvm model. But accessing a model from a view helper is again tricky and wrong. The helper needs to take it's parameters from the view.
The view's parameters are defined by the controller. That's great, we have the missing piece in the chain.
Helper -> View -> Controller -> Model -> Facade -> Inventory -> Virtual Machine. Here is our new dependency chain. Much longer, but also way more decoupled and correct. The model provides the facade to the controller and passes it to the view, subsequently letting the helper ask for the question: is the current path for a virtual machine? Which made us realize a distant code duplication was also present. The whole logic of the original method was already implemented in the facade.
We were pleased to also a remove a code duplication that lingered hidden in the dependency chain.
Finally we moved the logic from the facade to the Inventory. Facade's should have no logic, they should only be a common entry point to the module providing an easy to understand specialized public interface to any client.
All that done, Vadim and I looked at each other, and he said: "There was not one instance when a mistake or laziness did not come and bite us in the butt afterwards!"
We at Syneto always try to make our code better. We are proud of our accomplishments and we are not afraid to acknowledge our mistakes.
Disclaimer: Code descriptions, method names, functionality details are approximate in order to protect our project.
No comments:
Post a Comment