Following a recent discussion between Morbus and myself in #drupal, which came about from the latest drupaltoughlove.com review, the opinion is that all Drupal modules should be using elseif instead of else if. This is because it's "implied" by the first example on indentation in http://drupal.org/coding-standards. Drupal core itself is almost 50:50 on which it uses. However core isn't perfect and thoughts are that we should follow the example on the coding standards doc - even though it doesn't explicitly say it one way or another.

I'd like to hear other people's opinions on this. I've attached a patch that will check for "else if" and suggest using "elseif" instead, should that be the final outcome.

Cheers,
Stella

CommentFileSizeAuthor
coder_elseif.patch834 bytesstella

Comments

stella’s picture

stella commented

see also http://drupal.org/node/282405

damien tournoud’s picture

damien tournoud commented
Status: Active » Needs review
nancydru’s picture

nancydru
she/her/hers
Primary language English
Location Boston
commented

Frankly, I think both structure are wrong and I wish PHP didn't allow either. They both cause readability issues for humans. IMNSHO, the correct way to do it is:

  if (...) {
    ...
  }
  else {
    if (...) {
    }
  }

And if the IF statements are all concerned with the same variable, I would much rather see SWITCH used.

But what do I know?

stella’s picture

stella commented

I agree with you that there are some cases where using switch is a much better choice. However, I'm totally against your preferred method of nesting ifs like in the example above. I find it much harder to read and understand, and also think it is a lot more error prone as a result. Obviously, if you want to do the above, then you can as coder can't check for such things, but I really do find it quite ugly. Sorry, no offence intended.

Cheers,
Stella

nancydru’s picture

nancydru
she/her/hers
Primary language English
Location Boston
commented

Interesting because I find it easier to read and understand. Maybe it's because I grew up with many other languages that don't allow the same constructs that PHP does.

douggreen’s picture

douggreen commented
Status: Needs review » Postponed

If you want to pursue this further, please get it into the coder-standards. Thanks!

stella’s picture

stella commented

Well according to the discussion so far on http://drupal.org/node/282405 they seem to think it's already part of the coding standards, even though it's not explicitly stated in my mind, and it looks like Dries and others are in favour of that patch. I agree though, I think the coding standards should be made clearer.

Cheers,
Stella

douggreen’s picture

douggreen commented
Status: Postponed » Active

Feel free to write a rule, it's pretty simple. When #282405: Enforce coding convention on "elseif" settles, we'll commit this.

stella’s picture

stella commented
Status: Active » Needs review

Patch already attached in original post :)

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

FYI, the else if => elseif patch just got committed to core, so it's safe to assume this is "official" now.

morbus iff’s picture

morbus iff
they/them
commented
Status: Needs review » Reviewed & tested by the community

* Why is "else if" captured (via the parenthesis?).

Minor detail. Could go in either way.

douggreen’s picture

douggreen commented
Version: 6.x-1.x-dev » 6.x-2.x-dev

I removed the parens around "else\s+if" in the regex, I added a test case, and then I committed to the 2.x version.

morbus iff’s picture

morbus iff
they/them
commented

Can we get a clear statement on which version of coder to use and, if it's the non-existent 2.0, what the updates to the API are and when 2.0 will be released? Since this is now part of the existing coding standards (or, rather, always has been but is only now truly codified), sending this only to the dev-only 2.x is a bit annoying. Having developed a tougher-set-of-reviews module based on coder 1.x, I feel like I'm a bit in the lurch here on where I should be applying my continued efforts (since I've already been patching 1.x manually for, say, the concat of quotes issue).

douggreen’s picture

douggreen commented
Status: Reviewed & tested by the community » Fixed

If you're testing these patches, the version to use is the 2.x version. I created a release for it, http://drupal.org/node/314393, I'm not sure why it's not showing on the project page. If you're just a user, then the latest stable 6.x-1.0 version.

I'm working towards an official release, but not sure when that will be. Morbus, if I created a 2.x-1.0-BETA1 today, would that help? If we got the first 2.x stable out in the next week or two, would that satisfy you?

BTW, I forgot to mark this fixed.

morbus iff’s picture

morbus iff
they/them
commented

Yep, those would both be fine but I don't even need a beta or a stable. I just need an updating list of changes to the API - I'm perfectly fine with keeping my code updated to the latest dev if there are handholds on how to do it.

Anonymous’s picture

Anonymous (not verified) commented
Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.