[development] LinksDB vs. Links

Wed Aug 2 13:35:11 UTC 2006

On 2 Aug 2006, at 14:12, Morbus Iff wrote:

>> You must be looking at a very early release then.  I _AM_ using 
>> db_query properly (I have been as soon as I discovered that it could 
>> be used like printf), and I _AM_ using database prefixes.
> >
> > At least get your facts straight before you start slating me.
>
> Actually, the only reason you're using database prefixes is because 
> you went through and added them all shortly after I sent that message. 
> (?r1=1.1.2.11&r2=1.1.2.12, Wed Aug 2 08:00:47 2006 UTC).

That's when I checked that version in - I had done the changes a long 
long time ago.

OI have also joined head and 4.7 branches

>  * You've got an empty @file declaration.

Not put there by me - what is it?

>
>  * You're passing an unused array of string
>    replacements to t() in linksdb_help().

Copied and butchered from someone else's module - blame them not me.

>
>  * You're using HTML in the help of admin/settings/linksdb, which
>    means it has to be translated separately even though it's the
>    same string as the one preceding it. '<p>'.t().'</p>' instead.

Noted - I shall update.
>
>  * Your code doesn't follow the Drupal style guidelines at all.
>    http://drupal.org/node/318. Not mentioned there is an in-bred
>    "spaces after commas", which you love to not do.

That's pure prettiness
>
>  * You global $user in linksdb_menu() and you don't have to.

The module I learned from had it, and so do I.

>
>  * Your use of weights in linksdb_menu(), while they certainly
>    accomplish something, is abnormal. You never define weight 0,
>    default local tasks are set to -10, and the tabs usually are
>    ordered by declaration order, not manually defined weights.

I tried that and it seemed to re-order them alphabetically, which is 
certainly not what I want when the first one begins with V...

>
>  * MENU_NORMAL_ITEM is implied, and doesn't need to be specified.

I didn't know that - thanks, I shall remove them.

>  * You use "links/category/edit/4" when it really should be
>    "links/category/4/edit". Even with that said, you use arg(3)
>    in your linksdb_categories_edit() when you should be using
>    the naturally passed callback arguments of the menu (where
>    you'd use linksdb_categories_edit($cid)). You use arg()
>    throughout the module when you really shouldn't be.

That's not what I saw when learning - maybe it has changed in 4.7?

>
>  * You don't predeclare a bunch of things, such as $form in
>    linksdb_report() - predeclare it as $form = array(); first.

Noted.

>  * You don't do sanity checking - you assume
>    that arg(2) exists and is a number.
>
>  * You spelt "concise" wrong, and your sentence has no period.

Pedant

>
>  * You don't know how to use drupal_get_form(). If you give it
>    an ID of "reportform", then it'll happily use "reportform_submit".
>    However, instead of naming it "linksdb_report", you override this
>    automatic/free feature by forcing a callback function of "linksdb_
>    report_support", which is unnecessary.

The documentation seems to be sadly lacking in how that side of things 
work - I just used examples and got it working as best I could.

>
>  * No standardization in your code. You use $fid, $v for linksdb_
>    report_submit, $formid, $v in _moderate_submit, and $form_id,
>    $values in your _edit_submit. They're all the same thing.

It depends what mood I'm in and it doesn't affect how the code works in 
any way.  Also, as I learn new tricks in drupal the variables sometimes 
change with it.

>
>  * Your SQL isn't capitalized.

Pedant

>
>  * You rarely need to use drupal_goto anymore, and
>    almost never in the _submit hook for FormsAPI.

Except when you don't want the URL to contain the data you passed as 
pathinfo after you have submitted

>
>  * Your SQL queries will not work on PostgreSQL. It needs
>    db_query('word "%s" word') not db_query("word '%s' word").

PostgreSQL is not something I have access to for testing, and have no 
knowledge of whatsoever.

>
>  * You use double spaces after sentences in _report_submit.

I am english.  I use english grammar.

>
>  * Listen, I know I said you shouldn't use drupal_goto a few bullets
>    ago. That doesn't mean you go and use drupal_set_header with a
>    Location: instead. Man alive.

Does drupal_goto  work with external URLs?

>
>  * Oh, and you're passing ; at the end of most of
>    your SQL queries. You don't need to do that either.

Force of habbit as it is what I was taught to do on SQL*PLUS back in 
the early 90's

>
>  * You use "'&lt;Root&gt;'" in linksdb_deadlinks, and '<Root>'
>    in linksdb_suggest. Which is it? (For what it's worth, it should
>    be '<'. t('root') .'>', per Drupal core).

If I'd know that I'd have done it.
>
>  * To prevent things like "select * from {links_links} where dead=1
>    order by {links_links}.name", use table abbreviations, like:
>    "select * from {links_links} ll where dead=1 order by ll.name".

Why? I'm happy to type the full name, and it makes it far more readable 
later.  I personally abhor the use of table abbreviations.

>
>  * if(variable_get('linksdb_show_report',1)==1) - you don't need ==1.

True, but it sometimes is clearer on the reading of the code if you're 
specific that you want that value.

>
>  * linksdb_deadlinks doesn't translate the report messages.

My bad

>
>  * You use "links/report/$link->id" and then 'links/refer/' .
>    $link->id. Which is it? Concatenation or interpolation?

It depends on my mood - they both do the same job.

>
>  * Oooh, you do know how to predeclare $form = array() in _suggest().
>    I guess you must have made sloppy mistakes all the other times.
>    Story of your life, eh?

This is my first module - I am learning the best ways to do it.

>
>  * "({name},{url},{description},{active},{category})" - I have no clue
>    what you're even attempting there. Column names aren't prefixed.

yeah, I know, and they're already gone.  I was getting a bit 
overzealous with my prefixing the other day.

>
>  * Is it site staff or site administrators? You use both.

Which do you suggest?  I'm happy with either.

>
>  * theme_link() - Gah, you don't use theme('image') here.

theme whattage?

Never heard of it.  What does it do?

>
>  * Is it "Link" or "link"? Previously you have "Report
>    this link", but then you have "Edit Link".

Pedant

>
>  * $cat->shown=99999999;. Right. Heh.

why not? it does the job, and is nice and simple.

>
>  * You seem to use the "Report this link as dead"
>    thing a lot. That should be made a function.

Could do I guess.

>
>  * # for comments? What is this, Perl?

Why not? they're perfectly valid.  And perl is the other language I use 
day in and day out so I just stick to one comment style.

>
> And so on. But I'm getting bored.

Why not post some issues instead of slating me in public mr 'I'm oh so 
perfect and have never made even a slight slip in my life when learning 
a new system'?

>
> -- 
> Morbus Iff ( drowning in data, bereft of knowledge. )
> Technical: http://www.oreillynet.com/pub/au/779
> Culture: http://www.disobey.com/ and http://www.gamegrene.com/
> icq: 2927491 / aim: akaMorbus / yahoo: morbus_iff / jabber.org: morbus
>
> Scanned by MailDefender - managed email security from intY - 
> www.maildefender.net
>

Information in this electronic mail is confidential and may be legally privileged. It is intended solely for the addressee. Access to this mail by anyone else is unauthorised. If you are not the intended recipient any use, disclosure, copying or distribution of this message is prohibited and may be unlawful. When addressed to our customers, any information contained in this message is subject to intY's Terms & Conditions. Please rely on your own virus scanning and procedures with regard to any attachments to this message.

Scanned by MailDefender - managed email security from intY - www.maildefender.net