Are relatively “thick” controllers normal with Zen

Question

I've got a form that looks like this:

class Cas_Form_Company extends Zend_Form
{
    /**
     * @param Cas_Model_Company|null $company
     */
    public function __construct(Cas_Model_Company $company = null)
    {
        parent::__construct();

        $id = new Zend_Form_Element_Hidden('id');

        $name = new Zend_Form_Element_Text('name');
        $name->addValidator('stringLength', false, array(2,45));
        $name->addValidator(new Cas_Model_Validate_CompanyUnique());
        $name->setLabel('Name');

        $submit = new Zend_Form_Element_Submit('Submit');

        if ($company)
        {
            $id->setValue($company->GetId());
            $name->setValue($company->GetName());
        }

        $this->addElements(array($id, $name, $submit));
        $this->setMethod('post');
        $this->setAction(Zend_Controller_Front::getInstance()->getBaseUrl() . '/Asset/company');
    }

    public function Commit()
    {
        if (!$this->valid())
        {
            throw new Exception('Company form is not valid.');
        }

        $data = $this->getValues();
        if (empty($data['id']))
        {
            Cas_Model_Gateway_Company::Create($data['name']);
        }
        else
        {
            $company = Cas_Model_Gateway_Company::FindById((int)$data['id']);
            $company->SetName($data['name']);
            Cas_Model_Gateway_Company::Commit($company);
        }
    }
}

Now, this form depends on a controller, which looks something like this:

public function companyAction()
{
    if ($this->getRequest()->isPost())
    {
        if ($this->getRequest()->getParam('submit') == 'Delete')
        {
            Cas_Model_Gateway_Company::Delete(Cas_Model_Gateway_Company::FindById((int)$this->getRequest()->getParam('id')));
            $this->_helper->redirector->setCode(303)->gotoSimple('companies');
        }

        $form = new Cas_Form_Company();
        if ($form->isValid($this->getRequest()->getParams()))
        {
            $form->Commit();
            $this->_helper->redirector->setCode(303)->gotoSimple('index');
        }
        $this->view->form = $form;
    }
    else if ($id = $this->getRequest()->getParam('id'))
    {
        $form = new Cas_Form_Company(Cas_Model_Gateway_Company::FindById($id));
        $this->view->form = $form;
    }
    else
    {
        $this->view->form = new Cas_Form_Company();
    }
    $this->_helper->viewRenderer->setScriptAction('formrender');
}

It seems like the controller action is doing "too much" here, but I don't see an easy way to work around this. Generally speaking, I think the form should be the one worried about whether it's an add or edit or delete operation. But I can't seem to find a good way of going about doing that.

Is this a normal pattern for someone using Zend_Form or have I done something wrong?

Answer 1

It seems you mix up some responsibilities in your form, model and controller. Usually I introduce a service layer when I have some interactions with models from the controller, based on form data.

Also, I see you are handling multiple functions in one action, which I'd split between different actions. If you are working with a Company model, I'd suggest to create a CompanyController:

class CompanyController extends Zend_Controller_Action {}

Next, you probably want to view the company details on one page, modify these on another and create a new instance on a third page. I usually do this with a viewAction(), editAction() and newAction():

public function indexAction ()
{
    $service   = new Cas_Service_Company;
    $companies = $service->getAllCompanies();

    if (!count($companies)) {
        throw new Zend_Controller_Action_Exception('No companies found');
    }

    $this->view->companies = $companies;
}

public function viewAction ()
{
    $service = new Cas_Service_Company;
    $company = $service->getCompany($this->getRequest()->getQuery('id'));

    if (false === $company) {
        throw new Zend_Controller_Action_Exception('Company not found');
    }

    $this->view->company = $company;
}

public function editAction ()
{
    $request = $this->getRequest();
    $service = new Cas_Service_Company;
    $company = $service->getCompany($request->getQuery('id'));

    if (false === $company) {
        throw new Zend_Controller_Action_Exception('Company not found');
    }

    $form = new Cas_Form_Company(array('company' => $company));
    if ($request->isPost() && $form->isValid($request->getPost())) {
        $service->updateCompany($company, $form);
        // redirect here to company view for example
    }

    $this->view->form    = $form;
    $this->view->company = $company;
}

public function newAction ()
{
    $request = $this->getRequest();
    $form    = new Cas_Form_Company;
    if ($request->isPost() && $form->isValid($request->getPost())) {
        $company = $service->createCompany($form);
        // redirect here to company view for example
    }

    $this->view->form    = $form;
}

public function deleteAction ()
{
    $request = $this->getRequest();
    $form    = new Cas_Form_DeleteCompany;
    if ($request->isPost() && $form->isValid($request->getPost())) {
        $service->deleteCompany($form);
        // redirect here to index for example
    }

    $this->view->form    = $form;
}

Now you have seperate actions for all functions, it's quite easy to set up a form for Company:

class Cas_Form_Company extends Zend_Form
{
    protected $_company;

    public function init ()
    {
        $this->addElement('text', 'name', array(
            'label' => 'Name'
        ));

        // More elements here

        if (null !== $this->_company) {
            $this->populate($this->_company->toArray());
        }
    }

    public function setCompany (Cas_Model_Company $company)
    {
        $this->_company = $company;
    }
}

In the form I use several neat features:

1. Use init() instead of __construct(), no need to call parent::__construct() and the setOptions() is called. That is beneficial for #2 here:
2. Use a setter for Cas_Model_Company. When you need more dependencies, it's just a matter of putting them in the array when you construct the form (see editAction() as an example in the controller)
3. Inject form data simply by a check if you've set $this->_company. In my case I use Doctrine, so the toArray() is already there. Otherwise, you need to create this method yourself.

The last piece to put everything together is the service layer. Martin Fowler described this also in his book P of EAA and on his website: http://martinfowler.com/eaaCatalog/serviceLayer.html

A typical service class looks in my case always like this:

class Cas_Service_Company
{
    public function getCompany ($id)
    {
        // Get a Cas_Model_Company and load data from database
        // Example for Doctrine:

        $company = new Cas_Model_Company;
        $company = $company->find($id);
        return $company;
    }

    public function getAllCompanies ()
    {
        // Get a Cas_Model_Company and load all data from database
        // Example for Doctrine:

        $company   = new Cas_Model_Company;
        $companies = $company->findAll();
        return $companies;
    }

    public function updateCompany (Cas_Model_Company $company, Cas_Form_Company $form)
    {
        // Update model with new form data
        // Example for Doctrine:

        $company->fromArray($form->toArray());
        $company->save();

        return $company;
    }

    public function createCompany (Cas_Form_Company $form)
    {
        // Create model with form data
        // Example for Doctrine:

        $company   = new Cas_Model_Company;
        $company->fromArray($form->toArray());
        $company->save();

        return $company;
    }

    public function deleteCompany (Cas_Form_DeleteCompany $form)
    {
        // Get a Cas_Model_Company and load data from database
        // Example for Doctrine:

        $company = new Cas_Model_Company;
        $company = $company->find($form->getValue('id'));

        // Remove this instance
        if (false !== $company) {
            $company->delete();
            return true;
        } else {
            return false;
        }
    }
}

With this kind of service layer, you keep all code to access and modify models in one place. When you need another query (find a company based on a specific criteria) you can just add that method to your server class and call the method in (for example) a controller. It will keep your code very clean.

To conclude: use a service layer and just use the controller as information agent: getting data from source A and bring it to point B. Make a setter for your form to accept the model and use it as a kind of a decorator pattern

Answer 2

That's perfectly fine, all you are doing is sending values to your form and handling the response, redirecting a bit and handling the view.

Each of these things fits really good and would be a hassle to find if you've placed them elsewhere. I wouldn't think of "fat controllers" as in "requires more LOC than other controllers/actions" but rather "contains business logic". If you feel the cyclomatic complexity is becoming too large, try splitting your action into smaller ones.

edit: Actually, I would refactor the $form->Commit(); part to something like $repository->create($form->getValues() since a) nothing internal is used in Cas_Form_Company::Commit(), and; b) you will want to have storage related functions separated from validating & displaying your form. Think about debugging/changing the way your data is stored and you will now where to look if all f.ex. classes containing queries are doing that and only that - handling the DAL.