6
\$\begingroup\$

I have this code which runs when an Async HTTP call is made on a HorizontalScrollView

            //These 3 defined outside of the method
            ObjectAnimator animatorR = null;
            ObjectAnimator animatorL = null;
            boolean loop = true;

            Inside the method
            if (animatorL != null) animatorL.cancel();
            animatorL = null;
            if (animatorR != null) animatorR.cancel();
            animatorR = null;
            HorizontalScrollView sv = (HorizontalScrollView) findViewById(R.id.scroll_view);
            loop = true;
            sv.setOnTouchListener(new View.OnTouchListener() {
                @Override
                public boolean onTouch(View v, MotionEvent event) {
                    animatorL.cancel();
                    animatorR.cancel();
                    loop = false;
                    sv.setOnTouchListener(null);
                    return false;
                }
            });
            animatorR = ObjectAnimator.ofInt(sv, "scrollX", sv.getMaxScrollAmount()).setDuration(4000);
            animatorL = ObjectAnimator.ofInt(sv, "scrollX", 0).setDuration(4000);

            animatorR.removeAllListeners();
            animatorR.addListener(new Animator.AnimatorListener() {
                @Override
                public void onAnimationStart(Animator animation) {

                }

                @Override
                public void onAnimationEnd(Animator animation) {
                    Log.d("AnimR", "Ended");
                    if (loop && animatorL != null) animatorL.start();
                }

                @Override
                public void onAnimationCancel(Animator animation) {

                }

                @Override
                public void onAnimationRepeat(Animator animation) {

                }
            });
            animatorR.start();

            animatorL.removeAllListeners();
            animatorL.addListener(new Animator.AnimatorListener() {
                @Override
                public void onAnimationStart(Animator animation) {

                }

                @Override
                public void onAnimationEnd(Animator animation) {
                    Log.d("AnimL", "Ended");
                    if (loop && animatorR != null) animatorR.start();
                }

                @Override
                public void onAnimationCancel(Animator animation) {

                }

                @Override
                public void onAnimationRepeat(Animator animation) {

                }
            });
        }

This is my first time using Animators and the code seems very clunky. Where could I make improvements to this code?

\$\endgroup\$
4
  • 1
    \$\begingroup\$ Hi, welcome to CodeReview.SE. Can you please provide the whole class or whatever this snippet was supposed to be in ? \$\endgroup\$
    – SylvainD
    Mar 26, 2014 at 13:58
  • \$\begingroup\$ will you please look at the code and make sure that your indentation is correct? it looks like you have an if statement that is out of alignment \$\endgroup\$
    – Malachi
    Mar 26, 2014 at 16:01
  • \$\begingroup\$ @Josay the code this from is about 1000 lines long, I just cut out the part I wanted looking at at the moment, should I post all of it? @ Malachi yeah ones out of line when I copied it here, it's inline in the actual file. \$\endgroup\$
    – TMH
    Mar 26, 2014 at 22:05
  • \$\begingroup\$ You don't need to post the whole file. I see a couple of potential improvements. I will write up an answer either later today or tomorrow! \$\endgroup\$ Mar 27, 2014 at 19:53

1 Answer 1

3
\$\begingroup\$

The biggest change I can see here would be to extract a class out of the two anonymous inner classes. The two variations are exactly the same except for which ObjectAnimator they should call next, and which logging tag to use. So let's extract a class and pass those values to its constructor: (Place the code below inside the same class as you are using right now)

private class MyAnimListener implements Animator.AnimatorListener {
    private ObjectAnimator  other;
    private final String    logMessage;

    public MyAnimListener(String logMessage, ObjectAnimator startOther) {
        this.logMessage = logMessage;
        this.other = startOther;
    }

    @Override
    public void onAnimationStart(Animator animation) {
    }

    @Override
    public void onAnimationEnd(Animator animation) {
        Log.d(logMessage, "Ended");
        if (loop && other != null) 
            other.start();
    }

    @Override
    public void onAnimationCancel(Animator animation) {
    }

    @Override
    public void onAnimationRepeat(Animator animation) {
    }
}       

You could then use this code to add the listeners:

animatorR.addListener(new MyAnimListener("AnimR", animatorL));
animatorL.addListener(new MyAnimListener("AnimL", animatorR));

However, if your two animator instances often switch to/from null, I would think about what it actually is that you are trying to achieve. (Honestly I'm not sure what you are trying to do, which can make this a bit of an X-Y problem).

If you really really want the two animator instances to switch to/from null, then you will have to use two variables for the MyAnimListener and modify its ObjectAnimator other field, something like this:

    // Variable declarations
animRListener = new MyAnimListener("AnimR", animatorL);
animatorR.addListener(animRListener);
animLListener = new MyAnimListener("AnimL", animatorR);
animatorL.addListener(animLListener);

// Later on
animatorR = null;
animLListener.other = null;

Besides this, I actually think your code might be cleaner than it looks, if that makes any sense...


I would recommend using the same tag parameter for all Log calls throughout your entire map. The Logcat utility can filter on a specific tag, by using it like this:

adb -d logcat YourTag:D YourOtherTag:D System.err:W *:S

I usually use something like this to be informed about both my own tags, stack traces from applications gone mad, and some other more or less useful Android messages:

adb -d logcat MyTag:D MyOtherTag:D MyThirdTag:D dalvikvm:I AndroidRuntime:E ResourceType:W PackageManager:I System.err:W *:S
\$\endgroup\$
1
  • \$\begingroup\$ Thanks for the reply :). The reason they're going to null at the moment is because I had a problem with looping, when that block of code ran for the second time (And third etc...) the animation would happen twice, I tried using .cancel() but it didn't seem to fix it, but setting it to null did. \$\endgroup\$
    – TMH
    Mar 31, 2014 at 8:27

Your Answer

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

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