Is this proper coding?

[Boofo]

Can someone tell me in their opinion if this is proper coding? The following code is the fix for the division by zero error.

I am replacing this:

if ($totalthreads == 0)
{
    $totalthreads = 1;
}
if ($totalposts == 0)
{
    $totalposts = 1;
}

With this:

$totalthreads == 0 ? $totalthreads = 1 : $totalthreads; 
$totalposts == 0 ? $totalposts = 1 : $totalposts;

It works, but I am curious as to what your feelings are as to whether this is actually a proper way to do it or not.

Can anyone tell I got bored enough to do this?

[Boofo]

No opinions one way or the other?

[Lynne]

I don't know which is better. When I 'read' code, I like it better the first way - it just looks cleaner to me.

[Boofo]

I agree somewhat that it is easier to understand what the code in the first example is doing. But I also like the second way as it condenses 8 lines into 2.

[Lynne]

For single 'actions' to a condition, I actually like it like this:

PHP Code:

if ($totalthreads == 0) $totalthreads = 1; 
if ($totalposts == 0) $totalposts = 1; 


[Boofo]

Yes, that would be the better way to do it for single actions. I think I will use that instead of my way since I don't really need the else part of it for this particular code. Thanks for pointing that out.

I guess I'm just used to the brackets being there.

[Disasterpiece]

I can trump that.

PHP Code:

if (!$totalthreads) $totalthreads = 1; 
if (!$totalposts) $totalposts = 1; 


PHP Code:

$totalthreads = max($totalthreads,1); 
$totalposts = max($totalposts,1); 


[Boofo]

Originally Posted by Disasterpiece

I can trump that.

PHP Code:

if (!$totalthreads) $totalthreads = 1; 
if (!$totalposts) $totalposts = 1; 


PHP Code:

$totalthreads = max($totalthreads,1); 
$totalposts = max($totalposts,1); 


Will you explain the logic behind that? Also, how would you do the following using max?

if ($vbulletin->options['something_here'] == '') $comma_separated = '0';

[YankForum]

Originally Posted by Disasterpiece

I can trump that.

PHP Code:

if (!$totalthreads) $totalthreads = 1; 
if (!$totalposts) $totalposts = 1; 


PHP Code:

$totalthreads = max($totalthreads,1); 
$totalposts = max($totalposts,1); 


nice one , but the most important question here is which one will be processed faster in php? in my opinion the second proposed code by boofo should be the fastest since it only takes 2 values true or false??? of course as lynne says the first one is more readable and easy to customize , i like it the most
nice thread , thanks boofo

[Disasterpiece]

Originally Posted by Boofo

Will you explain the logic behind that? Also, how would you do the following using max?

if ($vbulletin->options['something_here'] == '') $comma_separated = '0';

in php you have automatic typecast, so everything evalueates to either true or false. Zero (numeric) evaluates to false, so the ! inverts the evaluation to "true" and therefore you set the totalthreads to 1. It's just that it looks nicer because it's short :P
if it takes one or more additional cycles to process it? maybe. But it's php after all, so the uber-performance isn't that of an issue.

max is a mathematic function which returns the greater value of the given params. If you don't want to have a number which is negative or equal to 0, you use max($n, 1) so you guarantee that you always have at least 1.
Not sure about the performance, It's possible that the php engine uses the native c math function, so this might even speed things up a bit? dunno but then again, doesn't really matter :P

Just intended to brag with my mad php skillz, honestly I think the idea itself is an issue here (it throws an error when it's zero? well, just increase it then. <- wtf?)
If you really want to get help, you should explain your problem instead of the solution

PHP Code:

if ($vbulletin->options['something_here'] == '') $comma_separated = '0'; 


I have nothing to add. maybe use "empty()" instead of testing for empty string, since the empty function checks for null as well and doesn't throw a warning when the value is not set.