4

I'd like to create a highly maintainable code, with unit tests.

I have read the best practices so directories are organized as per best practice, with just one bundle named AppBundle, and I'm using annotations.

I have an issue with business logic. I've read that I should not put business logic inside controllers. As the vast majority of my code was not meant to be reused I put the logic inside the controllers. Yesterday I read here that "logic should live in the controller unless you’re going to unit test it or until you need to re-use it" and this made me faint: am I not going to unit test if I put my logic inside controllers?

I understand that "controllers should be as lean as possible, with few lines of code to glue stuff together". But what about forms? A form often has a logic like "display the form, if it's valid do something and display another page". Now if I'm going to put the form creation and logic inside a service (or a model?) I'd have to put the page render command inside that, so basically the controller would be 1 line, with all the rest inside the service, and the actual decision of which page to display would be done inside the service, not the controller itself...so what's the point of a controller, just the routing?

I really need to understand this before proceeding (i've already developed 3 months on this and I'd have to rework a lot, but better now than never)...

Thank you!

EDIT: some additional considerations to address some of the comments below.

I have a clear understanding of how forms work in Symfony, with the creation of the form and it's management with the "isValid()" in the same place.

Let's imagine the following controller which performs the following operations:

get the current customer from security context
queries the DB to get the field $user->getIsBillable()
if the user is not billable
    queries the DB to find if someone else is paying for him
else //the user is billable
    create and manage form1
if the user is not billable but there is someone who is paying for him
    check if the license is active (I have to call an external API)
    if the license is not ok
        redirect to a page to update credit card info
if the user is not billable
    create and manage form2
else
    create and manage form3
render the license management page with all the necessary forms created

What would be supposed to do? Put all this stuff into a service which would return the forms (or null if a form is not created)? Consider that in the "is valid" of some of the forms I render the same page with simply updated info. Or should I create a service foreach of the forms creation and keep the logic in the controller?

Sergio Negri
  • 2,023
  • 2
  • 16
  • 38

4 Answers4

7

You should keep your logic outside the controllers in order for your code to be more reusable and to reduce code duplication.

The best practice is to have your business logic registered as a Symfony service instead. The controller should be as lean as possible as it only handles the view and accessing your services to run business logic functionality.

Using services will make your code more reusable and it will be easier to write unit tests for each part of your code.

Forms are handled in a different way in Symfony2 and you should read more about it: http://symfony.com/doc/current/book/forms.html

D_R
  • 4,894
  • 4
  • 45
  • 62
  • @SergioNegri, about your updated question you can also define the form as a service, and after you do so you can inject services into it. so you just need to create methods in the service that will validate each of the steps you mentioned and it will build the form by that flow. anyway consider accepting my answer since it answered your question. Thanks. – D_R Mar 24 '15 at 09:54
  • So you are suggesting that the controller is basically 1 line, calling a service, which in turn creates all the forms (which are services themselves)? – Sergio Negri Mar 25 '15 at 16:03
  • @SergioNegri, no you should create the form IN the Form Object, using the buildForm method. and in the controller you are just calling something like `$form = $this->createForm(new TaskType(), $task);` Read here: http://symfony.com/doc/current/book/forms.html#creating-form-classes – D_R Mar 25 '15 at 23:10
  • Yes, I had that part clear, my question is: if I'm creating the forms above in the controller (calling the createForm) then all the logic will be in the controller. On the other hand it seems many suggest that I should have a leaner controller: so the controller (almost empty) will call a service which in turn will create (via createForm) all the forms. Right? – Sergio Negri Mar 26 '15 at 08:54
  • @SergioNegri, I prefer to use my controller to call the buildForm methods since it's only a single line to do it in Symfony2. but you can also use a ViewModel pattern. **EDIT** Take a look at this bundle which is implementing ViewModel in Symfony2: https://github.com/aequasi/view-model-bundle – D_R Mar 26 '15 at 09:06
  • OMG all I needed was the additional layer of the ViewModel :( – Sergio Negri Mar 26 '15 at 10:08
  • @SergioNegri, I personally don't use that pretty often, but go for it if it fits for your needs. – D_R Mar 26 '15 at 10:16
2

Like most php frameworks, symfony2 has its problems.

Turns out symfony2 best practices are not always best practices. But in most cases symfony allows for loose coupling and proper dependency injection, it just doesn't tell you in the "book".

For example a very important topic: http://symfony.com/doc/current/cookbook/controller/service.html

I usually have my form types as services, my controllers as services and other services for the business logic. My controller actions are ideally around 3-5 lines of code. Sometimes when handling a file upload they will be slightly longer though.

You can unit test your business logic services, but for your controllers and forms you will need some sort of integration testing, or use heavy mocking.

I understand that "Selenium" is a good tool for testing such things, I haven't used it yet.

Marcel Burkhard
  • 3,453
  • 1
  • 29
  • 35
1

One thing that I would like add to other answers provided there to you is that Unit Test is different from Functional Test.

Unit Test

You need Unit Test when you want to test individual "unit" of code (such as method in a class)

Functional Test

You need Functional Test when you want to test a "suite" of functionalities (like many methods interactions, db interactions, web services and so on)

Recap

Ideally you want to write Unit Test(s) for a class that contains heavy business logic whereas you need Functional Test(s) for testing whole controller (so a page is render with all class interactions and db ones, the page is render, the page is posted and so on)

The best approach, to me - and it's only an opinion of a devel that tried as today to understand testing from a newbie point of view - is to mix each other: with Unit Test (and isolation, of course!) I'm sure that (simplifying) every input that I provide to my method will return the correct output, while functional test will help me to test the whole thing and interactions.

Answer

So, returning to your question - that seems wider that at first look could seems - a good way to procede is to keep all business logic outside controllers (even if you're sure [and to me you'll never be sure enaugh] that code you put in will be never reused elsewhere) by defining services (as said in other answers). Then you will do some unit test(s) onto your services and after that you will probably need some Functional Test(s) to check that all "the glue keep things together correctly".

I don't know if I'm missing something (it's likely). Just to be more complete, I will attach my older question on Symfony2 (I was studying it and MVC pattern at the time)

Community
  • 1
  • 1
DonCallisto
  • 29,419
  • 9
  • 72
  • 100
  • 1
    Nice addition to the answers. btw the attachment link is not working. – D_R Mar 23 '15 at 11:37
  • Thank you very much. Please see my question edited in case you want to add anything ;) – Sergio Negri Mar 24 '15 at 09:51
  • I read your original post...and got even more confused. I see business logic in your controller, don't you? I really don't get, for instance in my example, what would be the preferred approach. Controller calling a service which does everything? Controller calling several services (one for each form)? – Sergio Negri Mar 25 '15 at 16:05
  • @SergioNegri: in the first snippet there is, in the second (refactor one) isn't. Of course calling a service, verify for POST and bind request isn't business logic to me or, at least, could stay into controller: bind and verify for post is zone everywhere, is true, bit you'll not get any benefit to migrate that cose elsewhere. Don't you think? – DonCallisto Mar 25 '15 at 17:57
  • @DonCallisto that is how I would do it, but your case is very simple. In my case where I have to decide dynamically if creating some forms or not what would you do? Creating a 1 line controller which calls a service which does everything (including the creation of the forms via createForm)? – Sergio Negri Mar 26 '15 at 08:57
  • 1
    @SergioNegri: No one prevent you to write more than one line controllers. Is obviously up to you. You don't have to crystallize your mind onto this kind of things. When you face this kind of problems, to me, only question you have to ask is: Could this logic be applied elsewhere? Is obvious that forms "logic" could be contained both into controller or service but if you only need to perform (for instance) some if statement to decide whether create a form or not, at least imho, is pretty useless to create "ad-hoc" service. If the logic "expands", maybe, it could be useful. – DonCallisto Mar 26 '15 at 10:12
0

Structuring your code to be clean and maintainable it is a very hard stuff, I suggest you to start studying the SOLID principles than having a superficial understanding of MVC:

  1. Single Responsability Principle - SRP
  2. Open/Closed Principle - OCP
  3. Liskov Substitution - LSP
  4. Interface Segregation - ISP
  5. Dependency Inversion Principle - DIP
Alexandru Olaru
  • 6,842
  • 6
  • 27
  • 53