Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

enter image description here

    var s = player.getSurrounding();
    if (vY > 0) {
        if ((s[0] || (s[7] && xLocal < LOCAL_LEFT) || (s[4] && xLocal > LOCAL_RIGHT)) && (yLocal + vY * dt) > LOCAL_RIGHT) {
            vY = (LOCAL_RIGHT - yLocal) / dt;
        }
    } else if (vY < 0) {
        if ((s[2] || (s[6] && xLocal < LOCAL_LEFT) || (s[5] && xLocal > LOCAL_RIGHT)) && (yLocal + vY * dt) < LOCAL_LEFT) {
            vY = (LOCAL_LEFT - yLocal) / dt;
        }
    }
    if (vX < 0) {
        if ((s[3] || (s[7] && yLocal > LOCAL_RIGHT) || (s[6] && yLocal < LOCAL_LEFT)) && (xLocal + vX * dt) < LOCAL_LEFT) {
            vX = (LOCAL_LEFT - xLocal) / dt;
        }
    } else if (vX > 0) {
        if ((s[1] || (s[4] && yLocal > LOCAL_RIGHT) || (s[5] && yLocal < LOCAL_LEFT)) && (xLocal + vX * dt) > LOCAL_RIGHT) {
            vX = (LOCAL_RIGHT - xLocal) / dt;
        }
    }

This might be a long shot, but I threw this together. It seems very inefficient and repetitive. It might be hard to explain, but here I go.

The world is made up of a grid of blocks that is saved in a 40x600 boolean array (true if filled, false if not).

The red block represents the player, a simple block that can fly in all directions. The position is defined by it's center point (orange).

The numbers represent the index of the boolean array s, true if the block is filled and false if not. Filled blocks should be interacted with.

xLocal and yLocal represents the position of the player in it's current grid position. If the player is all the way to the left: xLocal = (playersize / 2) AKA LOCAL_LEFT. If the player is all the way to the right: xLocal = blocksize - (playersize / 2) AKA LOCAL_RIGHT. Same applies for yLocal but for up and down.

dt represents the game's delta time.

If the if-statements go through, the velocity is set so that xLocal will equal LOCAL_LEFT or LOCAL_RIGHT by the next time the position is updated.

share|improve this question
1  
Which language is this? Looks like C# to me right now, but it could be ActionScript as well. –  Pimgd Aug 1 at 11:46
    
I'm using JavaScript. It could be pretty much any language, I'm using just the basics here. –  John Aug 1 at 11:47
5  
Retagged your question to be a javascript question. Welcome to Code Review! The reason we ask for a language is because whilst programming from an abstracted mindset can be beneficial to the algorithm, each language has it's own features (which in some cases completely trivialize algorithms!). Plus, it prevents you from getting recommendations to use some function in a programming language you're not even using... –  Pimgd Aug 1 at 11:48
    
I didn't think about that, thank you! –  John Aug 1 at 11:53

2 Answers 2

up vote 8 down vote accepted

Naming

s is a poor name for a boolean array. I have no idea what s stands for. How about surroundingTiles?

Array ordering

Additionally, the order of the tiles seems strange to me. It's ... bottom, right, top, left, bottom right, top right, top left, bottom left. Without the image, I don't think I'd have gotten it. (For those wondering, it's non-diagonal, starting bottom, counter clockwise, then diagonal.) Any particular reason you have this layout within your array?

Magic numbers

Continuing from my previous point, this can be solved by naming the directions in constants:

var BOTTOM_INDEX = 0;
var RIGHT_INDEX = 1;
var TOP_INDEX = 2;
var LEFT_INDEX = 3;
var BOTTOMRIGHT_INDEX = 4;
var TOPRIGHT_INDEX = 5;
var TOPLEFT_INDEX = 6;
var BOTTOMLEFT_INDEX = 7;

I'm calling it Magic Numbers because without the image, I'd be left wondering which index is which. They're magic in that if they change, the code stops working. But why it has to be those numbers exactly is unknown... to the reader, at least.

Difference so far

var surroundingTiles = player.getSurrounding();
if (vY > 0) {
    if ((surroundingTiles[BOTTOM_INDEX] || (surroundingTiles[BOTTOMLEFT_INDEX] && xLocal < LOCAL_LEFT) || (surroundingTiles[BOTTOMRIGHT_INDEX] && xLocal > LOCAL_RIGHT)) && (yLocal + vY * dt) > LOCAL_RIGHT) {
        vY = (LOCAL_RIGHT - yLocal) / dt;
    }
} else if (vY < 0) {
    if ((surroundingTiles[TOP_INDEX] || (surroundingTiles[TOPLEFT_INDEX] && xLocal < LOCAL_LEFT) || (surroundingTiles[TOPRIGHT_INDEX] && xLocal > LOCAL_RIGHT)) && (yLocal + vY * dt) < LOCAL_LEFT) {
        vY = (LOCAL_LEFT - yLocal) / dt;
    }
}
if (vX < 0) {
    if ((surroundingTiles[LEFT_INDEX] || (surroundingTiles[BOTTOMLEFT_INDEX] && yLocal > LOCAL_RIGHT) || (surroundingTiles[TOPLEFT_INDEX] && yLocal < LOCAL_LEFT)) && (xLocal + vX * dt) < LOCAL_LEFT) {
        vX = (LOCAL_LEFT - xLocal) / dt;
    }
} else if (vX > 0) {
    if ((surroundingTiles[RIGHT_INDEX] || (surroundingTiles[BOTTOMRIGHT_INDEX] && yLocal > LOCAL_RIGHT) || (surroundingTiles[TOPRIGHT_INDEX] && yLocal < LOCAL_LEFT)) && (xLocal + vX * dt) > LOCAL_RIGHT) {
        vX = (LOCAL_RIGHT - xLocal) / dt;
    }
}

(Ckuhn also posted some naming changes, I suggest you add them in here as well.)

But Pim, if I do this, my if statements become very long!

Encapsulate Conditionals

Let's take a look at one of these paths.

var surroundingTiles = player.getSurrounding();
if (vY > 0) {
    if ((surroundingTiles[BOTTOM_INDEX] || (surroundingTiles[BOTTOMLEFT_INDEX] && xLocal < LOCAL_LEFT) || (surroundingTiles[BOTTOMRIGHT_INDEX] && xLocal > LOCAL_RIGHT)) && (yLocal + vY * dt) > LOCAL_RIGHT) {
        vY = (LOCAL_RIGHT - yLocal) / dt;
    }

if the vectorY is greater than 0... I guess that means if you're moving ... down... then...
if the bottom tile is solid or if either the bottom left tile is solid and the ... x something? or the bottom right tile is solid and x something but different...

It's way too confusing!
I can understand it, obviously, as I recognize the pattern (and you've given me an image for explanation). However, if I were to explain this code to someone else, I'd mentally recheck the code before continuing my explanation. I'd even manage to get it wrong at least once during my explanation ("and this checks for the upward movement, ... er, I mean the downward movement").

So, how can we fix that? Let's use English!

var surroundingTiles = player.getSurrounding();
if (movingDown) {
    if ((surroundingTiles[BOTTOM_INDEX] || (surroundingTiles[BOTTOMLEFT_INDEX] && passingLeftTileBorder) || (surroundingTiles[BOTTOMRIGHT_INDEX] && passingRightTileBorder)) && (yLocal + vY * dt) > LOCAL_RIGHT) {
        vY = (LOCAL_RIGHT - yLocal) / dt;
    }

It's still not clear enough. That's because I have no idea what (yLocal + vY * dt) > LOCAL_RIGHT means! Additionally, we still didn't get it clear enough.

var surroundingTiles = player.getSurrounding();
if (movingDown) {
    if ((hittingBottomTile || hittingBottomLeftTile || hittingBottomRightTile) && (yLocal + vY * dt) > LOCAL_RIGHT) {
        vY = (LOCAL_RIGHT - yLocal) / dt;
    }

Almost.

var surroundingTiles = player.getSurrounding();
if (movingDown) {
    if (hittingTileInBottomDirection && (yLocal + vY * dt) > LOCAL_RIGHT) {
        vY = (LOCAL_RIGHT - yLocal) / dt;
    }

Nice. It just got wondrously clean.
Sadly, computers are grumpy things that want everything specified, so we'll have to explain our human-readable English to them.

var movingDown = vY > 0;
if (movingDown) {
    var hittingBottomLeftTile = (surroundingTiles[BOTTOMLEFT_INDEX] && xLocal < LOCAL_LEFT);
    var hittingBottomRightTile = (surroundingTiles[BOTTOMRIGHT_INDEX] && xLocal > LOCAL_RIGHT);
    var hittingTileInBottomDirection = (surroundingTiles[BOTTOM_INDEX] || hittingBottomLeftTile || hittingBottomRightTile);
    if (hittingTileInBottomDirection && (yLocal + vY * dt) > LOCAL_RIGHT) {
        vY = (LOCAL_RIGHT - yLocal) / dt;
    }

At this point, you could consider shifting certain parts of the logic towards a function which does simple calculations. Your call. I'd prefer that to this inline logic, because if you use the functions, you can write this function on a different level of abstraction. That is, you can write English code consisting entirely of function calls. At that point, if you made a mistake, you'll spot it instantly. Because if you have a function that looks like this...

function checkForCollision(/*arguments omitted*/) {
    checkCollisionInDirection(LEFT);
    checkCollisionInDirection(RIGHT);
    checkCollisionInDirection(TOP);
    checkCollisionInDirection(LEFT);
}

The bug is obvious.

I'll leave the location of variable definitions and the (yLocal + vY * dt) > LOCAL_RIGHT to other answerers.

share|improve this answer
    
Thank you very much for putting your time into this! I definitely need to start using plain English more. It's surprising the difference it makes. I will look into implementation and report back! –  John Aug 1 at 12:50
    
This one sounds like a good candidate for an iterative review @user3186618. –  RubberDuck Aug 1 at 13:41

I'll let someone smarter than myself help you with your actual problem. I'm going to talk about naming a little bit. Naming, you see, it's important (and hard). If your variables had more descriptive names, you might not have had to write several paragraphs explaining what they are. Poor Mr. Maintainer will not have the benefit of your explanation either. By time he sees this code, you've already moved on and he's left figuring out what's what; mapping short and cryptic names to meanings in his mind.

  • vY & vX become vectorY & vectorX
  • dt should be deltaTime
  • xLocal & yLocal would be easier understood as playerX and playerY. Maybe even a little more verbosely as playerXCoord.
  • I can't think of anything better for your array of booleans, but s should be replaced as well.
share|improve this answer
    
I try to keep names that I use a lot short. s is used 3 times in the same line, I can't stand multi-line statements. Also, the assignment is the first line, I think that should clarify it at least a little. However, I do agree I could do a better job with the others. –  John Aug 1 at 12:10
    
Honestly, dt isn't terrible either. I understood it instantly, but someone without a math background might not have. –  RubberDuck Aug 1 at 12:14

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.