Is there any way to refactor this code or re-write it in a compact form?

Question

I'm working on an android app that produces image effects on an image. Below is the snapshot of my app:

As you can see, on the bottom - there is a horizontal scrollbar, and as the user touches one of the images on the horizontal scrollbar, the same effect is applied on the above image.

I've a total of 26 image effects and therefore 26 images in the horizontal scrollbar. Now, in my code, I've to find all those images and set their onClickListener()'s to a one particular listener.

I'm accomplishing the task in the following way:

    sepiaGreenishImage      = (ImageView) findViewById(R.id.sepiaGreenish);
    embossImage             = (ImageView) findViewById(R.id.emboss);
    sharpenImage            = (ImageView) findViewById(R.id.sharpen);
    slightYellowishImage    = (ImageView) findViewById(R.id.ligth_yellow);
    slightBluishImage       = (ImageView) findViewById(R.id.light_blue);
    slightReddishImage      = (ImageView) findViewById(R.id.light_red);
    slightGreenishImage     = (ImageView) findViewById(R.id.light_green);
    negativeImage           = (ImageView) findViewById(R.id.negative);
    greyScaleImage          = (ImageView) findViewById(R.id.greyscale);
    tintSeventyImage        = (ImageView) findViewById(R.id.tint_at_70);
    tintThirtyImage         = (ImageView) findViewById(R.id.tint_at_30);
    snowImage               = (ImageView) findViewById(R.id.snow);
    darkImage               = (ImageView) findViewById(R.id.darken);
    noiseImage              = (ImageView) findViewById(R.id.noise);
    flipImage               = (ImageView) findViewById(R.id.flip);
    rotateImage             = (ImageView) findViewById(R.id.rotate);
    gaussianBlurImage       = (ImageView) findViewById(R.id.blur);
    reddishImage            = (ImageView) findViewById(R.id.reddish);
    bluishImage             = (ImageView) findViewById(R.id.bluish);
    greenishImage           = (ImageView) findViewById(R.id.greenish);
    blackFilterImage        = (ImageView) findViewById(R.id.black_filter);
    increasedSepiaImage     = (ImageView) findViewById(R.id.increased_sepia);
    spiaBluishImage         = (ImageView) findViewById(R.id.sepia_bluish);
    brightImage             = (ImageView) findViewById(R.id.brighten);
    mirrorImage             = (ImageView) findViewById(R.id.mirror);

And then the following way, I'm setting onClickListener:

    sepiaGreenishImage.setOnClickListener(this);
    embossImage.setOnClickListener(this);
    sharpenImage.setOnClickListener(this);
    slightYellowishImage.setOnClickListener(this);
    slightBluishImage.setOnClickListener(this);
    slightReddishImage.setOnClickListener(this);
    slightGreenishImage.setOnClickListener(this);
    negativeImage.setOnClickListener(this);
    greyScaleImage.setOnClickListener(this);
    tintSeventyImage.setOnClickListener(this);
    tintThirtyImage.setOnClickListener(this);
    snowImage.setOnClickListener(this);
    darkImage.setOnClickListener(this);
    noiseImage.setOnClickListener(this);
    flipImage.setOnClickListener(this);
    rotateImage.setOnClickListener(this);
    gaussianBlurImage.setOnClickListener(this);
    reddishImage.setOnClickListener(this);
    bluishImage.setOnClickListener(this);
    greenishImage.setOnClickListener(this);
    blackFilterImage.setOnClickListener(this);
    increasedSepiaImage.setOnClickListener(this);
    spiaBluishImage.setOnClickListener(this);
    brightImage.setOnClickListener(this);
    mirrorImage.setOnClickListener(this);

Now, my question is how can I apply any kind of refactoring? Because I'm repeating myself a lot. Any kind of foreach loop or similar thing might help me a lot!

Put the IDs in a collection, create the image for each, and set the click listener? What make the most sense depends on how else you use them. — Dave Newton, May 04 '13 at 16:32
@vmironov Can I refer current instance of my class-**this** in XML layout? If so, how? If you provide me with a similar question(on setting onClickListener in XML) on the stackoverflow.com, it would be a pleasure! — Arslan Ali, May 04 '13 at 16:39
@ArslanAli, you can just specify a method name that should be called when `ImageView` is clicked. You can find a more detailed information here [android:onClick vs setOnClickListener](http://stackoverflow.com/questions/4153517/how-exactly-does-the-androidonclick-xml-attribute-differ-from-setonclicklistene) — Vladimir Mironov, May 04 '13 at 16:44

score 4 · Accepted Answer · answered May 04 '13 at 16:50

To do this programmatically, you could do something like:

ViewGroup buttonGroup = (ViewGroup) findViewById(R.id.image_effect_buttons);
for (int i = 0; i < buttonGroup.getChildCount(); i++) {
    ImageView imageView = (ImageView) buttonGroup.getChildAt(i);
    imageView.setOnClickListener(this);
}

Where "image_effect_buttons" is the id of whatever view (guessing a LinearLayout?) contains those image buttons. Note that this will cause problems if that ViewGroup contains any other children aside from the ImageViews you want to attach this listener to, but it's the simplest way to avoid enumerating them explicitly.

score 2 · Answer 2 · edited May 04 '13 at 16:40

2

Iterate over Arrays.asList(obj1, obj2, obj3...) and do it once in a for loop.

edited May 04 '13 at 16:40

Dave Newton

158,873
26
254
302

answered May 04 '13 at 16:36

Michal Borek

4,584
2
30
40

score 1 · Answer 3 · answered May 22 '14 at 19:06

You may be better off with a ViewPager vs the horizontal scroll view. The advantages with going with a ViewPager solution is that you could be recycling views and setting there properties at runtime.

A typical setup of this would include:

ViewPager declared in layout
Some type of Model Class to hold all relevant for each image setting
A fragment that may hold three image views or radio button group with drawables, etc
A PagerAdapter that contains an array list of fragments and models

It seems like a lot more code but it's definitely more extensible when it comes time to change data, or do any re-factoring. You also get the cool android page "swoosh" when changing items as well.

Reference: http://developer.android.com/training/implementing-navigation/lateral.html#horizontal-paging

Is there any way to refactor this code or re-write it in a compact form?

3 Answers3