Mission Clean Code

Freso,

This is a good idea. I've looked through the patch and it all seems good, but I have not actually applied it (as I have already made some mods to my installation of pingback which I will be suggesting in another post). I may take a fresh download and test out the patches, and post here again.

One thing I noticed - you missed one call to dpm in pingback_send()

It would be good to get this done and committed into the dev release, as it is a great module but does need tidying up. I can then make further suggestions when the bulk of these code changes are in place.

Jonathan

Excellent work. I have applied the patch to a fresh 2.x-dev install and it all looks good. I have also tested it and it works just the same, so I do not think any accidental errors have crept in. I have noticed the following, which you may like to include, just to make things even cleaner and more to standards:

1. In pingback.module, function pingback_send() there are three calls to dpm(), lines 376, 387, 392 which should be removed

2. in pingback_form_alter, line 69, there is the following code:

//hide pingback input format if desired for anon users
  elseif (
    $form_id == 'comment_form'
    && (!$user->uid)
    && variable_get('pingback_hide_format_for_anon', 0)
    //&& (isset($GLOBALS['pingback_bypass_format_hiding']) ? !$GLOBALS['pingback_bypass_format_hiding'] : TRUE)
  ) {

Is this the correct way to format according to coding standards? Should the tests be on the same line if they are short?. Also it includes a commented out test in the middle of the if construction, which I think should be removed (and placed in a separate comment for refernce if/when the bypass code is implemented)

3. pingback_exit() and pingback_xmlrpc() both need the standard comment block, ie

/**
 * Implementation of hook_... ().
 */

4. Line 322, comment /* --- theme_pingback_* --- */ should be improved. Also it might be good to move this definition to just after pingback_theme(), so that it is clearer that they are related.

5. Line 435 the comment “private_functions” is a bit misleading. Many of the functions are named with a leading _ but also many are not. Just remove it, as we are not going to rename any functions.

6. In pingback.admin.inc there is a very long comment starting ‘// to devs: every node update’ which appears three times, lines 36, 57 & 85. The second are third of these are not required, seems like they got copied accidentally. For the first one, is this the proper way to write such a comment on a single line where it is very hard to read? Maybe better in a comment block?

Take your pick as to which if any of the above you include, but obviously I hope you do all of them. This is a great clean-up. When it is done in dev, I can then share some fixes and improvements I have made, but it will be good to get this done first.

Jonathan

Great work guys. I'm on vacation at the moment, but I will commit this when I get to DrupalCon next week.

Thanks Freso, I've just re-applied the patch. Here are my responses:

1. Yes it is fine. I don't know why I thought they were still there, I must have still been looking at the first patch. Maybe I got confused with your file numbering which started with suffix 1, then had no suffix for the second patch ;-)
2,3,4,5 All good now.
6. My main point was that this is a comment that has been accidentally duplicated when copying lines to make new form items. The comment only makes sense on the pingback_mode entry so you could delete it from the other two. But I'm not going to argue too hard over this!

Yes the other little clean-ups are fine. Good work!

Jonathan