The engineers at Google have posted a list of what they consider to be bad coding practices. Reading it over, I was struck by how many of the violations are common in Symfony code. (Please note, the tone of the article is sarcastic, so they are saying the opposite of what they mean.)

Make Your Own Dependencies - Instantiate objects using new in the middle of methods, don’t pass the object in. This is evil because whenever you new up an object inside a block of code and then use it, anyone that wants to test that code is also forced to use that concrete object you new’ed up. They can’t “inject” a dummy, fake, or other mock in to simplify the behavior or make assertions about what you do to it.

Depend on Large Context Objects - Pass around ginormous context objects (or small ones with hard to construct contents). These will reduce the clarity of methods [myMethod(Context ctx) is less clear than myMethod(User user, Label label)]. For testing, the context objects will need to be created, populated, and passed around.

In most of the form, model and action code that I’ve written, and that I’ve seen others write, it is common to instantiate new objects in the middle of methods. The code in Symfony tends to become all about the side effects, not about the return values of the methods. Functional purists would no doubt want to dynamite the whole project, if they saw some typical Symfony code. One of the authors of the post, Miško Hevery, writes on his blog:

Every time I see Law of Demeter violation I imagine a haystack where the code is desperately trying to locate the needle.

class Mechanic {
Engine engine;
Mechanic(Context context) {
this.engine = context.getEngine();
}
}

The Mechanic does not care for the Context. You can tell because Mechanic does not store the reference to Context. Instead the Mechanic traverses the Context and looks for what it really needs, the Engine.

Hevery feels that the above code should be re-written so that the dependencies are injected:

class Mechanic {
Engine engine;
Mechanic(Engine engine) {
this.engine = engine;
}
}

This is one issue where Symfony is going to see big improvements soon. Fabien Potencier (the lead programmer of the Symfony framework) has written extensively on his own blog about a container for dependency management, which will be included in Symfony 2.0.

It is now time to dive into the Symfony 2 service container implementation.

The Dependency Injection Container in Symfony is managed by a class named sfServiceContainer. It is a very lightweight class that implements the basic features we talked about in the last article.

The Symfony Service Container is available as a standalone component in the symfony official Subversion repository: http://svn.symfony-project.com/components/dependency_injection/trunk/. Keep in mind that this component is still under heavy development, which means that things can change.

In Symfony speak, a service is any object managed by the container.

…[Because] the Container class extends the sfServiceContainer class, we can enjoy a cleaner interface.

Hevery is especially critical of the kind of code that involves multi-level reaching into other objects (a style of coding that I have seen on most of the Symfony projects that I’ve worked on):

class Monitor {
SparkPlug sparkPlug;
Monitor(Context context) {
this.sparkPlug = context.
getCar().getEngine().getPiston().getSparkPlug();
}
}

Formally, the principle of software design that Hevery is promoting is known as the Law Of Demeter:

The fundamental notion is that a given object should assume as little as possible about the structure or properties of anything else (including its subcomponents).

…When applied to object-oriented programs, the Law of Demeter can be more precisely called the “Law of Demeter for Functions/Methods” (LoD-F). In this case, an object A can request a service (call a method) of an object instance B, but object A cannot “reach through” object B to access yet another object, C, to request its services. Doing so would mean that object A implicitly requires greater knowledge of object B’s internal structure. Instead, B’s class should be modified if necessary so that object A can simply make the request directly of object B, and then let object B propagate the request to any relevant subcomponents. Or A should have a direct reference to object C and make the call directly. If the law is followed, only object B knows its own internal structure.

Most of the Symfony projects I’ve worked on are rife with lines of code like this (in the actions):

$this->getUser()->getGuardUser()->getProfile()->getEmail()

To conform to the above ideals (The Law Of Demeter) the code should be re-written as:

$this->getUser()->getEmail()

In other words, the MyUser class should have a method that takes care of fetching the email for you. This way your action class does not get wedded (highly-coupled) to the fact that the MyUser class has a sfGuardUser object which contains a profile object which has a method for fetching email. Instead, the MyUser class hides the mechanics (decouples) the action class from the details of what classes possess the email address. Your action classes can use one of their member objects (the MyUser object) to fetch the email address, but the details of how this email address is fetched is left to other classes to figure out. There are 2 advantages here:

1.) The code is easier to test.

2.) If at some point you feel the need to completely refactor the code that handles your user information, you do not have to hunt through your code and change the dozens of lines where you’ve fetched the email address. Instead, you only have to make sure that the method getEmail() in the class MyUser still knows how to get the email. It is easier to change that one method than to hunt down and change a few dozens lines like this:

$this->getUser()->getGuardUser()->getProfile()->getEmail()

This issue is especially relevant to me right now, because I’m re-doing a Symfony site that was created by a bunch of other programmers. I notice the original programmers wrote a lot of code like this:

foreach($form->getFormFieldSchema()->getWidget()->getFields() as $key => $object)
{
$label = $form->getFormFieldSchema()->offsetGet($key)->renderLabelName();
if(isset($validatorSchema[$key]) and $validatorSchema[$key]->getOption(’required’) == true) {
$label .= ‘^{getFormFormatter()->translate($title) . ‘”>’ . $symbol . ‘}‘;
}
$widgetSchema->setLabel($key, $label);
}

Perhaps the fault here is with the core Symfony team. The ideal of “loose coupling” suggests that the form objects should have a getFields method:

$form->getFields()

Perhaps such a convenience method should be auto-generated?

The core Symfony team can be faulted for promoting the above questionable practices. For instance, on the readme page for the sfGuardUserPlugin, this example is given:

$this->getUser()->getGuardUser()->getProfile()->getFirstName()

// or via the proxy method
$this->getUser()->getProfile()->getFirstName()

You’ll note that even the proxy method breaks the Law Of Demeter. The action code is still assuming that the user object contains a profile object and, even worse, it is making assumptions about what methods that profile object has. There should be a proxy method that allows:

$this->getUser()->getFirstName()

It is acceptable to call the user object since the user object is a member object of the current class. You can call its methods, but you should never call on objects inside of it. Rather, its methods should be smart enough to figure out what you want and get it for you, without you (or rather, your code) having to know the details of how that data is fetched. Hiding the details of implementation, at each level, is good object oriented design.

Symfony programmers obviously take their cue from the core team about what constitutes idiomatic Symfony code. Getting the idiom right is one of the theoretical advantages of using a framework - it allows other programmers to understand your code more easily.

There is a counter-argument to the Law Of Demeter, which is mentioned on the Wikipedia page:

A disadvantage of the Law of Demeter is that it sometimes requires writing a large number of small “wrapper” methods (sometimes referred to as Demeter Transmogrifiers) to propagate method calls to the components. Furthermore, a class’s interface can become bulky as it hosts methods for contained classes resulting in a class without a cohesive interface.

There is a balance of concerns here that programmers need to think about carefully. On the one hand, multi-level reaches through several objects leads to high coupling that makes the code brittle, difficult to change and difficult to test. But the alternative is to write hundreds of small methods which act as aliases for what you are actually trying to get (related to the Adapter or Facade pattern). Writing so many wrapper methods would be time consuming, and therefore expensive, at least in the short-term. So what is the right answer? I think it depends on the long-term ambitions of the project. If you are creating a site that is simple, or that will be unchanging, then it is probably best to go with what is quickest, which would be the multi-level reaches through objects that you see in a lot of Symfony code. But if you’re working on a project that is ambitious (perhaps you are working for a startup that hopes to become the next Facebook or Twitter), then it is best to give the long-term considerations priority over the short-term considerations. If the project hopes to scale up and add new features constantly, then you will need code that is flexible, code that can be changed often without breaking. The initial upfront time needed to write a bunch of wrapper methods will pay off over the long-term.

All the same, the truly best way forward would be for the Symfony framework itself to auto-generate more of these wrapper methods for us.

Hevery is also critical of context objects:

Most applications tend to have some sort of Context object which is the kitchen sink… It tends to have reference to just about every other class in your system, hence the compiler will need those classes too. This kind of code is not very reusable.

Even if you don’t plan to reuse the code, the Context has high coupling with the rest of the system. Coupling is transitive, this means Mechanic inherits all of the badness through association.

I’m looking at this project that I am suppose to refactor. I see many lines like this:

sfContext::getInstance()->getRouting()->getCurrentInternalUri()

This is another problem that derives from the lack of dependency injection in Symfony. For those types of sites where long-term considerations need to be given priority over short-term considerations of speed, the managed dependency injection coming in Symfony 2.0 will be very helpful.

The most serious mistake that I make in my own code is the over-use of statics, which the Google Engineers denounce strongly (remember, they are being sarcastic, so they are saying the opposite of what they mean):

Use Statics - Statics, statics everywhere! They put a great big crunch in testability. They can’t be mocked, and are a smell that you’ve got a method without a home. OO zealots will say that a static method is a sign that one of the parameters should own the method. But you’re being 3v1L!

Use More Statics - Statics are a really powerful tool to bring TDD Infected engineers to their knees. Static methods can’t be overridden in a subclass (sometimes subclassing a class and overriding methods is a technique for testing). When you use static methods, they can’t be mocked using mocking libraries (nixing another trick up the pro-testing engineer’s sleeve).

Create Utility Classes and Functions/Methods - For instance, you have a String which is a URL you’re passing around (obeying “Use Primitives Wherever Possible”). Create another class with static methods such as isValidUrl(String url). Don’t let the OO police tell you to make that a method on a URL object. And if your static utility methods can call to external services as well, that’s even better!

I’m glad to be called out on this one and reminded of the bad habits that I’ve fallen into. On each of my last 3 Symfony projects, I’ve written some custom functions for formatting dates. I made them all static methods of a Date class. The Date class was a pure utility class, without state. It holds nothing but static functions. I was cutting corners, for sure. At some level I was aware that this was only make-pretend OO design. It is good to be reminded that “a static method is a sign that one of the parameters should own the method”. This suggests that the methods really belong in the model classes wherever the data is coming from, or in the form class that first accepts the input. Assume I have a table called Questions and it has a field called “when_was_this_answered” which stores a Unix timestamp, then the model class Questions should maybe have the formatting method that turns that timestamp in a readable date. Of, if the dates are formatted depending on which time zone the user lives in, then the formatting methods probably belong in the user profile class.

The Google Engineers are also critical of helper functions, and especially on this issue, the Symfony framework is perhaps promoting bad practice:

Utils, Utils, Utils! - Code smell? No way - code perfume! Litter about as many util and helper classes as you wish. These folks are helpful, and when you stick them off somewhere, someone else can use them too. That’s code reuse, and good for everyone, right? Be forewarned, the OO-police will say that functionality belongs in some object, as that object’s responsibility. Forget it, you’re way to pragmatic to break things down like they want. You’ve got a product to ship after all!

Certainly, Symfony makes the use of helpers seem normal. The argument for them is that they are lightweight compared to auto-loading classes, and if you don’t need some helpers, then why include them? The argument against them is one that brings up long-term considerations, such as the hit matainability will take when you depart from OO design. One could argue that the helpers included in the Symfony framework are an exception since they are tested and vouched for by the core Symfony team, so as long as you are willing to trust the Symfony framework, you should be willing to trust the included helpers (a commenter on Potencier’s blog says “in symfony we trust”). But I think it’s safe to say that Symfony’s documentation encourages the use of programmer’s making their own helpers, and in that the Symfony project is possibly promoting bad practice (this summer I worked on a project that was initially headed up by an older, very experienced programmer. This was his very first Symfony project, and he read the documentation carefully. When I asked him why there were so many helpers, he suggested that he was trying to stay with the Symfony idiom).

Personally, I’d be happy to see all helpers removed from Symfony. The helpers used in the templates probably belong in a template or rendering class. Since Symfony 2.0 will have a new template enginge, 2.0 would be an appropriate release for the Symfony team to begin to deprecate helpers, while moving the funtionality to more appropriate places.