1
\$\begingroup\$

I have been working on a Magento module for sometime now (not because it is a large module, it's my first and I really struggled with it). I now have it working, which I was really pleased with at first, but now I would like to improve it by increasing the reusability of the code.

While making my module I have looked at other modules to learn from them and have seen that many actions are only a line or two and I have tried keeping in with the skinny controller approach but failed, my example is as follows:

I have a function that get a users details that they have inputted into a custom form.

protected function _setPostData()
{        
    $this->_salutation = $this->getRequest()->getPost('salutation');
    $this->_f_name = $this->getRequest()->getPost('f_name');
    $this->_l_name = $this->getRequest()->getPost('l_name');
    $this->_email = $this->getRequest()->getPost('email');
    $this->_emailAddressCheck = $this->getRequest()->getPost('emailAddressCheck');
    $this->_gender = $this->getRequest()->getPost('gender');
    $this->_country = $this->getRequest()->getPost('country');
    $this->_pref_lang = $this->getRequest()->getPost('pref_lang');        
}

I feel that this is not the correct way to do it and that there is a better way of achieving the same goal. As you can see, this function gets the posts data and assigns it to attributes that I've set at the start of the class. I have several other examples that are very similar to the above and if someone could please offer some guidance on this one I am sure I will be able to work out the others.

This example is held within the index action. Should I put it in a helper? Once it's created correctly, will there be a few occasions in which I will be able to use it again.

\$\endgroup\$
1
  • \$\begingroup\$ I'm not familiar with Magento. Is there no Form component that encapsulate all the form-request-validation-stuff? Just as a example not a recommendation Synfony Forms \$\endgroup\$ Aug 16, 2013 at 8:11

2 Answers 2

2
\$\begingroup\$

First, I would apply self-encapsulation by changing:

$this->_salutation = $this->getRequest()->getPost('salutation');

to:

$this->set_salutation( $this->get_parameter( "salutation" ) );

Since PHP can call functions using a string, it can be simplified further:

function set( $p ) {
  $f = '$this->set_' . $p;
  $f( $this->get_parameter( $p ) );
}

function get_parameter( $p ) {
  return $this->get_request()->get_post( $p );
}

Then:

$this->set( "salutation" );
$this->set( "first_name" );
$this->set( "last_name" );
$this->set( "email" );
// ... etc.

You can take that one step further by iterating over all HTML FORM variables, which removes the repetition altogether:

foreach( $this->get_form_parameters() as $param_name => $param_value ) {
  $this->set( $param_name );
}

However, that could lead to attempts on making arbitrary function calls by the client, so you'd have to be careful.

What I like about removing the repetition is that it sparks of being consistent with the DRY principle. The HTML form has the list of variables that need to be set -- now the developer need only change the HTML form variable list to change what variables are set by the object. This leads to a single source for the variable list.

\$\endgroup\$
1
\$\begingroup\$
  1. Use a variable for the request rather than getting it each time.
  2. Use consistent case (i.e lowerCamelCase rather than lower_pascal_case). This should also be done in the form post variables too.
  3. Use meaningful names for f_name and l_name (firstName and lastName)?
  4. Line up multiple set statements.

The code becomes:

protected function _setPostData()
{
    $request = $this->getRequest();

    $this->salutation        = $request->getPost('salutation');
    $this->firstName         = $request->getPost('firstName');
    $this->lastName          = $request->getPost('lastName');
    $this->email             = $request->getPost('email');
    $this->emailAddressCheck = $request->getPost('emailAddressCheck');
    $this->gender            = $request->getPost('gender');
    $this->country           = $request->getPost('country');
    $this->prefLang          = $request->getPost('prefLang');
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.