1
\$\begingroup\$

I am using the java.awt.Rectangle class to construct subsets of a GEOTIFF file. In order to do this I would need to specify the x,y offsets, height and width of each subset image. In my particular case I would need to crop the original GEOTIFF image bottom to top. The java.awt.Rectangle class specifies that the origin point is to be found in the upper left hand corner.

Please let me know if the calculation of the image offsets are correct and/or there is a better way of calculating the image offsets. The crop(r) method is a proprietary method used to crop subsets of the original image.

int width = 40;
int height = 34;
int cellSize = 3600;
int xOffset = 0;
int yOffset = 0;
int pixelWidth = cellSize * width;
int pixelHeight = cellSize * height;
for (int i = 0 ; i < width; i++)
{
    for (int j = 0; j < height; j++)
    {
        yOffset = pixelHeight - cellSize *(j+1) ;
        Rectangle r = new Rectangle(xOffset,
                                    yOffset,
                                    cellSize,
                                    cellSize);
        crop(r);
    }
    xOffset = pixelWidth - cellSize * (i+1);
}
\$\endgroup\$
2
\$\begingroup\$
int width = 40;
int height = 34;
int cellSize = 3600;
int xOffset = 0;
int yOffset = 0;
int pixelWidth = cellSize * width;
int pixelHeight = cellSize * height;

I would find this easier to follow in a different order:

final int CELL_SIZE = 3600;

final int WIDTH = 40;
final int HEIGHT = 34;

final int PIXEL_WIDTH = CELL_SIZE * WIDTH;
final int PIXEL_HEIGHT = CELL_SIZE * HEIGHT;

int xOffset = 0;
int yOffset = 0;

Now I see a natural progression of values and have the variables separate from the constants. The variables are also closest to where they are changed.

I changed the values that never change to final to reflect that.

The ALL_CAPS notation is common for constants.

for (int i = 0 ; i < width; i++)
    {
    for (int j = 0; j < height; j++)
        {
        yOffset = pixelHeight - cellSize *(j+1) ;
        Rectangle r = new Rectangle(xOffset,
                                    yOffset,
                                    cellSize,
                                    cellSize);
        crop(r);
        }
      xOffset = pixelWidth - cellSize * (i+1);
     }

You can write this as

for (int xOffset = PIXEL_WIDTH - CELL_SIZE; xOffset >= 0; xOffset -= CELL_SIZE)
{
    for (int yOffset = PIXEL_HEIGHT - CELL_SIZE; yOffset >= 0; yOffset -= CELL_SIZE)
    {
        Rectangle r = new Rectangle(xOffset,
                                    yOffset,
                                    CELL_SIZE,
                                    CELL_SIZE);
        crop(r);
    }
}

Then you don't need i or j at all and you don't have to calculate the offsets further.

I also changed the indent of the brackets to match the more common practice of lining up with the for declarations rather than the block contents.

You may want to do this in the other order:

for (int xOffset = 0; xOffset < PIXEL_WIDTH; xOffset += CELL_SIZE)
{
    for (int yOffset = 0; yOffset < PIXEL_HEIGHT; yOffset += CELL_SIZE)
    {

As that is easier to read. Not sure how much order matters.

If you need the original order exactly, you may need an extra loop. The original order started with an xOffset of 0 and then went to PIXEL_WIDTH - CELL_SIZE.

If you are committed to the i and j method, consider starting them at 1, as so

for (int i = 1 ; i <= width; i++)
    {
    for (int j = 1; j <= height; j++)
        {
        yOffset = pixelHeight - cellSize * j;

Then you don't have to add 1 to i and j each time.

\$\endgroup\$
  • \$\begingroup\$ great code review. Very much appreciated. Now my code is upto standards ! \$\endgroup\$ – gansub Aug 27 '15 at 15:40
  • \$\begingroup\$ If I am committed to the i and j method and I don't start at 0 I do not get the correct total number of subsets. Total number of subsets = 40 * 34 = 1360. \$\endgroup\$ – gansub Aug 27 '15 at 15:53
  • \$\begingroup\$ The last rectangle(going from bottom to top) should have x and y coordinates as 0,0 \$\endgroup\$ – gansub Aug 27 '15 at 15:54
  • \$\begingroup\$ @gansub You have to make all three changes to switch from 0 to 1. 0 becomes 1, < becomes <=, (i+1) becomes i. If you skip the second change in both loops, it will only do 39 * 33 subimages. In the original code, you should calculate the xOffset before the j loop rather than after if you want to do 0,0 last. The first version that I wrote should end with 0,0. \$\endgroup\$ – mdfst13 Aug 27 '15 at 17:47
  • \$\begingroup\$ great thanks ! Much appreciated. I look forward to reading more of your answers ! \$\endgroup\$ – gansub Aug 28 '15 at 2:11

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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