test.ical.ly | getting the web by the balls

Jan/13

15

4 simple tips to avoid fat controllers with Symfony

20 Comments · Posted by Frank in The real job

Fat ControllerFat controllers, you know them: Those bloated controllers which grew up in size drastically during a project lifetime. It is so easy to create them but difficult to get rid of them after a project went live once. Controllers should be kept to a minimum as they should be exchangeable and without any business logic.

In this article I provide 4 simple tips to avoid fat controllers and which are easy to follow.

by Frank Stelzer*

Let me give you an example for a fat controller. I just prototyped this code and never executed it, but i hope you get the idea:

[gist id=4354227 bump=4]

 

1. Do not start coding in controllers

This tip is quite obvious and sounds easy to realise. But experience has shown the very reverse. During crunch times or prototyping it is tempting to start coding directly in controllers and do pretty much everything there, this means calling lots of different services, sending mails, maybe calling other web services, form handling or any other logic.
In very short time requirements could be implemented this way but for every new line this code looses maintainability and testability.
So, if you have to implement a new feature do not create a controller for it in the first step. Just create a new Handler, Worker, Monkey class (or whatever you prefer naming it) and start completely from scratch. When you need a form, inject the FormFactory. When you need a repository, just inject it. Whenever you need another service, inject it.
According your dependencies you’ll recognize very soon when you have to split up your class. I personally break up a class when the dependecy count exceeds the limit of five, some people doing it even for three dependencies.
Defining and requesting those dependencies will slow down the productivity in the first minutes and for some developer this could be quite bothersome but in long term this will save the whole project a lot of time.
After you are done with this handler it’s time creating a new controller for it. The controller then does nothing more than calling this service and maybe assigning some parameters. The whole logic of this new feature is located outside of the controller and the controller itself contains no further logic and is only some lines big.

2. Use the single responsibility principle also for controller actions

Nowadays the single responsibility principle is well known and widespread. It is often used but not in controllers. Each controller action should only have one responsibility not more.

Take the updateAction method from the example. This method provides the edit view and in the case of a post request it tries to execute the update workflow. You may know kind of those methods from the symfony 1 auto generated module code or even from the Symfony2 documentation but personally i do not follow this approach to make my code cleaner and more secure.

The updateAction could be refactored in this way:

[gist id=4500311 bump=3]

The update action is split up in two actions. One for editing in the case of a GET request and the real update action in the case of a POST request. This is a pretty easy step thanks to the Method annotation.

Each action is now responsible for one thing and easy to understand. The new code may look more bloated than before but after optimizing the complete controller it will look another way.

 

3. Extract your shared methods or avoid them at all

The third tip is about shared methods in controllers. For new controllers they should be avoided at all as the logic there should be located somewhere else but not within the controller.
However when an existing controller contains those shared methods they have to be extracted and moved to some new classes.
The loadModel method in the given code snippet is an example for some shared logic misleadingly included in a controller. This code could be located everywhere but the controller is the worst place for it. Reusage of this code is not possible outside of this controller and leads to code duplication in the case it is needed somewhere else.
Solving this problem depends on how much the according code is encapsulated with the rest of the controllers’ code.
First of all you should create a new service and just cut and paste the original method content. Afterwards you have to analyse the dependencies of this code and inject them. Defining this service and its dependencies in the container is the next part. Now this service is ready for usage in the shortend controller. For backwards compatibility and for minimizing the refactoring time this new service should be called in the original controller method, like in this example:.

[gist id=4354250 bump=3]

I now this sounds easier than in reality but you should try it anyway. With some experience you get to know which code could be swapped out and from which one you should keep your hands off.

4. Go the test driven way

The swapped out code from the second tip is additionally also more testable and this leads me to the fourth tip.
Going the test driven way you are in the need to produce testable code quick and without pain. The test cases are mostly realised with unit tests and rarely or even not at all with functional tests. Functional tests are however the only chance to test controllers reasonable (i am ignoring the possibility to “unit” test controllers here). Consequently developers doing TDD are automatically writting code which is not located in controllers. When you don’t believe it or when you are not used to TDD then just try it. Write some first test case of some web page and try to implement code for it. For sure this code will not be located in a controller. Only in the very last step when this code should be executable from the web you’ll make it accessible in controllers with the help of services.

 

Conclusion

Summarising the goal should always be to write as less code as possible within a controller. When you have to work with existing fat controllers try to minimize them as soon as possible.

Taking the fat controller from the start a minimized version could look like this:

[gist id=4354236 bump=4]

The complete business logic has been removed from the controller and is accessed with the help of services. Those new services could be easily tested and are ready for re-usage.

But maybe you have any further trick? For any other ideas or feedback feel free to drop me some lines in the comments.

* Frank Stelzer (@frastel) has been working with PHP since 2001 and fell in love with Symfony during his university studies more than 6 years ago.
After his degree he started to work in the eSports industry and developed for a high load platform. Focusing on performance, clean code, unit testing or other aspects of code quality he loves to review and discuss about code.
For increasing his support in the Symfony world Frank joined the SensioLabs Deutschland Team in 2011 where he is working as a software architect.

Feel free to contact if you’re interested to write your own guest post!

· ·



  • http://twitter.com/sebwebdev Sebastian Knuell

    Nice post as usual! I am shure developers know about “thin controllers” by now. But as you point out, this best practice is often lost in the daily hastle. Even if you already have rather minimal controllers, there is almost certainly something to improve and tweak.
    I really like the FormHandler using try/catch in the update method for example. Avoiding those duplicated bind and validate methods. Splitting up the edit/update method makes a lot of sense as well: Single Responsibilty taking the HTTP method into account.

    If you have a lot of view specific formatting of your models before assigning it to the view, an optional view mapper class resp. layer can be helpful as well. Since you might not want to have this view bound logic in other services.

    And I like your reasoning concerning TDD: if you deny writing unit tests for controllers, but require all logic to be unit tested, you force the developer to move _all_ logic (not only mayor parts) to services/worker/builder/etc classes.

  • Alexandre Salomé

    bro’tip: if ($form->bind($request)->isValid())

  • cordoval

    sick, nice trick

  • cordoval

    Frank, Chris, thanks for hitting it again after a long while, it was a great post. Please do not stop.

  • http://twitter.com/sebwebdev Sebastian Knuell

    Well, sure. But I still prefer

  • http://twitter.com/weyandch Christian

    You’re referencing 3 different services for the same model class within the thinController implementation.
    When you’re working on a complex project, won’t the amount of services needing to be registered generate some overhead which badly influences maintainablitiy (100ish lines of service.yml) and performance?

  • heller

    If I separate the edit (with GET) and update (with POST) actions, the page will be stuck on the update url if a form validation error appends. So if I refresh the page or reopen the browser which restore the tab, the page can’t be found (because it’s no more a POST request).

    Isn’t this a bad thing ?

  • frastel

    Oh, I mixed up the routings in the latter example. The edit and update actions need to have the same URLs but different request methods. I updated the example. With this trick the normal edit action will be displayed when the user reloads the page. Thanks for the hint.

  • frastel

    The amount of needed services might be a problem on the first view. But in consideration of single responsibility one have to do it this way. One service for creating the form, one for the persistence and one for creating the paginated list. Admittedly I am not convinced about the finder service. When you define your entity repositories as services you could directly access them in the controller and do not need one additional finder class. This might be too much overhead.

  • http://twitter.com/BurkhardR Burkhard Reffeling

    Re: #3: Why create a new service and not just put it in a peer method (where it’s supposed to be)?

  • Jay

    You mention that each action should use the single responsibility principle. How exactly do you do this if your page has numerous responsibilities?

    For example, what if I had a “summary” page that pulled data from 5 different repositories and displayed it all for the user? The nature of this page has multiple responsibilities, so how do you address keeping this action thin?

  • http://twitter.com/sebwebdev Sebastian Knuell

    I’d also rather use an entity repository/finder service with multiple methods like find($id), fetchAll($offset, $blocksize), etc.
    In case one methods gets to complex you can easily extract it to a separate service class. For the form handler I do not see any overhead; totally worth it.
    Also the DBAOSC principle applies: Don’t Be Afraid Of Small Classes! :-)

  • http://twitter.com/sebwebdev Sebastian Knuell

    1. You could have an own service class that aggregates the data from 5 repositories. In your action you have only one service/method call to that class.
    2. If your summary page displays 5 separate lists/boxes, you could have one action for each box/list and your summary page action template would just dispatch these (sub-)actions.

  • Jeremy

    Thanks for your reply.

    Your first suggestion is definitely an option, but I don’t understand how it’s a better solution than just putting those calls in the action. You can still unit test your controller action and it’s very unlikely that the method will be used again, since it’s specific to that particular action. It seems that moving the calls to a service would be overcomplicating your codebase only for the sake of having a thin action.

    Sub-actions are also an option. Definitely cleaner, but it’s possible that your repository calls will need data from the previous repository calls, which would make parts of the methods a bit redundant. Calling sub-requests also comes with a performance compromise.

    I still haven’t found a solution for this situation that I’m really happy with. I think that protected controller methods do serve a purpose for this scenario since they offer the best performance, most clarity, and still keep your methods thin.

  • frastel

    I would definitely use sub requests for this approach (like the 2nd option Sebastian mentioned). This means one action which does almost nothing and the template calls 5 sub requests then (maybe with ESI). When some shared information or ressources are needed you could use the session or any elusive storage like redis for it.
    For the sake of scalability you should prefer this way otherwise your page will break as soon as too much “boxes” should be loaded.

  • http://twitter.com/sebwebdev Sebastian Knuell

    Jeremy:
    The first option makes sense if you need to aggregate and merge data from different service calls. This means you have some sort of logic and thus a separate class is justified. Since you are saying “but it’s possible that your repository calls will need data from the previous repository calls” that would be such a case IMO.
    That said the question in general is how you want to define “single responsibility”. Does it necessarily equal to one service call?

    Concerning the second option: as Frank points out shared data between requests can/should be cached, e.g. through a data cache layer like Memcache or Redis. Doctrine for example provides this out of the box: https://doctrine-orm.readthedocs.org/en/latest/reference/caching.html

  • Jeremy

    That’s a good point. I think that caching query results is a good compromise. This way, each service can handle its own responsibilities and you’re not faced with a large decrease in performance.

    Thanks for your thoughts!

  • http://twitter.com/stephchampagne Stephan Champagne

    Very good, short and precise, thanks for the inspiration :)

  • http://twitter.com/stephchampagne Stephan Champagne

    I wonder. Is there a reason you don’t validate your form in the thin update ? It cannot be done in the persistance handler as you lost the form with form->getData() and pass only the entity. You could get DBAL exceptions.

  • Pingback: Lessons Learned @ NZZ – Teil 5 – Refactoring von Fat Controllern | DaRaFF's Blog

<<

>>

Find it!

Theme Design by devolux.nh2.me
To top