CODEWITHSUNDEEP
phptypesparameters
mOrloff
mOrloff

Reputation: 2617

PHP Best Practices: Should a given parameter always have a consistent type?

I have a function that accepts a checkGlossary bool param as well as an optional glossary array.
Their states are directly tied together.
The glossary is never required if the bool is FALSE, and inversely, it is always required if the bool is TRUE.

To me, it seems like this could easily be simplified from:

// Current
function doSomething($param1, $param2, $checkGlossary=FALSE, $glossary=NULL){
    // blah blah blah
    if($checkGlossary)
        array_search($glossary[$param2]);
    // etc etc etc
}

... to:

// Proposed
function doSomething($param1, $param2, $glossary=FALSE){
    // blah blah blah
    if($glossary)
        array_search($glossary[$param2]);
    // etc etc etc
}


My only hesitation is over the fact that the type for $glossary (bool or array) would be unpredictable.
It doesn't bother me as long as I'm not going against some Best-Practices guidelines.

Thoughts?

Upvotes: 3

Views: 836

Answers (2)

Till Helge
Till Helge

Reputation: 9311

It is always a bad idea to have function parameters with what PHP calls mixed datatype. It requires additional code in the function to check the type of the parameter and obviously it can get very confusing.

Probably the easiest solution in your special case would be to use the array length as indicator of whether to use the glossary code. You need a way to declare that the glossary array should not be used. So you should ask yourself: When is it pointless to use the glossary? When it's empty of course. So, I would suggest that you get rid of the flag and define array() as default for your other parameter:

function doSomething($param1, $param2, $glossary=array()) {
    if (count($array) > 0) {
        // do your glossary code here
    }
    // all the other stuff goes here
}

To me this seems semantically correct and works just fine.

I don't know what exactly you are building there, but another solution would be to put it all into a class and have the glossary as instance variable. In case you could use the glossary in several function calls. It would roughly look like this:

 public class SomeAccurateClassName {
     private $glossary = array();

     function setGlossary(array $glossary) {
         $this->glossary = $glossary;
     }

     function doSomething($param1, $param2) {
         if (count($array) > 0) {
             // do your glossary code here
         }
         // all the other stuff goes here
     }
 }

Considering that you basically have a state (use glossary or don't use glossary) it might be a good idea to encapsulate that withint a class.

Upvotes: 2

vcampitelli
vcampitelli

Reputation: 3242

You can use is_array() and is_bool() to check its type!

Upvotes: 2

Related Questions