current community

your communities

more stack exchange communities

Stack Exchange
tour help
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.
up vote 6 down vote favorite

This code works but I'm not really sure if I wrote it good enough. It seems a bit vague but I can't really assess it properly. I'm particularly concerned with the xAxis variable. I don't like how I have to initialize it inside the first loop. I would rather have it next to yAxis. Is this a problem?

float yAxis = SPACE_BETWEEN_TILES;
float xAxis;

for(int i = 0; i < 3; i++) {
    xAxis = SPACE_BETWEEN_TILES;
    for(int v = 0; v < 3; v++) {
        shapeRenderer.rect(xAxis, yAxis, 10, 10);
        xAxis = TILE_WIDTH_AND_HEIGHT + xAxis;
    }
    yAxis = yAxis + TILE_WIDTH_AND_HEIGHT;
}
share|improve this question

3 Answers 3

up vote 6 down vote accepted

Write it this way instead:

for (int v = 0; v < 3; v++) {
    for (int h = 0; h < 3; h++) {
        float x = SPACE_BETWEEN_TILES + h * TILE_WIDTH_AND_HEIGHT;
        float y = SPACE_BETWEEN_FILES + v * TILE_WIDTH_AND_HEIGHT;
        shapeRenderer.rect(x, y, 10, 10);
    }
}

… because:

  • With your repeated addition, floating-point inaccuracies would accumulate.
  • It's easier to understand.
  • All variables are scoped as narrowly as possible.
  • i and v make a weird pair of variable names.

In addition, you should probably consider using double, as float is rarely useful.

share|improve this answer
up vote 7 down vote

I think it is mostly fine as it is. I would declare variables at the level they are actually needed, not before. Also, xAxis = TILE_WIDTH_AND_HEIGHT + xAxis; can be written as xAxis += TILE_WIDTH_AND_HEIGHT.

i and v are not very good variable names (too short, and they don't fit together). What does v even stand for? vertical? In that case, I would use vertical and horizontal.

TILE_WIDTH_AND_HEIGHT would be shorter as TILE_DIMENSION.

You could restructure it like this if you want to:

    for (int y = 0; y < 3; y++) {
        for (int x = 0; x < 3; x++) {
            shapeRenderer.rect(SPACE_BETWEEN_TILES + TILE_DIMENSION * x, SPACE_BETWEEN_TILES + TILE_DIMENSION * y, 10, 10);
        }
    }

The performance loss from transforming additions to multiplications should be fine in most cases.

share|improve this answer
up vote 3 down vote

With a little rearrangement, you can initialize xAxis both in and out of the loop, like yAxis:

float yAxis = SPACE_BETWEEN_TILES;
float xAxis = SPACE_BETWEEN_TILES;

for(int i = 0; i < 3; i++) {
    for(int v = 0; v < 3; v++) {
        shapeRenderer.rect(xAxis, yAxis, 10, 10);
        xAxis = TILE_WIDTH_AND_HEIGHT + xAxis;
    }
    yAxis = yAxis + TILE_WIDTH_AND_HEIGHT;
    xAxis = SPACE_BETWEEN_TILES;
}

This isn't really an issue, but I would probably initialize xAxis first because you write the coordinates as {x, y}, so this keeps that order:

float xAxis = SPACE_BETWEEN_TILES;
float yAxis = SPACE_BETWEEN_TILES;

for(int i = 0; i < 3; i++) {
    for(int j = 0; j < 3; j++) {
        shapeRenderer.rect(xAxis, yAxis, 10, 10);
        xAxis = TILE_WIDTH_AND_HEIGHT + xAxis;
    }

    xAxis = SPACE_BETWEEN_TILES;
    yAxis = yAxis + TILE_WIDTH_AND_HEIGHT;
}

Also, when you have two nested loops, it is traditional to use variable names i and j for your counters. If your counters represent any besides counters, you may want to name them a bit more specifically.

share|improve this answer

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.