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:

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.
if (!$totalthreads) $totalthreads = 1;
if (!$totalposts) $totalposts = 1;
$totalthreads = max($totalthreads,1);
$totalposts = max($totalposts,1);

[Boofo]

I can trump that.
if (!$totalthreads) $totalthreads = 1;
if (!$totalposts) $totalposts = 1;
$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]

I can trump that.
if (!$totalthreads) $totalthreads = 1;
if (!$totalposts) $totalposts = 1;
$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]

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


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.

[Boofo]

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

I think the second code (the true or false) is actually easier to read, in my opinion. I still can't get uses to the idea of not using brackets (like in Lynne's code) around if statements. It just seems to be open to me and not finished, even though it is, if that makes any sense.

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


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.

There is no problem. Just curious as to what other coders think.

[kh99]

If you're going for shortest code, how about:
$totalthreads += !$totalthreads;
$totalposts += !$totalposts;



Anyway, I'm not a php expert but I think I'd go for readability unless it's in a critical area where you're specifically worried about the speed. If the point is to make sure $totalthreads and $totalposts are a positive number, I'd probably go with
if ($totalthreads < 1)
{
$totalthreads = 1;
}
if ($totalposts < 1)
{
$totalposts = 1;
}


even if it's not "supposed" to ever be less than 0. (ETA: or on second thought maybe using max() would make the intention more obvious).

[Disasterpiece]

If you're going for shortest code, how about:
$totalthreads += !$totalthreads;
$totalposts += !$totalposts;

damn, you got me there :(

Altough I could argue, that the addition is always called and therefore produces more cycles then a comparison :P

However, this is way too little code to actually perform "real" optimization. In the end, it's the coders preferrence which is important and easier to read for himself. There are more grave issues of coding-crimes where I sometimes like to kick the developer in the face; how if/else and brackets are placed doesn't belong in this category tough.

[Boofo]

If you're going for shortest code, how about:
$totalthreads += !$totalthreads;
$totalposts += !$totalposts;



Anyway, I'm not a php expert but I think I'd go for readability unless it's in a critical area where you're specifically worried about the speed. If the point is to make sure $totalthreads and $totalposts are a positive number, I'd probably go with
if ($totalthreads < 1)
{
$totalthreads = 1;
}
if ($totalposts < 1)
{
$totalposts = 1;
}


even if it's not "supposed" to ever be less than 0. (ETA: or on second thought maybe using max() would make the intention more obvious).

But which was would be the fastest and less intensive?

[kh99]

Altough I could argue, that the addition is always called and therefore produces more cycles then a comparison :P
I agree.

... how if/else and brackets are placed doesn't belong in this category tough.
I also agree with that - I didn't mean to advocate any specific style. I guess my feeling about Boofo's second example in the OP is that, while it's isn't incorrect, my preference is to use the trinary (.. ? .. : ..) operator in places where you are interested in the resulting value, and the use of it for the "side effects" makes it a little more difficult to read and probably doesn't improve the efficiency. (Which I guess is pretty much what Lynne already said).

But which was would be the fastest and less intensive?
That I don't know anything about, for php at least. :o

[Boofo]

vb 4 use max in quite a few places in the code, which I had never noticed before.

It all really boils down to what we get used to in coding styles, I guess. There are always newer and better ways to do things, so change is inevitable. ;)