TalkPHP - View Single Post

wGEric · 12-13-2007, 11:34 PM

Quote:

Originally Posted by xenon

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:

You're still going to have a large list of parameters unless you make global variables or some other method. Each of those parameters can be changed while searching so you can't hard code them. Minimum requirement is PHP 4.3.3 so they can't use special PHP 5 functions.

Quote:

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).

register_globals? Always set your variables before using them so that nothing unexpected can get injected. Especially into your arrays. So if someone attempted to inject something within the array it would get cleared.

Quote:

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).

Let me highlight this line for you:

Code:

    $search = new $search_type($error);

$error is used when creating the object for the search. It is being passed by reference so this works perfectly fine. Yes it is confusing to look at at first but there's nothing wrong with it.

I think you should understand what the code is doing before you comment on it.