Is it just me who finds this poor style and potentially confusing:
<?php
function my_function($nodes) {
foreach ($nodes as $nid) {
// do stuff
}
}
?>To me, a variable names $nodes will be an array of nodes -- that is, node objects. If it's an array of nids, I would call it $nids to avoid confusion about what we have there.
I'm curious if other people agree (in other words, is it worth my time writing a patch for core or will it just lead to bikeshedding?)
In general, though, I think it's a good idea for variables to be named in such a way that describes what they hold; also for the same data to have the same variable name as it travels through functions. In other words, avoid this:
<?php
function foo() {
bar($ponies);
}
function bar($caterpillars) {
}
?>Unless there's a good reason, say if bar() is generic and can accept any animal. In which case, call its parameter $animals, obviously.
14 comments
Where did you see this
Where did you see this pattern?
Grep core! ;)
Grep core! ;)
I'd rather not. Where did you
I'd rather not. Where did you see this pattern?
Start from
Start from node_admin_nodes_submit(). The loop in particular is in node_mass_update().
Still in D7
I agree that this isn't good code. The variable $nodes should either be an array of node objects, or it should be renamed to $nids. The code is still in D7 by the way:
foreach ($nodes as $nid) {_node_mass_update_helper($nid, $updates);
}
yes
yes please, it is a code style bug.
Agree
Code should be descriptive. Don't use $nodes to describe $nids.
I have found it only once
I have found it only once (D6.14), but I agree that that this is not a good example of good variable naming.
----------------------------------------Find 'foreach ($nodes as $nid)' in 'C:\Data\Trash\drupal-6.14\modules\node\node.admin.inc' :
C:\Data\Trash\drupal-6.14\modules\node\node.admin.inc(369): foreach ($nodes as $nid) {
Found 'foreach ($nodes as $nid)' 1 time(s).
Search complete, found 'foreach ($nodes as $nid)' 1 time(s). (1 files.)
Yes to the first, no to the second
For the first point, yes. $nodes should be an array of node objects. $nids should be an array of node ids. This is a bug. Please file a patch. :-)
For the second, it's a good ideal to shoot for but is not always possible. The variable should have accurate meaning in the context in which it appears. That meaning may be different in different contexts. Consider a function that is recursive, which may have a $parent_id parameter. Whatever function initially calls that routine will not pass in something called $parent_id, but probably $starting_id, or $base_id, or maybe even $nid. It should make sense in the context in which it appears.
That said, if it's the same meaning it should be the same name. Eg, if in the function it's called $thing_list and in the caller its called $list_of_things, that's just someone being sloppy.
I think it's laziness and
I think it's laziness and inexperience.
I my experience, the younger and less experienced a developer is, the less importance he gives to things like variable naming and coding standards. You have to be the guy that goes back years later and has to fix the code to appreciate naming a node ID "$nid" and a node object "$node" instead of simply not caring and just using $x everywhere.
For the most part, Drupal core is very cleanly written and easy to understand. This is one of the reasons I chose to work with it over Joomla and some other systems where much less attention seemed to go into code maintainability (which is the long term benefit of this type of nit-picking :)
This echos the post I made a
This echos the post I made a month or two back about self-documented code:
http://www.advomatic.com/blogs/dave-hansen-lange/drupal-maintainability-...
my ?
I was thinking... PHP doesn't support 'my function' syntax, it's perl syntax...
Where this came from?
Oops. Consider the example
Oops. Consider the example pseudocode ;)
Meh. IMHO, if I read $nodes
Meh. IMHO, if I read $nodes by itself i'd assume it was an array of $node objects -- but since the very next line is "as $nid" this code is 100% crystal clear to me. If it was $nodes as $node where $node was in fact $nid -- that's another story.
I actually prefer $nodes to $nids in this case. $nids => $nid may as well be $ids => $id. While $nids is more accurate, i'd its much less meaningful being no more than a plural of $nid.
For the programmer from Mars at least $nodes offers clues:it will be easier for a martian to determine what the strange numbers referred to in $nodes = array(1,2) is vs $nids = array(1,2). Probably doesn't matter though, cause the martians will sooner or later have to accept that $node->revision_id is $node->vid, and that $node->published is $node->status and so on...
At least $comment->status = 0 no longer actually means a comment is live lol...
Post new comment