Page 1 of 2

Proper Usage of a Controller

Posted: Sun Jul 26, 2009 12:36 pm
by SidewinderX
In an MVC architecture, should the controller be responsible for passing POST/GET values to the model? From my understanding it _should_ but it doesn't seem practical since I am ending up with method headers and method calls miles long, and since they super globals it really is unnecessary to pass the values around.

$model->updateProject($_POST['project_id'], $_POST['title'], $_POST['start_month'], $_POST['start_day'], $_POST['start_year'], $_POST['end_month'], $_POST['end_day'], $_POST['end_year'], $_POST['description'], $_POST['visible']);

Re: Proper Usage of a Controller

Posted: Sun Jul 26, 2009 1:32 pm
by alex.barylski
In an MVC architecture, should the controller be responsible for passing POST/GET values to the model? From my understanding it _should_ but it doesn't seem practical since I am ending up with method headers and method calls miles long, and since they super globals it really is unnecessary to pass the values around.
Yes, at least I do, although I don't access GPC directly I use the response object.

If your parameter counts are that long and you feel funny, that's your spider sense tingling letting you know it's maybe time to refactor the method into two or more.

I personally hate long parameter lists, but sometimes when dealing with models it's somewhat nessecary as a single table can easily have 10 or more fields. Tough call to make.

You could pass the parameters in via an associative array, which is an idea I've been toiling with for a while as it makes your code slightly more dynamic and easy to change. I'm pedantic about the order of parameters (integers first, strings, etc) so I frequently refactor the method signatures and using associative arrays would make the order irrelevant at the expense that documentation systems would no longer be able to generate a accurate API for your models, which is why I haven't done it yet!

The other option is to treat the model as an object, like a wrapper around a data mapper, and initialize the object with the params:

Code: Select all

$model = new Model();
$model->first_name = 'Alex';
$model->create();
The only issue I have with the above, is that it would probably promote the practice of using the data mapper as the model, which is not good practice, the model should not be bound to any particular data source and this would also probably result in having validations done in the controller, which the model should do, IMHO.

Re: Proper Usage of a Controller

Posted: Mon Jul 27, 2009 2:05 am
by matthijs
Many or most frameworks will wrap the POST/GET values in a request object, which is passed to the controllers and models (possibly in a registry). Then you access the values by getters like $request->get('username')

Re: Proper Usage of a Controller

Posted: Mon Jul 27, 2009 4:03 am
by Jammerious
SidewinderX wrote:$model->updateProject($_POST['project_id'], $_POST['title'], $_POST['start_month'], $_POST['start_day'], $_POST['start_year'], $_POST['end_month'], $_POST['end_day'], $_POST['end_year'], $_POST['description'], $_POST['visible']);
Me being a newbie, I'd try and make a general purpose function...

Code: Select all

public function get_post($posts){
  if(!empty($posts)){
    $ret = array();
    foreach(explode($posts,",") as $post){
      if(isset($_POST[$post]){
        $ret[] = $_POST[$post];
      }
    }
    return $ret;
  }
  return null;
} //you would need to add some more checks like to see if all of your requested fields were found etc.
// preping the data
$data = get_post("project_id,title,start_month");
//check if it is ok and stuff, like isset($data)
$model->updateProject($data); //maybe even the updateProject checks if all arguments were supplied...
 

Re: Proper Usage of a Controller

Posted: Mon Jul 27, 2009 4:08 am
by Eran
Instead of passing each field individually, just pass off the entire array.

Code: Select all

class FooController {
     public function barAction() {
           $model = new Model();
           $model -> updateProject($_POST);
           ...
     }
}
Change the model to accept arrays and you have a much more maintainable interface

Re: Proper Usage of a Controller

Posted: Tue Jul 28, 2009 12:41 pm
by alex.barylski
Instead of passing each field individually, just pass off the entire array.
The only draw back now is your interface is arbitrary and impossible for documentors to generate doc for you.

While it's easy enough to print_r() the params inside the method to determine their names and guess at what they do, having that constantly in my face as a cnocrete interface while implementing software is personally important to me.

Yes you could document the structure/array emulating the fields something like:

Code: Select all

function doSomething($array /* $first_name, $last_name, $middle_name, $age, $sex */)
{
  extract($array); // Emulate having local vars -- kinda anti-pattern-ish
}
The comments might *not* stay synchronized with actual parameters and thus become as usless as the comments in most source code, outdated and defunct.

I guess that is why, in this case I prefer explicit programming practices, not to mention they are clear to even the most beginner of developers.

This is a topic of great interest to me as I constantly refactor my method signatures, more than anything else maybe, second to moving files around maybe.

Cheers,
Alex

Re: Proper Usage of a Controller

Posted: Wed Jul 29, 2009 1:29 am
by Christopher
PCSpectra wrote:The only draw back now is your interface is arbitrary and impossible for documentors to generate doc for you.
It isn't arbitrary just because it is an array. It is implied that inside the method that accepts the array there would be checks for any/all necessary values -- with errors identified. Documentation is not only interfaces...

Re: Proper Usage of a Controller

Posted: Wed Jul 29, 2009 2:44 am
by Eran
The only draw back now is your interface is arbitrary and impossible for documentors to generate doc for you.
It's not artbitray, it's just much less verbose. If the attributes of a Project object change (number or name) the interface doesn't have to. It's also much easier to handle (validate + filter) arrays than individual values. Since a method can only have one return value, if there are several error mesages it needs to be an array - better to be consistent with in/out parameters.

Documentating the individual parameters in such a case would only make it less maintainable as well, as any change to the parameters would require a change in the documentation (unless you forget and they go out of sync). My rule of thumb is - for more than 3 parameters, pass an array. Especially when many of the parameters might be optional - it's much worse to pass a bunch of nulls just to get to the parameter position you want in a method.

Re: Proper Usage of a Controller

Posted: Wed Jul 29, 2009 9:47 am
by alex.barylski
It is implied that inside the method that accepts the array there would be checks for any/all necessary values -- with errors identified.
Assuming you performed validations in the model, certainly that would tell the developer what the expectations were, but when using a component or a method as a client developer, implemetation details are not typically what your after, it's the interface, which when passing an array is arbitrary.

The implementor is now responsible for making sure the API is documented, whereas, if you went with an explicit API, at least auto-documentors can do most of that for you.

I see benefits and nagatives to each approach, but having to manually maintain documentation, IMHO is the most boring and the most likely thing to be incorrect. Autodoc builders in this case are a god send.

Re: Proper Usage of a Controller

Posted: Wed Jul 29, 2009 10:01 am
by alex.barylski
If the attributes of a Project object change (number or name) the interface doesn't have to. It's also much easier to handle (validate + filter) arrays than individual values
I think I said pretty much the same thing in my first post. :)
Since a method can only have one return value, if there are several error mesages it needs to be an array - better to be consistent with in/out parameters.
I'm a huge fan of consistency but this is probably a requirement due to your design choice when implementing models. I don't return error codes or messages in my models, I throw exceptions.
Documentating the individual parameters in such a case would only make it less maintainable as well, as any change to the parameters would require a change in the documentation (unless you forget and they go out of sync).
It would be if you documented the parameters, I don't.

My methods all use self describing parameters, so I only run the generator to build the API, no additional comments needed, something like:

Code: Select all

Person::createPerson($first_name, $last_name, $age, $city_id)
Thats all I need for docs and the rest is self-explanatory...again the only headache is adding a parameter or removing one, the API is less fragile, but again, you lose the explicitness that gives you auto-documentation.

p.s-arbitrary was the wrong word...perhaps 'less' explicit would be more appropriate...which possibly equates to less dynamic or more rigid. It's a toss up...explicit programming makes everything very clear, implied logic means you have more figuring to do but the pay off comes in maintenance.

It's the crux between dynamic and static programming principles that drive me crazy every day. :)

Cheers,
Alex

Re: Proper Usage of a Controller

Posted: Wed Jul 29, 2009 10:14 am
by Eran
I don't return error codes or messages in my models, I throw exceptions.
Really? you consider an error in a submitted form to be an exceptional case? I usually just show the user an error message. It's pretty common practice to use an array of error messages for user feedback, never seen exceptions used in such a manner
I think I said pretty much the same thing in my first post.
I might have missed it, but how can that be consistent with what you are saying now? if you specify all the data attributes of an entity directly in the interface, any change in those attributes will force the interface to change (and all uses of it in the application to change as well).

Code: Select all

Person::createPerson($first_name, $last_name, $age, $city_id)
That might seems nice for creating, but replace it with an update method (like the OP has asked about) and then you start passing off nulls to attributes you don't want to update. it starts looking messy, especially as the number of parameters grows (imagine a person entity with 10+ attributes)

Re: Proper Usage of a Controller

Posted: Wed Jul 29, 2009 11:14 am
by matthijs
I agree with Pytrin here. if you have one or just a couple of fields/attributes, then it might make sense to set them each in the constructor. But as soon as you deal with more form values it gets messy. Forms can easily have dozens of fields. Are you really going to do

Code: Select all

Person::save($first_name, $last_name, $age, $city_id, $street, $streetnumber, $postcode, $interest, $favoritepet, $favoritecolor, $some, $more, $fields /* et cetc etc */)

Re: Proper Usage of a Controller

Posted: Wed Jul 29, 2009 12:01 pm
by alex.barylski
Really? you consider an error in a submitted form to be an exceptional case? I usually just show the user an error message. It's pretty common practice to use an array of error messages for user feedback, never seen exceptions used in such a manner
Yes sir, error messages and outgoing error messages/codes are so 1990's :P

Seriously though making the switch really cleaned up my code.
I might have missed it, but how can that be consistent with what you are saying now?
It's not consistent, why do I have to be consistent when playing devils adovcate with myself and anyone willing to listen? :D
if you specify all the data attributes of an entity directly in the interface, any change in those attributes will force the interface to change (and all uses of it in the application to change as well).
It's a give or take, drop fixed parameter support and say bye bye to autodocs...keep them in place and the problem of maintenance and refactoring becomes more of an issue.
That might seems nice for creating, but replace it with an update method (like the OP has asked about) and then you start passing off nulls to attributes you don't want to update. it starts looking messy, especially as the number of parameters grows (imagine a person entity with 10+ attributes)
I see little difference in updates to be honest, unless your trying to update a single field, in which case I would probably implement a updateField($name, $value) and keep the regular update() form for updating.

Re: Proper Usage of a Controller

Posted: Wed Jul 29, 2009 12:11 pm
by alex.barylski
I agree with Pytrin here. if you have one or just a couple of fields/attributes, then it might make sense to set them each in the constructor. But as soon as you deal with more form values it gets messy. Forms can easily have dozens of fields. Are you really going to do
No, of course not. I would probably break my form up into logic sections and have the model reflect those sections, something like:

Code: Select all

updateAccountDetails()
updatePersonalDetails()
updateForumDetails()
My method signatures rarely (if ever) go beyond 6 or 8 arguments I hate horizontal scrolling. :P

At the same time, I really like having the API for my entire application printed in front of me that I can refer to instantly and simultaneously while working on problems.

Re: Proper Usage of a Controller

Posted: Wed Jul 29, 2009 12:23 pm
by Eran
Yes sir, error messages and outgoing error messages/codes are so 1990's
well they're back.. and throwing exceptions over non-exceptional cases is so 2002 ;) How do pass those exceptions to the user interface to be shown as error messages? just wondering
say bye bye to autodocs
Personally, I don't use autodocs. I don't see much value in it, I document what needs documenting myself (ie, everything that isn't completely self explanatory). Documentation generated just from method definition is not very useful in my opinion (and any good IDE can do that by the way)
I see little difference in updates to be honest, unless your trying to update a single field, in which case I would probably implement a updateField($name, $value) and keep the regular update() form for updating.
I'm not talking about one specific field. When updating often you are only updating a partial list of fields (for example, just the username and email). The array approach accepts a partial list of parameters the same as the full list, with the same interface as the create method. Much simpler to implement, maintain and modify.

How do you handle a variable number of arguments to update using your approach? do you create mock null variables to pass for the parameters that aren't available, or do you use error suppression? create a method for any partial combination of parameters? either solution sounds pretty bad