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;
        }
    }
}
\$\endgroup\$
7
  • \$\begingroup\$ What type does orientation have? \$\endgroup\$
    – FD_
    Feb 13, 2014 at 18:29
  • \$\begingroup\$ It has type String. \$\endgroup\$
    – bhaskarc
    Feb 13, 2014 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, 2014 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, 2014 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, 2014 at 18:47

2 Answers 2

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

you have:

orientation.contentEquals("landscape"

better practice for null safe:

"landscape".equals(orientation)

even better, use constants or enums!

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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