This is part two of my series, The DX Files: Improving Drupal Developer Experience.
Many Drupal APIs accept a boolean argument (TRUE or FALSE) to determine some behavior. I believe that practice should be banned in all but exceptional cases, instead using a defined constant with a descriptive name.
Here is a perfect example from Drupal core:
<?php
$output = node_view($node, FALSE, TRUE);
?>Now, quick! Who can tell me what passing FALSE as the second argument and TRUE as the third argument means? Unless you are a Drupal guru, you almost certainly have no idea. Compare the previous line of code with this one:
<?php
$output = node_view($node, NODE_FULL_VIEW, NODE_IS_PAGE_CONTENT);
?>Now it is much more clear what is going on. We're displaying the full view of a node and it is the page's primary content. Perhaps you do not know exactly what those constants mean ("A 'full view' as compared to what?", you might ask) but you are certainly better off than if you just see "TRUE" or "FALSE". If you later encountered
<?php
$output = node_view($node, NODE_TEASER_VIEW, NODE_IS_NOT_PAGE_CONTENT);
?>everything would become pretty clear(*).
The end result of this change would be an API that is easier to learn and code that is easier to read, understand, and maintain.
* Disclaimer: I'll grant that it is far from obvious exactly what NODE_IS_PAGE_CONTENT actually causes to happen and in fact in a recent query on #drupal, several senior Drupal developers (myself included) couldn't remember, but it is still way better than "TRUE". This is an unrelated issue with node_view() that ought to be improved.
Comments
I kinda have to disagree
I kinda have to disagree with you in terms of making code easier to write, I would say it would cause more harm than good. When typing out code it is much easier to type true or false and most color coding editors make it easy to notice a typo, not the case if we have defined constants. Secondly, if it is a boolean argument with only two options, why go through the trouble to remember both options names instead of just true or false? I really don't like the idea of having to remember defined constant names for every single api function.
Recently there was api change from 5 to 6 for the l and url functions which made it easier to type out parameters because many people who wanted to display html within the links needed to add all the parameters to turn the last one true (enabling html in links) and now instead the third argument is just an array, which has keys that are equal to the parameters from the previous api version. In this case I like how they consolidated the last 5 arguments into a single keyed array.
l API 6.x http://api.drupal.org/api/function/l/6
l API 5.x http://api.drupal.org/api/function/l/5
And this is probably even
And this is probably even better not for hardcore Drupal developers, but people just starting out with coding Drupal, or just dabbling, who don't breathe the API and thus have to look things up constantly. You make quite a good argument.
In regards to my previous
In regards to my previous post, while I disagree that it makes it easier to write/remember, I will agree that it could make code more readable by new comers who may find that more attractive. However, we do have a very verbose and friendly api website that makes it quite easy to look this information up.
I like this idea. My other
I like this idea.
My other pet peeve is the need to remember that NODE_TEASER_VIEW comes before NODE_IS_PAGE_CONTENT. Could we, perhaps, adopt the following calling style? (And whatever argument-parsing libraries are needed to make it easier to implement and document?)
<?php$default_output = node_view($node);
$teaser_output = node_view($node, array(NODE_TEASER_VIEW));
$page_teaser_output = node_view($node, array(NODE_IS_PAGE_CONTENT, NODE_TEASER_VIEW));
?>
This is essentially the same as the design of l() in Drupal 6 -- which now has an $options array instead of a list of arguments -- and it's intended to solve the same problem all across the codebase. Who among us, when working with the Drupal 5 version of l(), has not struggled to remember whether $html is the sixth argument or the seventh?
So... the idea here is to
So... the idea here is to make peoples' code more readable? It seems, though, that at least when writing the code this wouldn't help much b/c you'll still have to reference the API docs just to find out what constant you're supposed to use.
And just b/c I'm proud of him, Lyle knew the arguments. ; )
Is this not a reversal of position? --> http://jaspan.com/dx-files-improving-drupal-developer-experience#comment...
Not so sure about this.
Not so sure about this. Modern editors have function help for this. They reuse all our nice doxygen too. This is going to add lines of code (namely defines), and still requires knowing the position of each argument. So it is a bit of a wash IMO.
We seem to be moving towards
We seem to be moving towards associative arrays for "the passing of boolean flags as arguments", rather than constants. E.g:
http://api.drupal.org/api/function/l/5
vs
http://api.drupal.org/api/function/l/6
Personally, I prefer associative arrays to constants, when it comes to argument passing. They yield the same benefit of improved readability. They don't occupy the global namespace. And you can pass them in whatever order you want (and you can easily omit whichever ones you want). Anyway, Barry, I do at least agree with your main point, which is that the current de facto standard is horribly developer-unfriendly. Death to
call_func($stuff, FALSE, FALSE, array(), NULL, TRUE, '', TRUE, 0)!I also disagree. There are
I also disagree.
There are *so* many places where we pass booleans. If you'd define constants for all of them, then there'd be *a lot* of constants to look up. But then again: both in the case of TRUE/FALSE and constants, some editors will show the documentation, for them it won't matter.
It does of course increase the readability.
Finally, Ryan has a very sharp eye, it seems you've changed your mind about this?
I only use constants when
I only use constants when using bitwise arguments.
ie: node_view($nid, NODE_IS_PAGE_CONTENT | NODE_TEASER_VIEW)
Especially in cases where the properties need to be extendable via code (
advanced error handling and logging code for instance, where you can trigger multiple errors
in a single call, and modules can define additional error types).
It also means you have to test them using if ($prop & NODE_IS_PAGE_CONTENT)
If I may sum up the score so
If I may sum up the score so far:
Several people dislike defined constants. Alas, that's not a real surprise given the reception to http://drupal.org/node/260226
So let me relentlessly focus on the positive: Several people seem to like arrays of boolean flags a la http://api.drupal.org/api/function/l/6 . I like those, too, as I've already said. This:
<?php$page_teaser_output = node_view($node, array('node_is_page_content', 'view' => 'teaser'));
?>
improves readability, even without the define()s.
Of course, with defined constants there was a chance that the compiler would warn you when you forgot a name and found yourself typing NODE_IS_PAGE instead of NODE_IS_PAGE_CONTENT. When you use strings as flags you don't get such a warning: instead you get silent failure. You could have every function carefully check its arguments to make sure that every flag in the argument array is a recognized flag... but that would consume runtime resources on every function call, so that's no good. I guess the other alternative is to do very thorough testing.
Several people point out that function calls with named boolean flags aren't necessarily much easier to write than the old giant-list-of-arguments style, because you might still have to consult the api.drupal.org page just to remember the names of the flags. This is probably true for some people. (I, myself, find it easier to remember names than to remember the specific order of a bunch of arguments, but everyone's brain is different.) But even if this change doesn't make the code easier to write, it will make it easier to read. Which is at least as important, and arguably much more important, since Drupal code is (we hope) read dozens of times more often than it's written.
Post new comment