10 replies to this topic

[coderWalker]

Right now I am using this:

if (
world0->blockExists(x-1,y,z,world0->xOffset-location.x,world0->yOffset-location.y,world0->zOffset-location.z)==true && 
world0->blockGetAmount(x-1,y,z,world0->xOffset-location.x,world0->yOffset-location.y,world0->zOffset-location.z)==-1
)
{

}
else
{
	code
}

I am trying to clean up this code along with 14 other files. However I can't afford to have the wrong answer.
So what would the if statement need to change to so code can be in the standard if and not the else?

It wouldn't be this, because the && complicates it:

if (
world0->blockExists(x-1,y,z,world0->xOffset-location.x,world0->yOffset-location.y,world0->zOffset-location.z)==false  && 
world0->blockGetAmount(x-1,y,z,world0->xOffset-location.x,world0->yOffset-location.y,world0->zOffset-location.z)!=-1
)

[BitMaster]

De Morgan's laws

[gazliddon]

That's some horrid looking code IMHO

You could do this though

bool test = world0->blockExists(x-1,y,z,world0->xOffset-location.x,world0->yOffset-location.y,world0->zOffset-location.z)==true && 
	world0->blockGetAmount(x-1,y,z,world0->xOffset-location.x,world0->yOffset-location.y,world0->zOffset-location.z)==-1;

if (!test)
{
	...
}

[btower]

To explain with a readable example (since you need to do 14 other files).

If you have:

if (a && <img src='http://public.gamedev.net/public/style_emoticons/<#EMO_DIR#>/cool.gif' class='bbc_emoticon' alt='B)' /> {
} else {
// do something
}

You can rewrite it as:

if (!a || !<img src='http://public.gamedev.net/public/style_emoticons/<#EMO_DIR#>/cool.gif' class='bbc_emoticon' alt='B)' /> {
  // do something
}

If your statements inside if () are complex, or you simply can't be bothered with going through it, gazliddon's suggestion is a good alternative. It makes your code a little bit more reabable I suppose. You could also just negate the if statement like this:

// before
if (a && b && c && d && e) {
} else {
  // do something
}

// after
if (! (a && b && c && d && e) ) {
  // do something
}

[teutonicus]

Right now I am using this:

if (
world0->blockExists(x-1,y,z,world0->xOffset-location.x,world0->yOffset-location.y,world0->zOffset-location.z)==true && 
world0->blockGetAmount(x-1,y,z,world0->xOffset-location.x,world0->yOffset-location.y,world0->zOffset-location.z)==-1
)
{

}
else
{
	code
}

Easier to read = easier to refactor.

bool blockExists = world0->blockExists(x-1,y,z,world0->xOffset-location.x,world0->yOffset-location.y,world0->zOffset-location.z)==true;
bool blockAmountIsInvalid = world0->blockGetAmount(x-1,y,z,world0->xOffset-location.x,world0->yOffset-location.y,world0->zOffset-location.z)==-1;

if (blockExists && blockAmountIsInvalid)
{

}
else
{
	code
}

Now you can easily get rid of the else ...

bool blockExists = world0->blockExists(x-1,y,z,world0->xOffset-location.x,world0->yOffset-location.y,world0->zOffset-location.z)==true;
bool blockAmountIsInvalid= world0->blockGetAmount(x-1,y,z,world0->xOffset-location.x,world0->yOffset-location.y,world0->zOffset-location.z)==-1;

if (!blockExists || !blockAmountIsInvalid)
{
	code
}

Although without seeing more of the code, that condition doesn't make much sense to me


edit: holy flood of answers, Batman

[owl]

Easier to read = easier to refactor.

bool blockExists = world0->blockExists(x-1,y,z,world0->xOffset-location.x,world0->yOffset-location.y,world0->zOffset-location.z)==true;
bool blockAmountIsInvalid = world0->blockGetAmount(x-1,y,z,world0->xOffset-location.x,world0->yOffset-location.y,world0->zOffset-location.z)==-1;

if (blockExists && blockAmountIsInvalid)
{

}
else
{
	code
}

I think this one ^^ is the better. You can even make it more readable by adding more variables:

int xof_param = world0->xOffset-location.x;
int yof_param = world0->yOffset-location.y;
int zof_param = world0->zOffset-location.z;

bool blockExists          = world0->blockExists   (x-1, y, z, xof_param, yof_param, zof_param);
bool blockAmountIsInvalid = world0->blockGetAmount(x-1, y, z, xof_param, yof_param, zof_param)==-1;

if (blockExists && blockAmountIsInvalid)
{

}

[coderWalker]

Thanks everyone! I think the best thing to do is just negate the statement.
I never even thought of that solution

Yea the If statements are pretty bad.
I really need to make them more readable somehow.

I Always seem to over complicate things.

[Brother Bob]

Thanks everyone! I think the best thing to do is just negate the statement.
I never even thought of that solution

No, that's the easy way out. The best thing to do it to understand boolean algebra and design the logic to properly describe what you want it to do.

Yea the If statements are pretty bad.
I really need to make them more readable somehow.

I Always seem to over complicate things.

In my opinion, what makes your if-statement ugly and difficult to read is the way your function calls look like. Many calculations and multiple parameters that perhaps should be collected in structures. For example, not only should you factor out the calculations like owl demonstrated, but is there a reason you're not sticking your (what appears to be) vectors in a vector structure to collect three linked parameters into one?

If you do, then your two functions only take two parameters, and one can be calculated outside the function calls. Then you don't even have to calculate the individual boolean results like owl did. What you gain by that is short circuiting, in that the second function call doesn't have to be evaluated if the first one completely determines the outcome of the entire expression. And the if-statement doesn't become messy anyway, since the function calls are clean and compact looking.

[coderWalker]

Thanks everyone! I think the best thing to do is just negate the statement.
I never even thought of that solution

No, that's the easy way out. The best thing to do it to understand boolean algebra and design the logic to properly describe what you want it to do.

Yea the If statements are pretty bad.
I really need to make them more readable somehow.

I Always seem to over complicate things.

In my opinion, what makes your if-statement ugly and difficult to read is the way your function calls look like. Many calculations and multiple parameters that perhaps should be collected in structures. For example, not only should you factor out the calculations like owl demonstrated, but is there a reason you're not sticking your (what appears to be) vectors in a vector structure to collect three linked parameters into one?

If you do, then your two functions only take two parameters, and one can be calculated outside the function calls. Then you don't even have to calculate the individual boolean results like owl did. What you gain by that is short circuiting, in that the second function call doesn't have to be evaluated if the first one completely determines the outcome of the entire expression. And the if-statement doesn't become messy anyway, since the function calls are clean and compact looking.

Me and Owl must of posted at the exact same time. I didn't even see it, but now I do. I think Owl's is best also
combining it like he did also saves some computing time. Since it just reads from memory and doesn't have to calculate.

[owl]

Me and Owl must of posted at the exact same time. I didn't even see it, but now I do. I think Owl's is best also
combining it like he did also saves some computing time. Since it just reads from memory and doesn't have to calculate.

What I posted is exactly what teutonicus proposed but I just made the calculations outside the function call for readability's sake. I don't think it's gonna be faster and if it is it'd be a despicable improvement, but I've learned to be willing to trade a few cycles for readability and code maintainability

You might want to explore what Brother Bob proposed: Implementing algebraic vectors into your code:

class vector3
{
  public: // Make it right
   int x;
   int y;
   int z;
};

// make a method in class world that combines blockGetAmount and blockExists
// the offset seem to be inside world so you don't even need to pass it as an argument.
bool World::BlockValidExists(vector3& v_pos, vector3& v_xof)
{
    return (blockExists (v_pos, v_xof) && (blockGetAmount(v_pos, v_xof)!=-1));
}

// and then ...

vector3 xof_vector (world0->xOffset-location.x, world0->yOffset-location.y, world0->zOffset-location.z);
vector3 pos_vector (x-1, y, z);

if (world0->BlockValidExists(pos_vector, xof_vector))
{

[Darg]

I was just about to mention, before I saw Owls latest post that you're passing exactly the same variables through to World twice asking for two separate results. It would be far more readable if you simply introduced a third function in World that takes the variables once and does the two tests then passes back False if both of the values are true.

You'll learn as you go on, especially with larger projects that if you come back to that code in a couple of months you'll have to spend half an hour just deciphering what it was you were doing with it all. In my opinion readable code is more important even than super efficient code. I know a lot of people will say that when programming for realtime calculations like games efficiency is paramount but readable code will make development easier and quicker which should give you plenty of time at the end for efficiency changes if they're even needed at that point