Page 2 of 3

Re: CRUD controllers

Posted: Fri Aug 07, 2009 3:33 pm
by Eran
If your controllers don't share behavior you are probably writing FAT controllers or your application is not CRUD based.
My controllers are not fat, they're big boned!! seriously though, they're quite thin (8-10 lines MAX per action. most are 2-4). However, there are many UI considerations, as to where the creation of an entity was maybe 3-4 fields (and the rest preconfigured defaults), the editing action is in fact editing several related entities. That happens a lot with more complex data as it streamlines the UI (instead of multi-page forms).

The model still handles those, however sometimes those related entities do not update each other - meaning the controller separates which data goes to which model (as per its responsibility). In this case (which happens often in my experience), it is simply better in my opinion to separate the controller actions. Though you can push all the data to the model and have it decide with several conditionals / operations how to handle the data - I believe it is the controller's responsibility to do so in that case.

I do use a lot of helpers, both in the controller and in the view that take care of those repetitive tasks (I have quite the library of those). Those however are activated manually and not magically. I can understand why people would like their controllers to operate magically and handle most operations abstractly - I did so for a time and it works. Now I prefer to have more control since the data I deal with mostly requires many small adjustments that make the abstractions much less useful overall.

Re: CRUD controllers

Posted: Fri Aug 07, 2009 4:35 pm
by josh
I don't see how having an abstract crud controller results in less control, youre not commited to using the default implementation of the actions just because you refactor your controllers. 10 lines per action x 4 actions = 40 per controller x 15 controllers = 600 lines of controller code. I had over 1k & 6k in the model and was able to remove 700 lines of controller code without "loosing control" of anything. In fact now when I need to override something there is a template method for it, now instead of copying 10 lines of code just to change 1, I just override the relevant template method. I think you are taking "automagically" too literally, the forms are doing most of the work in populateTo() and populateFrom(). All that is automatically happening is a little bit of "glue" to piece together the components.

I had one with the multiple models too, I simply wrapped them in a composite object which strengthened my domain model.

Just because you need to change the impl. of editAction() does not mean you have to override listAction(), deleteAction(), etc...

Likewise some of my stuff had decisions the users have to make regarding deletion cascades, etc... So on those controllers there is some properties, and a deleteAction(), no need to write a fresh listAction() just because were adding something to the delete action

What if you wanted to do something application wide in your controllers, at least zend has controller plugins but what if you need to put something in the middle of a method, its not AOP after all, if you had a hook there already it wouldnt be a concern then

Re: CRUD controllers

Posted: Fri Aug 07, 2009 4:50 pm
by Eran
I agree that there is no necessity to use the abstract functions - which is my point. I still have those (very similar to yours - listAction, deleteAction, editAction etc) in my base controller class, however I find myself hardly ever using those even when it appears at first to be a perfect fit. Somehow they all end up needing custom adjustments and I like to be able to see the logic in front of me while making those adjustments.

As I've said, using those abstract method can definitely work. It's my preference now to have those actions directly in the controller.

By the way, I think the original question was whether to separate the create and edit actions, not whether to abstract them or not.

What is AOP by the way?

Re: CRUD controllers

Posted: Fri Aug 07, 2009 8:07 pm
by josh
Ah, thats specifically handled by my "findOrCreateModel" TM, if there is an id it tries to find that model, otherwise we are probably creating. My template methods get passed an id in some cases, for instance on saving I can easily split logic for generating the success message, on editing we tell the user the record was updated, otherwise ( if id == 0 ) we tell them it was created

AOP = aspect oriented programming

While working with the code immediately all in front of you may be your preference, there are numerous problems with that IMO, mainly because when the API changes you have to go find every place that needs to be updated ( from the way it sounds ).

So from the sounds of pytrin, you try to use the CRUD actions, if one needs to be augmented you copy & paste that method into the subclass controller, then you change or insert code? For changing code I would extract to a TM and override only that TM. For adding code I would add a TM with a blank implementation on the abstract controller, and implement that method to insert behavior at that point. When I have bugs 75% of the time they are in the controller layer because it is the hardest to test, overridng a success message doesnt require additional testing, however if you override that whole method to change 1 line, its simply code duplication. For me personally when there is too much controller code I find myself applying similar refactorings to each controller,

when I need to add a new module I have a basic process,

create the controller, set protected $model = 'Name_Of_Model';

- create the database table ( or refactor the legacy table and import it, I do this in a database refactoring script )
- write functional tests that use the models' data mapper to load data, and populate it to the form, testing my model, form, database, and data mapper
- write separate unit tests for the above components if the module is going to be complex

I'm not saying youre doing anything wrong, that is the right way to program actually IMO, but when I am copying methods and inserting methods I always put like a @todo docblock to go back and extract a template method at that point. Its one thing to get your prototype up and running but if you write your whole app taking lots of shortcuts it could come back and bite you, if you are going to have lots and lots of controllers eventually it will become time consuming to maintain 1000s of lines of controller code, especially when a lot of that maintenance involves mundane tasks like locating the 15 places you copied some line of code that now needs to change

For instance if you had to add a save and continue button on _every_ page how many files would you have to touch? 1? 20? What about if you had to add ajax to every page? etc... What if you wanted to add custom fields the admin of the app could configure from a control panel for any screen on the application? Encapsulation is the goal we are working towards

Like if your editAction were 100 lines it would not make sense to override the whole thing just to add 1 line of code like:

$invoice->setCreated( new Zend_Date );


Just put a hook called preSave() or something and do it there, boom. hours of down the line troubles avoided





Another argument against this is you try to debug and you keep hitting a line break 100s of times before you get to the use case you need to debug because the abstract behavior is used so much. If your unit testing is good enough and you use phpunit you can run a single test to recreate the conditions. This is contrasted where you have the code infront of you but its impossible to update everything at once.

Re: CRUD controllers

Posted: Sat Aug 08, 2009 7:12 am
by Eran
While working with the code immediately all in front of you may be your preference, there are numerous problems with that IMO, mainly because when the API changes you have to go find every place that needs to be updated ( from the way it sounds ).
The thing is - this is not an API. The API is in the models, I don't feel like I need double abstraction to protect me from changes. So far, after dozens of projects done this way there has never been a need to go and change all of the CRUD controllers methods, especially since most of them are not very similar as I've said before. If I would have encountered the need to make a central change to all those methods in the controllers in one place - I would have stayed with the abstraction. But I haven't, and the reduced complexity and increased visibility of logic are worth more to me.

The findOrCreate is another thing I have an issue with - it may work for simple cases, but what about a primary key that is a composite of a foreign key and a timestamp? in this case most likely you only create and update, so it's a waste to try and look for it each time. Another problem is when the ID is derived from other entities and not from user input (such as a username). Of course, you can solve that and still use your abstract methods, but the code becomes more convulted and ridden with conditionals. I prefer to do the simple implementation up front, since controllers are generally not reusable, than keep adding exceptions to an abstract method (which I've done previously). It makes the logic much easier to track, especially for developers new to the stack you've built.

Actually I do have tests for most of my CRUD methods - I prepare a request manually and run the actions one by one. At the least I can catch any errors thrown.

Re: CRUD controllers

Posted: Sat Aug 08, 2009 11:52 am
by josh
So you override only findOrCreate() in that case ( and in this case it would simply always create ), overriding a 4 line method instead of a 40 line edit action, doesn't require any extra conditionals. In fact it eliminates all conditionals from that controller, in your situation.

I'm not sure what you mean by "adding exceptions to an abstract method". However it seems like your controllers don't have as many "seams" ( Michael Feathers ). Controllers are code too and IMO part of the API, I have lots of controller code that eventually becomes model code, personally.

Also reduced complexity and all logic inline is the argument for a 1-tier application.

Re: CRUD controllers

Posted: Sat Aug 08, 2009 11:55 am
by Eran
There are no conditionals in the controller, since it goes to different actions. Also
overriding a 4 line method instead of a 40 line edit action
What 40 line edit action? as I've said, most of my actions are 8-10 lines max.
adding exceptions to an abstract method
I meant by that overriding the abstract behavior with situation specific code (ie, an exception to the rule). I didn't mean exception as in the error throwing kind

Re: CRUD controllers

Posted: Sat Aug 08, 2009 11:57 am
by josh
I thought we were discussing the pros and cons of having the findOrCreate method in my design.

Re: CRUD controllers

Posted: Sat Aug 08, 2009 12:00 pm
by Eran
We were talking about a lot of things, no? :)

about that, I just thought - how do you separate the physical URLs if both edit and create are in the same actions? does the URL stay the same and is only added a parameter (say, id=15)

Re: CRUD controllers

Posted: Sat Aug 08, 2009 12:12 pm
by josh
Naw I have a newAction() which just delegates to editAction()

ZF renders the view for the action that was dispatched so I do some overrides to the form in the view scripts before it renders, which takes care of buttons saying "Create" vs "save".

I meant in my case the editAction would be 100s of lines if not broken down into template methods, when I have it broken into an "API" it is only 40 lines, it is much easier for me to override 1 off template methods and have 1 place to maintain the complexity. it is a design principle called inversion of control. Yes indirection is 1 of the costs to abstraction, and no there is no right or wrong method to doing your project.

Here is an example where I needed to change the "guts" of the editAction(). Here is without a template method:

Code: Select all

 
 
public function editAction()
    {
        if( $this->_getParam( 'employer' ) )
        {
            $invoice = new Invoice();
            $invoice->setEmployer( $this->getMapper( 'Employer' )->find( $this->_getParam( 'employer' ) ) );
        }
        else
        {
            $invoice = $this->findModel();
        }
        
        $this->setBreadcrumbOptions( 'admin-invoice-edit', array(
            'label' => $invoice->getTitle(),
            'params' => array(
                'id' => $invoice->getId()
            )
        ));
        
        
        /***/
        $form = new Invoice_Form( array(
            'invoice' => $invoice
        ));
        $form->populateFrom( $invoice );
 
        if( $this->getRequest()->isPost() )
        {
            if( $form->isValid( $this->getRequest()->getPost() ) )
            {
                $form->populateTo( $invoice );
                
                $invoice->setCreated( new Zend_Date );
                
                $this->getMapper()->save( $invoice );
                $this->flashMessenger->addMessage( 'Invoice Saved' );
                return $this->_redirect( $this->url( array(
                    'module' => 'Admin',
                    'controller' => 'Invoice',
                    'action' => 'index'
                ), 'default', true ) );
            }
        }
         /****/
        
        $this->view->id = $invoice->getId();
        $this->view->invoice = $invoice;
        $this->view->employer = $invoice->getEmployer();
        
        $form->addElement( 'text', 'employer', array(
            'value' => $invoice->getEmployer(),
            'disabled' => 'disabled',
            'label' => 'Employer',
            'order' => 1
        ));
        $this->view->form = $form;
    }
 
 
The part between the comments is what I needed to change. ( as well as the $this->view part and adding an element to the form ). This was written before my refactorings. Now I will go back and eliminate most of that code, the only code that really needs to be overriden:

in the TM for form creation:

$invoice->setCreated( new Zend_Date );

Code: Select all

 
$form->addElement( 'text', 'employer', array(
            'value' => $invoice->getEmployer(),
            'disabled' => 'disabled',
            'label' => 'Employer',
            'order' => 1
        ));
 
TM for setting up view

Code: Select all

 
$this->view->id = $invoice->getId();
        $this->view->invoice = $invoice;
        $this->view->employer = $invoice->getEmployer();
 
TM for findOrCreateModel

Code: Select all

 
# if( $this->_getParam( 'employer' ) )
#         {
#             $invoice = new Invoice();
#             $invoice->setEmployer( $this->getMapper( 'Employer' )->find( $this->_getParam( 'employer' ) ) );
#         }
#         else
#         {
#             $invoice = $this->findModel();
#         }
 
and lastly implement doSave


Code: Select all

 
$invoice->setCreated( new Zend_Date );
 
Having the code all inline can also have consequences of making it harder to read thru IMO, as you can see I am able to override the spots I need to

The downside to having the huge method is my success message, redirect, etc.. are all not encapsulated. To repeat this scheme w/o breaking into separate methods thru 30 controllers would have meant major problems for my project


I recommend reading http://en.wikipedia.org/wiki/Hollywood_Principle

and

http://www.matthewtmead.com/blog/?p=8

Re: CRUD controllers

Posted: Sat Aug 08, 2009 12:51 pm
by Eran
Most of my create/edit actions look something similar to this:

Code: Select all

class SellersController {
        ....
    public function addAction() {
        if($this -> isPost()) {
            $sellers = new Sellers();
            $result = $sellers -> add($_POST);
            if(is_numeric($result)) {
                $this -> _redirect('/admin/sellers/list');
            } else {
                $this -> view -> errors = $result;
            }
        }
        echo $this -> render();
    }
    
    public function editAction() {
        $id = $this -> getParam('id');
        $sellers = new Sellers();
        if($this -> isPost() && is_numeric($id)) {
            $result = $sellers -> save($_POST,$id);
            if(is_numeric($result)) {
                $this -> view -> saved = true;
            } else {
                $this -> view -> errors = $result;
            }
        }
        $this -> view -> data = $sellers -> get(array('name','email','username')) -> where("id=" . (int) $id) -> queryOne();
        echo $this -> render();
    }
}
Those two are similar on the surface, however there is more often than not additional logic that I prefer having in the controller since it is only relevant on the context of the specific action. This additional logic needs to fit usually right in the middle of the method, which would make it awkward to implement if it were abstract.

For me this is much more straightforward than using hooks (which I don't like in general) as they make the logic harder to track, especially for developers not very familiar with your stack.

Re: CRUD controllers

Posted: Sat Aug 08, 2009 2:23 pm
by josh
pytrin wrote:This additional logic needs to fit usually right in the middle of the method, which would make it awkward to implement if it were abstract.
That is an indication of coarsely breaking down your methods. Ideally each method should perform 1 task, not always a realistic goal, but a goal nonetheless. Zend_Controller_Action has an API ( so why cant your controllers be considered the same? ), all I can say is it has been proven bugs correlate with LOC.

What if every controller needed a method to return its errors via a JSON object, having the method getErrors() would provide a nice API to observe that data, whereas a linear program suffers from repetitive types of maintenance when adding new features. Not dissing your code, I dont know what your project is like, etc... but having the low cohesion should not be one of your design goals just because you prefer working with linear code

Re: CRUD controllers

Posted: Sat Aug 08, 2009 2:32 pm
by Eran
I get what your saying, and if logic itself repeats in the controller I immediately refactor it to its own method (like your getErrors example), however - the logic I want to inject is domain and situational specific, and is not reusable.

You are correct that a method should be doing only one thing (ideally), and in this case a controller action is a method that should state all the control logic for that specific action. It's very hard to break it to smaller parts than what I've shown, and that is why I like to be able to modify it directly rather than dance around a more abstract API with hooks and what-have-you.

Re: CRUD controllers

Posted: Sun Aug 09, 2009 1:29 am
by josh
But in your case you are repeating isVald() checks, isPost() checks, pushing the errors to the view, and others. The part you are "injecting" may not be re-usable but the rest of it definitely is ( wether you prefer to have it re-used or not ).

If 100% of the controller code for each action is directly within the method you'd be writing a transaction script ( aka fat controller ) ( not to mention not using of the API on Zend_Controller_Action ), you are of course free to pick your own coding style but it is pretty well accepted low coupling and high cohesion leads to more maintainable code. Plenty of applications don't even separate presentation from business logic ( because it is the programmers "preference" ).

Basically the cons of keeping code linear = low cohesion, high coupling ( duplication = harder to maintain )
Pros to linear code = all your code is in one method?

Pros to template method = in the abstract editAction() the order of the TM calls can be re-arranged, new hooks easily added, existing hooks removed, wrap hooks in conditionals / loops
your perceived cons, but my perceived pros = your code is organized under intent revealing names, granular methods containing less code, each method is more readable but its more work to look at the "big picture" ( this is subjective though, although I would say it is generally well accepted that these are pros for any OO code )

I mean even writing transaction scripts with presentation embedded is a valid way to program, but if I were writing a project with 50 screens I'd want to make damn sure Im not touching 50 controllers every time I change my screens... on the other hand it sounds like you may not have 1000s of lines of controller code, in which case yeah building a framework out of it would be overkill, I assume we were talking about large scale apps though ( at least I was I mean ).

Arguably repeating literal strings all over your app is easier then using constants or variables, repeating string literals has the pros of you can literally look at the code and 100% of it is there, but we all at some point learned to work with some level of indirection, most of us ( at least me ) reject the indirection at first, until we experience problems with it first hand, only then seeking out a better way

Basically we are arguing procedural vs OO, which as we know is a bit of a holy war. Personally I would say it is pretty darn hard to write good OO code, it is however surprisingly easy to write procedures, then over time hierarchically group those procedures as you think of method names for each "group" of procedures, leaving your high level methods reading like plain english, your low level methods become single task utility methods.

Should controllers be OO? There is no answer to that, do OO controllers = higher maintainability? yes and thats a fact, does it make it harder to read? Well I can work with the "forest" in my abstract controllers and I can work on each individual "tree" separately, but it does make it harder to see how all the "Trees" fit into the "forest", indirection is not to be abused

My 2 cents

Re: CRUD controllers

Posted: Sun Aug 09, 2009 1:42 am
by Eran
I'm not really getting you. Yes, I am checking for POST submission in the controller but that method is part of the API (of the controller). The validation is part of the Model's API. Both are abstracted away - and are resued within the controller. So what are the cons in that?
The Model can validate from different actions or from inside other models, easily. I'm passing the errors to the view since that might not be the only scenario - in other cases the errors response could be aggregate to other errors and logged to file, for example. This is the action specific code, and in my opinion should not be abstracted.

Anyway, I think we pretty much ran our course through this discussion as we are not offering new information, just contrasting views. Probably someone has more to say about the OPs original question.