Code Smell?

PHP
1

Hi,

Given the following Zend Controller code:


public class Account_Controller_Customer {

    public function emailPreferencesAction() {

        if ($this->getRequest()->isPost()) {
            $submittedEmailPreference = new Account_Models_EmailPreference($this->getRequestParams());
      
        //do more stuff here

        }
    }
}

Does passing the request params as a constructor param to a model object
constitute a code smell? Is the bigger code smell the coupling between the controller and the model? Given the controller /model are already coupled what does it matter that the request params are passed to the model? Would it be better to pass each individual array value to the constructor, i.e:

$submittedEmailPreference = new Account_Models_EmailPreference(param[‘value’], param[‘value2’]…, param[‘valueN’]);

That seems pretty smelly given you could have several params being passed to the constructor and the difficulty that would arise in reading such a lengthy constructor.

Your thoughts would be appreciated!
-Sam

2

I would violate TooManyParams, for constructors. Some of the arguments against TooManyParams don’t seem to apply for constructors.

Eg.
The function/method appears too complicated, but usually constructors are just a list of assignment statements.

Passing a associative array just seems icky, and leads to alot of if (array_key_exists(‘foo’, $params)) throw new Exception(‘foo parameter missing’) kinda stuff, its just unpleasant, when you get it free if it was a parameter.

Zend Framework 2.0 has apparently committed to using what they’re calling a “universal constructor”, seemingly every (not just model) constructor takes an associative array. It makes me sad.

3

That’s what I thought when reading this thread: using VO’s because you dislike the way something looks might make sense to some people, but it adds complexity from where I’m standing. But then, not using a VO doesn’t solve the problem of the code smell: you’re still passing an array (which isn’t very explicit), or you’re writing them as parameters, which may lead to the “too many parameters” smell (which I dislike as well).

If you are running into the “too many parameters” smell, you might want to review where you actually use those parameters: do you really need them to instantiate the object? I’ve seen people adding every possible parameter as an (optional) parameter to the constructor, and having methods that don’t accept parameters, while those methods are the ones actually using them.

So, instead of having:

<?php
class Foo {
  function __construct( $foo, $bar, $qux ) {
    list( $this->foo, $this->bar, $this->qux ) = array( $foo, $bar, $qux );
  }

  function connect( ) {
    // uses $foo and $bar.
  }
}
?>

You might want to go for a different approach, which makes more sense on the whole, if you ask me:

<?php
class Foo {
  function __construct( $qux ) {
    $this->qux = $qux;
  }

  function connect( $foo, $bar ) {
    // uses $foo and $bar.
  }
}
?>

That - at least - is a refactoring you can try to get rid of the too many parameters, and I think this is a cleaner solution than passing in an array because you have too many parameters to list them.

This doesn’t usually apply to instantiation of so called “Domain Model” objects though: they tend to need all the information before calling a single method on it (e.g. validate or save?) so I too pass in an array, although I dislike doing that. I’d love to see there was parameter passing as it is implemented in JavaScript:


<?php
class Foo {
  function __construct( $foo, $bar, $qux = null ) {
  }
}
$foo = new Foo( array( 'bar' => 1337, 'foo' =>42 ) );
?>

Is that the best of both worlds, or is it unnecessary and unreadable? (syntax may be improved, obviously) (-:

4

@pgb101 - thank you for your very thoughtful response

My intention with the following response is to walk through some different possibilities without being pedagogical in the slightest.

To me the biggest culprit in all of this is the framework. In an ideal world
I would expect Zend to populate the form (via introspection) and make that
available via a method param. That would make the following possible:

public function emailPreferencesAction(Zend_Form emailPreferencesForm) {

	$emailPreferencesVO = new EmailPreferencesValueObject(emailPreferencesForm);
	$submittedEmailPreference = new Account_Models_EmailPreference($emailPreferencesVO);

}

Appears relatively smell free to me. No bucket programming, well defined interfaces, allows for type hinting etc. The only issue, that I can see, is having to update both the VO and the model if there is a change to the (hypothetical) form.

Back to the real world. The issue is the associative array we are forced to deal with. That leads to the following code possibilities:

// Code possibility #1
//
public function emailPreferencesAction() {
	
	$submittedEmailPreference = new Account_Models_EmailPreference();
	$submittedEmailPreference->setValue1($this->getRequest->getParam('value1'));
	......
	......
	$submittedEmailPreference->setValueN($this->getRequest->getParam('valueN'));

}

Stinks to high heaven? Sure. We’re not encapsulation that which varies. Leads to code duplication (having to
repeat this whenever we create an instance of EmailPreference). Also, what if we forget to call one (or more)
of the mutators? I don’t think anyone is advocating this, correct?
Next up:

// Code possibility #2
//
public function emailPreferencesAction() {
	
	$submittedEmailPreference = Account_Models_EmailPreference::create($value1, $value2.....$valueN);
	//OR
	$submittedEmailPreference = new Account_Models_EmailPreference($value1, $value2.....$valueN);
	....
	
}

Works fine for 3/4 params. Anymore than that and we start a merry walk down this path - TooManyParams
Interesting - mentioned as a possible solution is to pass in an associative array, which leads me to:

// Code possibility #3
//
public function emailPreferencesAction() {
		
	$submittedEmailPreference = new Account_Models_EmailPreference($params);
	....
	
}

Reservations regarding this possibility have been expressed earlier (bucket programming etc). However there isn’t
a general consensus as some folks advocate this possibility and see it as reasonably (if not entirely) smell free. Finally:

// Code possibility #4
//
public function emailPreferencesAction() {
	
	$emailPreferencesVO = new EmailPreferencesValueObject($params);
	$submittedEmailPreference = new Account_Models_EmailPreference($emailPreferencesVO);
	....
}

We’re still indulging in bucket programming but the model interface is well defined. Of course if we add a new field to our UI, we’d have to update both the value object and the model as mentioned earlier.

Unless I’m missing a key point it seems to me that given the framework, a code smell is unavoidable (unless of course you don’t think #3 is an issue. I’m trying my hardest to be inclusive here!) But to those who feel #3 is an issue, is it really a matter of choosing the least smelly code? If it is personally I’d go for #4.

Thoughts?

Cheers,
-Sam

5

Hi…

Yes.

And you will find that awkward later if you want to test the controller separately from the model. It might be your app is not so complicated that you need to test the controllers, so the extra magic would not be worth it. Wait a while.

yours, Marcus

6

Hi…

The only coupling is the key names in the hash you passed.

Passing individual parameters would remove those names and you’d generate an error if you forgot to completely add an extra parameter everywhere in the code. You may want that. Bit like strong typing.

You’re winning with minimal cleaner code. In this case, assuming you have a test suite, I reckon you are ahead right now.

yours, Marcus

7

The key names are not coupling, since they are not related specifically to the controller. They are just constructor requirement, wherever the object is being instanced. And the parameters themselves can all be optional using an array, which is harder to do with defined parameters.

8

Hi…

Another possibility:


$submittedEmailPreference =
        new Account_Models_EmailPreference(
                $request->just('email', 'name', 'country'));

You are still passing a hash, but the information is restricted in the caller (controller). This should satisfy security auditors if that’s a concern.

yours, Marcus

9

While I understand that the term “coupling” has become a very dirty word in programming, that doesn’t make the mechanism itself inherently evil.

If modules cannot communicate with each other, it would not be possible to have functional software. Letting them communicate is a good thing. We like software that works and actually does something :wink:

Coupling is what facilitates this conversation. That is not to say that coupling cannot be misused or used rather poorly. In fact uncontrolled coupling is quite ugly and is a bad thing. This is where the “coupling is evil” mantra stems from, but it is quite an oversimplification of things that more resembles fear mongering than actual sound advise.

Controlled coupling, however, is quite a desirable thing.

I won’t disagree that coupling can and has been gravely misused and abused, but that doesn’t make the mechanism in and of itself inherently bad or evil. Fundamentally, it’s neither good nor bad… it is how the programmer uses and controls it (or doesn’t control it) that ultimately decides that.

10

Hi,

Ok, just to make things clearer - in this hypothetical instance, the array contains eight params containing values submitted by the user. ‘TooManyParams’ advocates using an associative array, param object or some other method (certainly not passing each individual param) to instantiate, in this case, the EmailPreference model object (yes, all params are required for instantiation). There are some opinions advocating against ‘bucket programming’ - i.e. not passing an array as a constructor param. I’d like to see how opponents of bucket programming would deal with this particular situation. A concrete code example would be greatly appreciated! I’d also like to hear from advocates (Marcus?) of what some call ‘bucket programming’. Why do you think this term isn’t a pejorative i.e. why passing an array as a constructor param is ok in light of the concerns raised by the ‘anti-bucketers’?

Also, if you were Ruler of the World, how would you improve Zend Framework so that this discussion could be more easily resolved? I mentioned earlier that I would prefer the Zend form to be auto-populated by the framework rather than being handed an array of params. But that still leads us to the issue of how the model is instantiated? Obviously not by passing the form as a constructor param to the model…

Cheers,
Sam

11

If there is some sort of HTML form description that deals with the validation, it seems to make sense to have the factory method in there?


$form = new Accounts_Email_Form($request);
try
{
$submittedEmailPreference = $form->createEmailPreference();
}
catch (InvalidFormSubmissionException $exception)
{
$form->display(); 
}

Thus preventing an invalid EmailPreference object being created.

12

Hi…

Adding lot’s of abstract interfaces to decouple things has a cost in complexity. If you end up paying that cost building flex points you don’t use, then you’ve lost. Don’t add flex points you don’t obviously need. The world does not need another bloated “general purpose” framework.

With “value objects” (actually DataTransferObjects) you may find your self creating lot’s of versions of StdClass for very little gain. Using interfaces/classes as a type system, to trap errors such as reading things that could get deprecated, has a cost. Look at what systems you have in place already. Type systems and unit testing cover a lot of the same ground. If your testing is already thorough, trying to graft on a type system is going to be duplicated effort.

These are engineering trade-offs. Imagine I find a bug in the future and I have to figure out what’s going on - what’s going to soak up my time?

yours, Marcus

13

I would say both optional parameters and hashmaps as intermedaries in contracts at interface level are both code smells. Replacing one code smell with another better deemed code smell is still smelly.

The sense of code smell is something acquired through personal experience, it is not purely ideoligical as it is heavily tied to someones ability to sense technical debt which is also something that is quite hard to teach. If you work with someone quite closely you can help to teach them it by explaining how design decisions in the past are causing the pain they are experience on the task they are doing at hand now.

What is acceptable practice also depends on the size and complexity of the project. Some things are pretty innocuous and harmless at a small project size working on your own but cause collapse at a certain size point in say a team of 10. Seeing things collapse and get eye blindingly hard where originally that working pattern was simple and light is part of where this sense is attained from, it is attained from pain like a lot of learning experiences.

Passing an associative array is about the laziest thing that can be done especially the whole of the request array( I term it bucket programming as a hashmap is a data bucket of anything ), anything could be in it and the only way to collate the real hidden interface requirement of Account_Models_EmailPreference is to go off on an adventure through its internal implementation. Some developers are happy to do that when it is their own implementation as they can remember it most of the time as they typed it.

I follow a simple rule, if a class has to be opened and read more than a few times to be instanciated and used by people who did not write it then at a design level it has failed. It suffers from lack of clarity at interface level and if programming to interface versus implementation that is a real big issue.

An intermediary object with default values that is passed by type hinting == more effort today for one person today, better clarity for everyone forever.

One simple indication of possible code smell is lack of clarity as monsters lurk where it is not clear. If it is not clear it cannot be trusted. Forcing the opening of uneeded files also forces them to be hit in the face with code smells that are outside of there current remit as everything ends up smelling a bit as it gets old. Out of sight, out of mind.

Anyway I like this quote here
http://www.c2.com/cgi/wiki?CodeSmell
"
There are two major approaches to programming: [is there a page that covers this? there should be]

* Pragmatic: CodeSmells should be considered on a case by case basis
* Purist: all CodeSmells should be avoided, no exceptions

…And therefore a CodeSmell is a hint of possible bad practice to a pragmatist, but a sure sign of bad practice to a purist.
"

I am pretty purist, sometimes refered to by my colleagues as fundementalist, but I do take it case by case on most things. I also believe when confronted with a choice between two things I do not like to refuse both and come up with a third way that keeps everyone happy, even if it involves a lot of cross language research in my time for that case. For example PHP is hashmap/associative array happy, perhaps to a fetishistic degree so a completely fresh perspective from different external language communities may gain a better and much more elegant leftfield answer.

Eg. optional paramaters which are a parallel of overloading there is this

The creation methods document at creation point what the combinations of the original optionally passed values actually action at a human level.

After reusability at an application level there comes cross project and then cross framework etc. Different problems with increasingly required refined solutions to deal with increasingly bigger things over ever increasing time lines with a manageable effort level. Most of the books such as refactoring to patterns are a guide to maybe make this process more manageable. This software stuff is still a barely started story, requirements will always get bigger and more imaginative. That is what makes it so fun, it would be boring otherwise.

14

Where is the coupling between the controller and model? passing an array of arguments to the constructor could happen anywhere, there is no specific constraint that it has to happen in the controller. It could happen in a cron-job, an API call or any other use-case. I use this technique a lot and it actually increases reuse of model methods.

If you were passing the request object itself or anything that is controller related, then it would’ve been a different story.

Regarding passing an array to the constructor - once you have several parameters (3+), it is usually best to pass an array rather than depend on parameter order - what happens if you need to pass only the 4th parameter? do you pass nulls or any other default value? this always leads to complications.

15

I’m not saying you shouldn’t use them per se, it’s just that I find it harder to read code which accepts some array and “does stuff” with it, because you can’t see which values it actually expects to receive. Unreadable means it’s harder to debug and change, so passing in an array isn’t my cup of tea. Doesn’t mean I don’t use it at all, just that I’d rather not.

Good point.

It differs between situations. Do you have a real-life example of a method that takes an array with eight parameters? Chances are you could refactor them out into another class, or that the method is “doing to much” for it’s own good. I do agree with Ren though: the “smell” isn’t that stinky when it comes to the constructor.

16

Hi…

I’m not an advocate for anything in particular beyond code that can be easily understood and easily changed/fixed. I’m happy to use separate parameters, hashes or DTOs or a mixture.

The final call for those parameters depends on what they are (do they group into subgroups), the number, how often they change and environmentals such as the development process (testing, auditing).

Labelling one technique as “bucket programming” isn’t helpful here. What is normally a warning to be careful has become a reason to disqualify an option. One which might be perfectly appropriate for the situation.

Even a global can be the right answer sometimes.

Pejoratives are memorable labels to keep beginners from shooting themselves in the foot. We are already weighing up the options critically. Surely we don’t need them.

yours, Marcus

17

Why pass them in the constructor at all?


$foo = new Foo;
$foo->bar = 1337;
$foo->foo = 42;
18

Arguments to a constructor are better off being values or components that are vital to an objects function. request data in my view does not really fit either of those descriptions.

Care to elaborate on why do you think that? constructor arguments are usually used to configure the instance. In many cases, default values can be supplied. The bast way in PHP to pass configuration data of unknown size is using an array (or a configuration object that serves a similar purpose). In this way we specify a flexible interface to the class.

If we choose to use fixed parameters instead, the interface of the class would change as the class evolves. That would probably require changes in other parts of the application - which is why it less preferable to the more flexible array / config object method.

19

Not all coupling is inherently bad.

I disagree, it sure is.

The problem is, independent pieces of software are rarely useful beyond a very limited scope, so they must collaborate with other units of funcitonality in order to truly accomplish something practical.

Dependencies are unavoidable, and thus so is coupling to one degree or another. Do we want concrete coupling or abstract coupling? Almost always it makes sense to choose the abstract form, but edge-cases suggest sometimes concrete coupling is acceptable and a better choice. So we must determine, as professionals what and when to couple components and how tightly those components can be coupled.

Cheers,
Alex

20

+1 to above comment :slight_smile: