Fixed Strange Nesting of if/else Statements

[Pieter Boelen]

I have uploaded on the ftp JRH files 16-02-09. It's preparations for another part of WoodesRogers 2.

Thanks for that!

But can you pretty please double-check your code in 'TakeItems'? If I read it correctly, it ends up as:

if (something )
{
   if(itm2=="item1")
   {
   }
}
else
{
   if(itm2=="item2")
   {
   }
}
else
{
   if(itm2=="item3")
   {
   }
}

I am not sure what that is going to do, but it is VERY doubtful that it would do what you intend it to.
Note how there are multiple 'else' statements there for the same 'if'.

It should probably be either:

if (something )
{
   if(itm2=="item1")
   {
   }
   else
   {
     if(itm2=="item2")
     {
     }
     else
     {
       if(itm2=="item3")
       {
       }
     }
   }
}

In other words, nested if-statements; but that gets VERY annoying with the amount that you've added.

OR it should be:

if (something )
{
   switch(itm2)
   {
     case "item1":
     break;
     case "item2":
     break;
     case "item3":
     break;
   }
}

Where this last one is a much cleaner and safer solution.

You could probably even use:

if (something )
{
   if(itm2=="item1")
   {
   }
   if(itm2=="item2")
   {
   }
   if(itm2=="item3")
   {
   }
}

So same thing as you have now, but without the additional 'else' statements.

 

[Jack Rackham]

As long as it still works Pieter I'm happy for all clean ups.
However I pushed in those 3 special cases in between other if cases so I really don't know how
to rewright that thing.

 

[Pieter Boelen]

As long as it still works Pieter I'm happy for all clean ups.
However I pushed in those 3 special cases in between other if cases so I really don't know how
to rewright that thing.

It is your code. I don't know how it is meant to work or what it is meant to do.
All I know is that when I look at it, it looks very strange to me. So if I'm indeed right, it means there can be a lapse in logic there somewhere.
Then in some cases it may not do what you meant it to. Or it could have other unexpected and unintended side-effects.
Which seems rather scary to me....

 

[Pieter Boelen]

@Jack Rackham: I did a quick test to see how the code in the above example actually runs in the game.
If I execute this through console:

  if (true)
   {
     LogIt("Part 1");
   }
   else
   {
     LogIt("Part 2");
   }
   else
   {
     LogIt("Part 3");
   }
   else
   {
     LogIt("Part 4");
   }

Then ONLY "Part 1" is executed.

On the other hand, if I do this:

  if (false)
   {
     LogIt("Part 1");
   }
   else
   {
     LogIt("Part 2");
   }
   else
   {
     LogIt("Part 3");
   }
   else
   {
     LogIt("Part 4");
   }

Then Parts 2, 3 AND 4 get executed.

This surprises me, since by my logic, it should be 1&3 for "true" and 2&4 for "false".

In any case, it does indicate that probably the code example that I found could be modified like this:

  if (false)
   {
     LogIt("Part 1");
   }
   else
   {
     LogIt("Part 2");
     LogIt("Part 3");
     LogIt("Part 4");
   }

This should be functionally equivalent, but not look so odd.

 

[Pieter Boelen]

@Jack Rackham: To be on the safe side, I do think the above change should be made.
Should just require removing some of this code:

   }
  else
  {

That is apparently superfluous and just makes it look confusing.

 

[Jack Rackham]

The problem is that at the end of all these if/else cases there is this thing:

  }
        }
    //<<-- JRH

        else takeitem = true;        // for any other item: just take it

not sure how to get that to work. A switch situation with the takeitem = true;
as "default" below all the cases?

I really don't want any extra trouble with things that Do work even if the code
is very homemade.

 

[Pieter Boelen]

I really don't want any extra trouble with things that Do work even if the code
is very homemade.

Let me put it this way.... seeing code that looks like this is POSITIVELY TERRIFYING to me:

   if (false)
  {
  LogIt("Part 1");
  }
  else
  {
  LogIt("Part 2");
  }
  else
  {
  LogIt("Part 3");
  }
  else
  {
  LogIt("Part 4");
  }

One if-statement should have a single else; not more.
Since that isn't the case here (???), that makes it very confusing for anyone looking at that code.
And "confusing" quickly leads to bugs, which we really don't want....

not sure how to get that to work. A switch situation with the takeitem = true;
as "default" below all the cases?

I think right now that "takeitem = true" would happen in ALL cases, because based on my quick test last week, I think ALL 'else' statements get executed.
So functionally you could just remove the 'else' around it and it will still do the same thing it currently does.
If that doesn't seem right to you, then some more rethinking may be required.

This might be the best solution of them all:

if (something )
{
  switch(itm2)
  {
  case "item1":
  break;
  case "item2":
  break;
  case "item3":
  break;
  //default:
  takeitem = true;  // for any other item: just take it
  }
}

But I'm not familiar enough with what that large section does to be able to say if that would serve your purpose properly or not....

 

[Jack Rackham]

I'll give it a try and will test it.

 

[Pieter Boelen]

I'll give it a try and will test it.

Much appreciated!

 

[Jack Rackham]

Done, and this:

 switch(itm2)
  {
  case "item1":
  break;
  case "item2":
  break;
  case "item3":
  break;
  //default:
  takeitem = true;  // for any other item: just take it
  }

worked fine!

 

[Pieter Boelen]

Done, and this:

 switch(itm2)
  {
  case "item1":
  break;
  case "item2":
  break;
  case "item3":
  break;
  //default:
  takeitem = true;  // for any other item: just take it
  }

worked fine!

EPIC! Definitely seems much more comfortable to me!

 

[Pieter Boelen]

@Jack Rackham: If you could post the changed file before the weekend or so, that would be much appreciated!

 

[Jack Rackham]

Sure, here it is.

 

[Pieter Boelen]

Sure, here it is.

Included in my game now. Thanks again!