Setting image margins and location based on orientation

Question

This code is for Android but I guess anyone can take a chance at it.

It does what I want it to do. Just that I am not comfortable with its deep level of nesting and too much of value swapping.

Can this be optimized for readability or in compactness?

for(int imagenum, imagenum <76, imagenum++){

    if (orientation.contentEquals("landscape")
            || orientation.contentEquals("reverse landscape")) {
        params.height = imageheight;
        params.width = imagewidth;
        imageButton.setImageResource(R.drawable.imageback);
        if (imagenum <= 38) {

            params.leftMargin = imagenum * xspaceforeachimage;
            params.topMargin = 0;
        } else {
            params.leftMargin = (imagenum - 39) * xspaceforeachimage;
            params.topMargin = topMargin;
        }
    } else if (orientation.contentEquals("portrait")
            || orientation.contentEquals("reverse portrait")) {
        params.height = imagewidth;
        params.width = imageheight;
        imageButton.setImageResource(R.drawable.imagebacklaid);
        if (imagenum <= 38) {
            params.leftMargin = 0;
            params.topMargin = imagenum * xspaceforeachimage;
        } else {
            params.leftMargin = topMargin;
            params.topMargin = (imagenum - 39) * xspaceforeachimage;
        }
    }
}

Something tells me something big is missing from your code above...did you truncate it? If that is literally your code, you don't need a for loop at all--just calculate everything with imagenum=75. — Tenfour04
– Tenfour04, Commented Feb 13, 2014 at 18:33
You can able to use this line orientation.contains("landscape"); instead of this line if (orientation.contentEquals("landscape") || orientation.contentEquals("reverse landscape")) use same thing for "portrait" condition. — Rajan
– Rajan, Commented Feb 13, 2014 at 18:36
I know it's absurd; that's why I asked if you truncated something. imageButton and params are never reassigned inside your for loop, so they will result with whatever you give them in the last run of the loop. — Tenfour04
– Tenfour04, Commented Feb 13, 2014 at 18:47

Tenfour04 · Accepted Answer · 2014-02-13 18:59:52Z

This doesn't save many lines of code, but I think it's more readable and maintainable:

boolean landscape = orientation.contains("landscape");

for(int imagenum = 0; imagenum <76; imagenum++){

    /* Missing code that assigns params and imageButton using imageNum? */

    int shortSideMargin = imagenum * xspaceforeachimage;
    int longSideMargin = 0;
    if (imagenum >= 39)  { //new row or column, shift back and over
        shortSideMargin -= 39 * xspaceforeachimage;
        longSideMargin = topMargin;
    }

    if (landscape) {
        params.height = imageheight;
        params.width = imagewidth;
        params.leftMargin = shortSideMargin;
        params.topMargin = longSideMargin;
        imageButton.setImageResource(R.drawable.imageback);
    } else {
        params.height = imagewidth;
        params.width = imageheight;
        params.leftMargin = longSideMargin;
        params.topMargin = shortSideMargin;
        imageButton.setImageResource(R.drawable.imagebacklaid);
    }
}

thanks buddy..definitely much cleaner and readable than mine. thanks for taking time to redo that :) — bhaskarc
– bhaskarc, Commented Feb 13, 2014 at 19:02

Erik B · Accepted Answer · 2014-03-06 01:20:04Z

1

you have:

orientation.contentEquals("landscape"

better practice for null safe:

"landscape".equals(orientation)

even better, use constants or enums!

answered Mar 6, 2014 at 1:20

Erik B

1112 bronze badges

Add a comment |

Stack Exchange Network

Setting image margins and location based on orientation

2 Answers 2

You must log in to answer this question.

Hot Network Questions

Setting image margins and location based on orientation

2 Answers 2

You must log in to answer this question.

Related

Hot Network Questions