2

It seems that I can't use a variable defined in an if statement in a later part of the same statement. Is there a way around this? Here's the statement:

if ($cat = getCat($item) && 
    ($masterCat= getVC($cat) || $masterCat= getTC($cat))) {
    echo "success";
} else {
    echo "fail";
}

As you can see, $cat is defined in the first part. If that part evaluates False then the other parts won't run. My problem is that I get

Undefined variable: cat

even though getCat is called and $cat defined before the RHS of the && operator is evaluated.

Building this code I learned about short circuit evaluation and Truthy values but now I'm stuck trying to "one line" it. I can nest if statements but was wondering as there are three possible points of failure if there's a way to do this where the "fail" part is only defined once (it's bigger in the real code, not just echo "fail";)

functions below

function getCat($pItem) {
    //looks up $pItem and returns a category or null if $pItem doesn't exist
}

function getVC($pCat) {
    //looks up $pCat quickly, returns master category or null if not in quick lookup table
}

function getTC($pCat) {
    //looks up $pCat thoroughly, 
    //returns master category OR
    //returns null if $pCat has expired (shouldn't happen but prevents crashing if it ever does)
}
Community
  • 1
  • 1
franki
  • 123
  • 2
  • 13
  • 2
    Have you tried separating it outside of the if statement? That would tell you real quick if the problem is the assignment happening in the conditional, or if something else weird is going on. – FatBoyXPC Mar 08 '17 at 14:24
  • yeah, it wasn't what I thought, it was the problem as pointed out by @Boldewyn below. – franki Mar 08 '17 at 15:38

3 Answers3

4

You learned about short circuit evaluation, and your next lecture should include operator precedence :-)

In a nutshell: && binds stronger than =, so that the code is evaluated like this:

$cat = (getCat($item) && ($masterCat = (getVC($cat) || $masterCat = getTC($cat)))

Inside the parentheses, $cat is indeed not defined yet. Simple fix: Add the parentheses yourself:

if (($cat = getCat($item)) && 
    (($masterCat = getVC($cat)) || ($masterCat = getTC($cat)))) {
    echo "success";
} else {
    echo "fail";
}

Re: code style, assignments inside conditionals are mostly thought of being a bad idea in PHP. That is, because a future reader cannot quickly distinguish between "cleverness" and a buggy == and needs to put thought in understanding this structure.

For learning PHP syntax your code is all good to go, but in a production environment you might want to follow @FatBoyXPC's advice.

Boldewyn
  • 81,211
  • 44
  • 156
  • 212
  • 1
    I did know this about precedence but had totally not noticed that this was what was happening. This gets the answer as it answers the question I asked. Having read the assorted feedback, I think I should probably do it the way @Code Lღver suggested. – franki Mar 08 '17 at 15:35
1

Why not you have define this variable outside of if condition:

$cat = getCat($item); //define the variable here
if ($cat && ($masterCat= getVC($cat) || $masterCat= getTC($cat))) {
    echo "success";
} else {
    echo "fail";
}

Outside of if condition the variable $cat you can use later also.

Code Lღver
  • 15,573
  • 16
  • 56
  • 75
1

If the issue is that you cannot get it to work as a one liner why not stop hindering yourself?

$cat = getCat($item);
//Ternary if you getVC does not return null use it else use getTC
$masterCat = isset(getVC($cat)) ? getVC($cat) : getTC($cat);

if ($cat && $masterCat) {
    echo "success";
} else {
    echo "fail";
}

While yes there are two more lines your if statement is readable your don't have to worry about some weird precedence and there is no reduction in run time speed.

Actually revising the idea

//if $cat was null I assume $masterCat should be
if (isset($masterCat)) {
    echo "success";
} else {
    echo "fail";
}

But in reality you could also give yourself some flexibility by capturing the cases and doing things off of them.

$cat = getCat($item);
if(isset($cat)){
    $masterCat = isset(getVC($cat)) ? getVC($cat) : getTC($cat);
    if(isset($masterCat)){
        //do work
    } else {
        //fail for no master
    }
} else {
    //fail for no cat
}

One liners are not any better usable and readable code is.

Additional

Point about double calling on ternary. You could first look up getVC then do the ternary off that.

$masterCat = getVC($cat);
$masterCat = isset($masterCat) ? $masterCat : getTC($cat);
nerdlyist
  • 2,842
  • 2
  • 20
  • 32
  • Thanks for the feedback. I didn't go with ternary because it seemed that I'd be calling getVC($cat) twice. Also you're right, $mastercat should be null if $cat is null so an opening condition both getVC() and getTC() that returned null if $pCat is null would save processing time. I just didn't want to call them at all unless I knew $cat was valid. – franki Mar 08 '17 at 15:30
  • 1
    @franki good point on the ternary. I added another way to go with it. It seems like the nested `if`s are what you really need. Just remember 6 month from now this code will even be foreign to you. Just make it easy to read. – nerdlyist Mar 08 '17 at 15:37