TalkPHP - View Single Post

xenon · 12-13-2007, 10:35 PM

I mentioned, and highlighted good in "good style". What's wrong with that code? Hmm let's see...

Code:

gen_sort_selects($limit_days, $sort_by_text, $sort_days, $sort_key, $sort_dir, $s_limit_days, $s_sort_key, $s_sort_dir, $u_sort_param);

Skipping that function which takes an enormously large number of parameters (instead of using some setters in a class for example), we reach this nifty init:

Code:

// clear arrays
$id_ary = array();

The comment is so useful here, that it might aswell be omitted. $id_ary is never used up to that point, so there's nothing to clear but the mind of the developer who wrote that. You might want to search other files, perhaps it's defined somewhere else, but it's not.

Up to this point, you might have reached about 20 trigger_error statements and 50+ nested if's (just in the /search.php file). I'd say that's a real improvement to the code (not).

Something really captured my attention. This:

Code:

// We do some additional checks in the module to ensure it can actually be utilised
    $error = false;
    $search = new $search_type($error);

    if ($error)
    {
        trigger_error($error);
    }

    // let the search module split up the keywords
[...]

I'll let you decide what's wrong with phpBB's source code (except the fact that modifying it is a free ride in hell).

12-13-2007, 10:35 PM	#17 (permalink)
xenon The Frequenter Join Date: Dec 2007 Location: Bucharest, Romania Posts: 438 Thanks: 3	I mentioned, and highlighted good in "good style". What's wrong with that code? Hmm let's see... Code: gen_sort_selects($limit_days, $sort_by_text, $sort_days, $sort_key, $sort_dir, $s_limit_days, $s_sort_key, $s_sort_dir, $u_sort_param); Skipping that function which takes an enormously large number of parameters (instead of using some setters in a class for example), we reach this nifty init: Code: // clear arrays $id_ary = array(); The comment is so useful here, that it might aswell be omitted. $id_ary is never used up to that point, so there's nothing to clear but the mind of the developer who wrote that. You might want to search other files, perhaps it's defined somewhere else, but it's not. Up to this point, you might have reached about 20 trigger_error statements and 50+ nested if's (just in the /search.php file). I'd say that's a real improvement to the code (not). Something really captured my attention. This: Code: // We do some additional checks in the module to ensure it can actually be utilised $error = false; $search = new $search_type($error); if ($error) { trigger_error($error); } // let the search module split up the keywords [...] I'll let you decide what's wrong with phpBB's source code (except the fact that modifying it is a free ride in hell). __________________ I have optimistic thoughts, even though sometimes (if not always) life's a bitch.