current community

your communities

more stack exchange communities

Stack Exchange Inbox Reputation and Badges
tour help
Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 1 down vote favorite

I am trying to set a OnClickListener to a image in a loop. If the params platform is "android" then use market app, instead of default browswer. Is there a better solution to my exception handling or eliminatingsome of the if-else ?

private void setupListener(ImageView image, final String platform,
        final String urlLink) {
    image.setOnClickListener(new View.OnClickListener() {

        @Override
        public void onClick(View v) {
            if (platform.equalsIgnoreCase(PLATFORM_ANDROID)) {
                // open with market app
                String packageName = extractPackageName(urlLink);
                if (packageName != null) {
                    try {
                        Intent market = new Intent(Intent.ACTION_VIEW);
                        market.setData(Uri.parse("market://details?id="
                                + packageName));
                        activity.startActivity(market);
                    } catch (Exception e) {
                        e.printStackTrace();
                        startInBroswer(urlLink);
                    }
                } else {
                    startInBroswer(urlLink);
                }

            } else {
                // open with default broswer.
                startInBroswer(urlLink);
            }

        }
    });
}

private void startInBroswer(String urlLink) {
    Intent browser = new Intent(Intent.ACTION_VIEW, Uri.parse(urlLink));
    activity.startActivity(browser);
}
share|improve this question

2 Answers 2

up vote 1 down vote accepted

Here is another version with guard clauses:

private void setupListener(final ImageView image, final String platform, 
        final String urlLink) {
    image.setOnClickListener(new View.OnClickListener() {

        @Override
        public void onClick(View v) {
            if (!platform.equalsIgnoreCase(PLATFORM_ANDROID)) {
                startInBroswer();
                return;
            }

            final String packageName = extractPackageName(urlLink);
            if (packageName == null) {
                startInBroswer();
                return;
            }

            try {
                final Intent market = new Intent(Intent.ACTION_VIEW);
                market.setData(Uri.parse("market://details?id=" + packageName));
                activity.startActivity(market);
            } catch (final Exception e) {
                // TODO: log the exception
                startInBroswer();
            }
        }

        private void startInBroswer() {
            final Intent browser = new Intent(Intent.ACTION_VIEW, 
                Uri.parse(urlLink));
            activity.startActivity(browser);
        }
    });
}

Two things to notice:

  1. I've removed the urlLink parameter of the startInBroswer method.

  2. It is a bad idea to use printStackTrace() in Android exceptions.

share|improve this answer
1  
Thanks for the guardclause link –  wtsang02 Jan 30 '13 at 22:35
up vote 2 down vote

Define field listener and assign it as OnClick handler. You'll have only one object. 

View.OnClickListener mListener

@Override
public void onCreate(Bundle savedInstanceState) {

    mListener = new View.OnClickListener() {

        @Override
        public void onClick(View v) {
            if (platform.equalsIgnoreCase(PLATFORM_ANDROID)) {
                // open with market app
                String packageName = extractPackageName(urlLink);
                if (packageName != null) {
                    try {
                        Intent market = new Intent(Intent.ACTION_VIEW);
                        market.setData(Uri.parse("market://details?id="
                                + packageName));
                        activity.startActivity(market);
                    } catch (Exception e) {
                        e.printStackTrace();
                        startInBroswer(urlLink);
                    }
                } else {
                    startInBroswer(urlLink);
                }

            } else {
                // open with default broswer.
                startInBroswer(urlLink);
            }

        }
    }
    super.onCreate(savedInstanceState);
}

private void setupListener(ImageView image, final String platform,
        final String urlLink) {
    image.setOnClickListener(mListener);
}
share|improve this answer
    
Nice tip. Thanks but it doesn't solve the problem with the clauses which I am more concerned about. –  wtsang02 Jan 30 '13 at 22:34

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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