Bizarre code

[timnp]

We're running Version 3.6.8

In includes/functions_reputation.php Line 101 we have the following

$reputation_value = ($reputation_value - ($reputation_value - 500)) + (($reputation_value - 500) / 2); 

What the?

Surely, we can at least reduce ($reputation_value - ($reputation_value - 500)) to "500"?

[StrongMotive]

I have the same to - the comment explains before what it is

if ($reputation_value > 500)
{ // bright green bars take 200 pts not the normal 100
$reputation_value = ($reputation_value - ($reputation_value - 500)) + (($reputation_value - 500) / 2);
}

[timnp]

Well I think the simpler:

$reputation_value = ($reputation_value / 2) + 250

Has the same effect.

Remember that this is inside a test for if $reputation_value is above 500 anyway.

I'm pretty happy with what it is actually doing (although the reason I noticed it is because I'm about to rewrite it), but it's a stupid line of code. A waste of my precious CPU cycles!

[timnp]

Also, I just found this at line 82 - 86

    if ($post['reputation'] == 0)
    {
        $reputationgif = 'balance';
        $reputation_value = $post['reputation'] * -1;
    } 

Now here's the question, if $post['reputation'] is indeed zero, then why are we multiplying it by -1 before we store it in $reputation_value?

[timnp]

Anyone from vBulletin want to take this one?

[Steve Machol]

Sorry, I don't understand your question. If you are having a specific problem, what is it?

[timnp]

I have several examples of some inefficient code in vBulletin and I didn't even look very hard, I could probably have a look for a load more. I was wondering if there is any logic to these sections of code or if vBulletin were planning on tidying them up for the next release?

[Steve Machol]

Sorry, I'm not a programmer. If you are having a specific problem that you believe is caused by this code, then please report this in the 3.6 Bug Tracker here:

http://www.vbulletin.com/forum/project.php?projectid=6

[timnp]

Thanks. I have posted about this over there. ( http://www.vbulletin.com/forum/proje...?issueid=23466 ) The specific problem is that although this code works and does not cause faults, it is written in a very bad way which could be easily improved.

Code written like this makes it very difficult to debug, awkward to write plugins for and time wasting trying to figure out what bits of code do (especially when the answer is "nothing"), it also wastes the CPU cycles in my server (although in this instance, hardly by very much but every little helps).

There are far worse examples in vBulletin but these are the ones that struck me as particularly inexcusable. I mean really, what is the point of multiplying 0 by -1, any school kid knows that's always a big fat zero!

[Analogpoint]

There are far worse examples in vBulletin but these are the ones that struck me as particularly inexcusable. I mean really, what is the point of multiplying 0 by -1, any school kid knows that's always a big fat zero!

If there are indeed "far worse examples", you need to post them so they can be fixed. If there aren't, you need to control your hyperbole. And FWIW, I agree things like this need to be fixed, even though they probably don't affect performance in a noticeable manner.

[timnp]

If you like, I'll post them as I find them?

I haven't done a lot of hacking around with vBulletin code itself, the first file I really looked into was the functions_reputation.php file and these immediately stood out.

A colleague of mine has already pointed out some of the problems with unoptimised queries here http://www.vbulletin.com/forum/showthread.php?t=246692

[Analogpoint]

If you like, I'll post them as I find them?

Hmmm, that's what I thought . But yes, if you find any more, you should post them.

A colleague of mine has already pointed out some of the problems with unoptimised queries here http://www.vbulletin.com/forum/showthread.php?t=246692

Looks like Keloran dissapeared from that thread, because he couldn't actually show a specific query that could be improved.

[Keloran]

sorry no, i had to go into recovery, i started to lose my mind finding so many, ill list them all once i have the full capabilitys of my mind back

thanks alot for putting me in therapy

[timnp]

This block of code appears roughly 8 times in postings.php

        if (can_moderate($threadinfo['forumid']))
        {
            print_no_permission();
        }
        else
        {
            eval(standard_error(fetch_error('invalidid', $idname, $vbulletin->options['contactuslink'])));
        } 

I think it should be in a function

[timnp]

Okay so this is a bit weird,

In includes/functions.php we have this bit of code... (note the comments)

/**
* Essentially a wrapper for the ternary operator.
*
* @deprecated    Deprecated as of 3.5. Use the ternary operator.
*
* @param    string    Expression to be evaluated
* @param    mixed    Return this if the expression evaluates to true
* @param    mixed    Return this if the expression evaluates to false
*
* @return    mixed    Either the second or third parameter of this function
*/
function iif($expression, $returntrue, $returnfalse = '')
{
    return ($expression ? $returntrue : $returnfalse);
} 

So, if this is deprecated how come its used a stunning 39 times within the same file that advises against its use! What is going on?

I searched to see how many times it appears in the whole of vbulletin. Over 1000!! MY EDITOR GAVE UP SEARCHING!

This is bad practice and is slowing vbulletin down, you're making it run a function that runs a command thats built into the PHP core when you could just run the core command anyway. What baffles me is that whoever wrote the comments on this function is clearly aware of this but no one has bothered to go through and look for where it is used.

Happy Christmas!