1
\$\begingroup\$

I'm working on an Android app, and at one point I need to retrieve a request code in a callback method and perform some database operations depending on the request code. It started out ok when there were just 2 request codes, but then I had a need to add two more, and I can't really find a good way to avoid a messy looking method.

This is the method right now

@Override
public void onActivityResult(final int requestCode, int resultCode, Intent data) {
    final Context applicationContext = getContext();

    DevLog.v(TAG, "onActivityResult()   requestCode="+requestCode+"   resultCode="+resultCode+"   data="+data);

    if( (requestCode == ACTIVITY_REQUEST_ADD_SENDER ||
            requestCode == ACTIVITY_REQUEST_EDIT_SENDER ||
            requestCode == ACTIVITY_REQUEST_ADD_SOS_RECEIVER ||
            requestCode == ACTIVITY_REQUEST_EDIT_SOS_RECEIVER)
            && resultCode == RESULT_OK
            && data != null
            && applicationContext != null ){

        final String number = getNumberFromContactResultIntent(data);
        if (number != null) {
            AsyncTask.execute(new Runnable() {
                @Override
                public void run() {
                    switch (requestCode) {
                        case ACTIVITY_REQUEST_ADD_SENDER:
                            AppDatabase.db(applicationContext).getSenderDao().insert(new Sender(number));
                            break;
                        case ACTIVITY_REQUEST_EDIT_SENDER:
                            if (mEditSender != null) {
                                mEditSender.number = number;
                                AppDatabase.db(applicationContext).getSenderDao().update(mEditSender);
                            }
                            break;
                        case ACTIVITY_REQUEST_ADD_SOS_RECEIVER:
                            AppDatabase.db(applicationContext).getSosReceiverDao().insert(new SosReceiver(number));
                            break;
                        case ACTIVITY_REQUEST_EDIT_SOS_RECEIVER:
                            if (mEditSosReceiver != null ) {
                                mEditSosReceiver.number = number;
                                AppDatabase.db(applicationContext).getSosReceiverDao().update( mEditSosReceiver );
                            }
                            break;
                    }
                }
            });

        }
    }
    else
        super.onActivityResult(requestCode, resultCode, data);
}

What bugs me the most is the repeated check of the requestCode parameter. First I need to check if it's any of four values (the if), and then I have to perform different operations depending on exactly which of those four values it is (the switch).

If I remove the if( (requestCode ... part and move the switch statement out from the AsyncTask, I would still need to perform the other checks (resultCode == RESULT_OK && data != null ... etc), AND I would have to create one AsyncTask in each case, so I'd basically end up with even more redundant code than there already is.

I would also need to move the

getNumberFromContactResultIntent(data)

call, since it only makes sense for these four request codes and shouldn't be called on any other request codes.

I considered putting the four request codes into an array (they are static final int) and call it something like REQUEST_CODE_GROUP_ADD_OR_EDIT and perform

Arrays.binarySearch(REQUEST_CODE_GROUP_ADD_OR_EDIT, requestCode) 

on it, which would at least reduce the if statement slightly. But it seems a bit silly to turn four simple and efficient == checks into a much more complex operation. It's highly unlikely I'll need to add more similar request codes in the future, and if I do it will most certainly only be a limited number of them (2-4).

Is there a neat way to simplify something like this, where one needs to combine first checking for any of a limited set of values, perform some operations that is common to all of them, and then perform some operations that are specific for each value?

\$\endgroup\$
  • \$\begingroup\$ It would be probably too little for a full feature answer in this site but I hope that this could help: en.wikipedia.org/wiki/Chain-of-responsibility_pattern (It helped me when I had similar situation in my own projects) \$\endgroup\$ – mpasko256 Oct 5 '18 at 14:22
  • \$\begingroup\$ In addition, don't hesitate to extract switch cases or if branches into separate methods or classes. Software always have a tendency to grow fast. Sooner or later you will end up with a requirement to add new cases. It would be better to maintain readability, flexibility and scalability since very beginning! \$\endgroup\$ – mpasko256 Oct 5 '18 at 15:01
  • 1
    \$\begingroup\$ I had a look at the pattern, and I guess it sort of does what I need, but I think the increased complexity is not worth it in this case just to avoid this one "ugly" method. I also considered breaking out the switch or if, and while it could be an improvement, in a sense it would just move the problem elsewhere. I'm still interested in ways to improve these kinds of scenarios, but I actually ended up refactoring my Sender and SosReceiver classes into one Contact class, so the problem sort of solved itself ;) \$\endgroup\$ – Magnus W Oct 5 '18 at 18:20
3
\$\begingroup\$

You could define an abstract class that holds a request code and a command. Then for each code, create a concrete subclass that hold that code and command associated with that code. Then you pass an instance of that concrete subclass, do your null checks, and call the command.

\$\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.