1
\$\begingroup\$

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;
        }
    }
}
Improve this question
\$\endgroup\$
  • \$\begingroup\$ What type does orientation have? \$\endgroup\$ – FD_ Feb 13 '14 at 18:29
  • \$\begingroup\$ It has type String. \$\endgroup\$ – bhaskarc Feb 13 '14 at 18:31
  • \$\begingroup\$ 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. \$\endgroup\$ – Tenfour04 Feb 13 '14 at 18:33
  • \$\begingroup\$ 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. \$\endgroup\$ – Rajan Feb 13 '14 at 18:36
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ – Tenfour04 Feb 13 '14 at 18:47
1
\$\begingroup\$

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);
    }
}
Improve this answer
\$\endgroup\$
  • \$\begingroup\$ thanks buddy..definitely much cleaner and readable than mine. thanks for taking time to redo that :) \$\endgroup\$ – bhaskarc Feb 13 '14 at 19:02
1
\$\begingroup\$

you have:

orientation.contentEquals("landscape"

better practice for null safe:

"landscape".equals(orientation)

even better, use constants or enums!

Improve this answer
\$\endgroup\$

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.