1

Is there a logic error/case that I'm not seeing?

I'm using PhpStorm 2017.1 with PHP language level 7.1 and CLI Interpreter PHP 7.1.8

I tried the following cases:

  1. User with carrinho(DB) and with and without carrinho_id on request
  2. User without carrinho(DB) and with and without carrinho_id on request
  3. carrinho_id on request with no user
  4. carrinho_id with wrong user(not logged)
$user = Auth::guard('api')->user();
if(!(isset($user) && ($carrinho = $user->getCarrinho()) != null)){
    if(!isset($data['carrinho_id']) || (($carrinho = Carrinho::find($data['carrinho_id'])) == null) || ($carrinho->usuario_id != null))
        $carrinho = Carrinho::salvar([]);
    if(isset($user))
        $carrinho->setUsuario($user->id);
}

if($carrinho->addProduto($data)) //"$carrinho" Variable might have not been defined
    return response()->json([
        'carrinho_id' => $carrinho->id
    ]);
return response()->json(['msg' => "O produto já está no seu carrinho"],422);

Two possible cases

$user exist

if(!(true && ($carrinho = $user->getCarrinho()) != null))

2 two possible path

1 - Has $carrinho

if(!(true && true)) -> !(true && true) -> !(true) -> false -> skip if and load the $carrinho from the user

2 - doesn't have $carrinho

if(!(true && false)) -> !(true && false) -> !(false) -> true -> Go inside the if and define the $carrinho

The code inside the first if will allways have a $carrinho

The problem lies on the first if. How do i know that? Because if I do this, the warning goes off.

if(!(isset($user) && ($carrinho = $user->getCarrinho()) != null)){
    if(!isset($data['carrinho_id']) || (($carrinho = Carrinho::find($data['carrinho_id'])) == null) || ($carrinho->usuario_id != null))
        $carrinho = Carrinho::salvar([]);
    if(isset($user))
        $carrinho->setUsuario($user->id);
}else{
    $carrinho = Carrinho::salvar([]);
}
Alberto Guilherme
  • 338
  • 2
  • 13
  • You're creating `$carrinho` inside your first if statement, but if that fails ($user is not set, for instance), then `$carrinho` won't be created. – aynber Jan 23 '20 at 14:27
  • PhpStorm doesn't run the code. It analyzes all possible branches and finds out that there are cases when the code reaches the line `if ($carrinho->addProduto($data))` and `$carrinho` has not bee initialized. Simplify the conditions, don't do assignments in the `if` condition and you'll find them out. As it is now, the code is very difficult to read and understand by a human. – axiac Jan 23 '20 at 17:09

3 Answers3

1

Your code may have a problem during execution and PHPStorm says about it. You won't have $carrinho object for some cases, for example, if $user variable exists

if(!(isset($user) && ($carrinho = $user->getCarrinho()) != null)){
    if(!isset($data['carrinho_id']) || (($carrinho = Carrinho::find($data['carrinho_id'])) == null) || ($carrinho->usuario_id != null))
        $carrinho = Carrinho::salvar([]);
    if(isset($user))
        $carrinho->setUsuario($user->id);
}

and code $carrinho->addProduto($data) will fail.

You need to fix it. For example, you сan move your code into conditions block

if(!(isset($user) && ($carrinho = $user->getCarrinho()) != null)){
    if(!isset($data['carrinho_id']) || (($carrinho = Carrinho::find($data['carrinho_id'])) == null) || ($carrinho->usuario_id != null)) {
        $carrinho = Carrinho::salvar([]);
    }
    if(isset($user)) {
        carrinho->setUsuario($user->id);
    }
    if($carrinho->addProduto($data)) {
        return response()->json([
            'carrinho_id' => $carrinho->id
        ]);
   }

}

return response()->json(['msg' => "O produto já está no seu carrinho"],422);
Maksym Fedorov
  • 6,383
  • 2
  • 11
  • 31
1

This code is extremely hard to read, because you've combined side effects (assignments) with complex conditions involving multiple boolean operators. Let's try to write it out as a set of discrete operations:

// First condition to be evaluated is isset($user)
$haveUser = isset($user);
// If that condition is false, the && will lazily skip the next part
if ( $haveUser ) {
    // Now we conditionally assign $carrinho ...
    $carrinho = $user->getCarrinho();
    // ... and test its value
    $haveCarrinho = ($carrinho != null);
}
// Having done all that, we combine the two conditions
$haveBoth = $haveUser && $haveCarrinho;
// Now we invert that condition for our if statement
if ( ! $haveBoth ) {
    // We know here that $carrinho has either never been set (because $haveUser was false) ...
    //   ... or it was null (setting $haveCarrinho to false)

    // Line 3 - another if statement to unpack
    $haveIdInData = isset($data['carrinho_id']);
    // If that condition is false, the || will shortcut
    if ( ! $haveIdInData ) {
        $carrinho = Carrinho::salvar([]);
    }
    // If we don't short-cut, the rest of line 3 runs
    else {
        // Try finding it by the ID in $data
        $carrinho = Carrinho::find($data['carrinho_id']);
        // If it's null, we short-cut at the next ||
        if ($carrinho == null) {
            $carrinho = Carrinho::salvar([]);
        }
        else {
            // Else we make the next check
            if ($carrinho->usuario_id != null) {
                $carrinho = Carrinho::salvar([]);
            }
        }
    }

    // On to line 4! Reusing our condition from above, since the state of $user won't have changed
    if ( $haveUser ) {
        // This will give a horrible error if $carrinho is null
        $carrinho->setUsuario($user->id);
    }
}

// We've reached line 5, and expect $carrinho to be set, but we have no guarantee of that at all!

That's a lot of logic for 4 lines of code!

Tidying up a little bit, without making it as cryptic as the original, I think this is equivalent:

$carrinho = null;
if ( isset($user) ) {
    // Now we conditionally assign $carrinho ...
    $carrinho = $user->getCarrinho();
}
if ( $carrinho == null ) {
    if ( isset($data['carrinho_id']) ) {
        $carrinho = Carrinho::find($data['carrinho_id']);

        if ($carrinho == null || $carrinho->usuario_id != null) {
            $carrinho = Carrinho::salvar([]);
        }
    }
    if(isset($user)) {
        $carrinho->setUsuario($user->id);
    }
}

Now we can see what was probably intended: the Carrinho::salvar line should be the fallback for any other undefined state, rather than nested inside other conditions.

With a bit of effort, we can eliminate nested conditions altogether, giving something much more readable like this:

// Initialise variables to a known state
$carrinho = null;
$loadedFromUser = false;
// Try on user object
if ( isset($user) ) {
    $carrinho = $user->getCarrinho();
    $loadedFromUser = ($carrinho != null);
}
// Not found? Try looking up by input data
if ( $carrinho == null && isset($data['carrinho_id']) ) {
    $carrinho = Carrinho::find($data['carrinho_id']);
}
// Discard if it already has a user ID, but wasn't loaded from user
if (!$loadedFromUser && $carrinho != null && $carrinho->usuario_id != null) {
    $carrinho = null;
}
// Still not found? Create an empty one
if ($carrinho == null) {
    $carrinho = Carrinho::salvar([]);
}

// Now we know we have an object some way or another, and can assign a user ID to it
if(isset($user) && !$loadedFromUser) {
    $carrinho->setUsuario($user->id);
}

Some of these conditions might not be quite what was intended, but by splitting them out, we can now follow the logic much more easily and make changes as appropriate.

IMSoP
  • 89,526
  • 13
  • 117
  • 169
  • thanks for the reply. On Line 3 you added a extra `!`. This is what will happen: If `$data['carrinho_id'] == null` It will create a carrinho on line 3. If the seccond condition on Line 3 return true, means that it did not find a carrinho, so it will create a new card. – Alberto Guilherme Jan 23 '20 at 19:11
  • The code inside the first `if` runs fine. The problem lies on the first `if` itself. I know now that the code is hard to read, but to me it came naturally. I added some more information on the question. I apologize if I have been rude. – Alberto Guilherme Jan 23 '20 at 19:21
  • @AlbertoGuilherme You're right, I think. To be honest, the code is so dense with boolean combinations and side-effects, I've got a bit lost trying to follow it. – IMSoP Jan 23 '20 at 19:32
  • I truly thank you for your answer. Its really elaborated and organized. I will not marked as the correct one, but I will do a +1 for the answer. – Alberto Guilherme Jan 23 '20 at 19:36
0

After some questioning and some reseach, I came to the conclusion that hard code does not help anyone, in tearms of team work and project growth. I decided to implement the code in the following way:

$user = Auth::guard('api')->user();
$hasUser = isset($user);
if($hasUser)
    $carrinho = $user->getCarrinho();
if(!isset($carrinho)){
    if(isset($data['carrinho_id']))
        $carrinho = Carrinho::find($data['carrinho_id']);
    if(!isset($carrinho) || $carrinho->usuario_id != null)
        $carrinho = Carrinho::salvar([]);
    if($hasUser)
        $carrinho->setUsuario($user->id);
}
if($carrinho->addProduto($data))
    return response()->json([
        'carrinho_id' => $carrinho->id
    ]);
return response()->json(['msg' => "O produto já está no seu carrinho"],422);

I will not accept this anwser because I'm still not conviced that the code had errors because no one could find any. But I will leave this here to show that hard code does no help.

Alberto Guilherme
  • 338
  • 2
  • 13