Avoiding duplicate code blocks

Question

I have something like this in my code:

        switch (item.getItemId()) {
            case R.id.search:
                if (currentFragment == null || !(currentFragment instanceof ClipsFragment)) {
                    ClipsFragment clipsFragment = new ClipsFragment();
                    fragmentTransaction.replace(R.id.container,
                            clipsFragment).commit();
                    currentFragment = clipsFragment;
                } else {
                    fragmentTransaction.replace(R.id.container,
                            currentFragment).commit();
                }
                currentDrawerItem = R.id.search;
                return true;
            case R.id.download_managers:
                if (currentFragment == null || !(currentFragment instanceof DownloadFragment)) {
                    DownloadFragment downloadFragment = new DownloadFragment();
                    fragmentTransaction.replace(R.id.container,
                            downloadFragment).commit();
                    currentFragment = downloadFragment;
                } else {
                    fragmentTransaction.replace(R.id.container,
                            currentFragment).commit();
                }
                currentDrawerItem = R.id.search;
                return true;

            default:
                return false;
        }

As you can see there are two similar switch cases.
Any way to convert them to a method using Java 7?
I tried a bit but the instanceof is tricky.
None of my attempts are worth posting here.

Note: Fragment and DownloadFragment extend Fragment.

Well, yes you can. Are ClipsFragment and DownloadFragment related , or implement a common interface? Read-up on re-factoring. — OldProgrammer, Aug 20 '16 at 16:47
I have no idea why I'm getting down votes. Is the question not clear? — Binoy Babu, Aug 20 '16 at 16:49

OneCricketeer · Accepted Answer · 2016-08-20T18:27:44.780

1

They both extend Fragment

Okay, then you need really need to use the specific subclasses other than to create an instance. Just declare the current Fragment as those new instances.

Is this cleaner for you?

boolean handled = true;
switch (item.getItemId()) {
    case R.id.search:
        if (currentFragment == null || !(currentFragment instanceof ClipsFragment)) {
            currentFragment = new ClipsFragment();
        }
        break;
    case R.id.download_managers:
        if (currentFragment == null || !(currentFragment instanceof DownloadFragment)) {
            currentFragment = new DownloadFragment();
        }
        break;
    default:
        handled = false;
}

if (currentFragment != null) {
    fragmentTransaction.replace(R.id.container,
            currentFragment).commit();
}
if (handled) {
    currentDrawerItem = item.getItemId();
}

return handled;

edited Aug 20 '16 at 18:27

answered Aug 20 '16 at 16:59

OneCricketeer

179,855
19
132
245

Oh yeah, it's definitely cleaner. Although I was thinking of the first part mainly. Just curious is all. – Binoy Babu Aug 20 '16 at 17:02
Your wipe of the duplication is good. Nevertheless, I find that the `handled` boolean you introduced is not very suitable. You handle both `currentFragment!=null` and `handled` in your code. I find it error prone. Fail-fast with a return false for default case ease the reading. A single return statement is not always desirable. – davidxxx Aug 20 '16 at 18:18
@davidhxxx Sure. I just needed stuff after the switch-case, so I replaced the direct return statements with a `break` – OneCricketeer Aug 20 '16 at 18:25

davidxxx · Answer 2 · 2016-08-20T18:36:04.647

As you can see there are two similar switch cases. Any way I could convert them to a method using Java 7? I tried a bit but the instanceof is tricky.

To avoid switch cases and replace them by a direct method call, you could use object and polymorphism. In your case, you could introduce an interface with a method :

public interface  Processing{
    Fragment doProcess();
}

You could have two implementations of this (according to items Id in your code) :

DownloadManagerProcessing
ReadProcessing

Then, you could use a factory which returns the convenient Processing instance :

public ProcessingFactory{
  ..
  Map<Integer, Processing> processingByItemId;
  ..
  public Processing getProcessing(int itemId){
     return processingByItemId.get(itemId);
   }
..
}

You switch case :

 switch (item.getItemId()) {
            case R.id.search:
             ... 
            case R.id.download_managers:
            ...
            default:
            return false;
  }

would replace by :

Processing processing = ProcessingFactory.getProcessing(itemId);
if (processing==null){
   return false;
}
Fragment fragment = processing.doProcess();
return true;

In this solution, we rely always on the itemId but the work for associating them to a processing is isolated.
So, if needed, you could add other types of processing in the factory relying on the itemId without worry about its mapping.

Edit (implementation starting from clean code)

By starting from a code with no strong duplication as cricket_007 did, you could again use method to avoid a switch case. In this version, you can use a factory to retrieve the matching instance for an ItemId :

public FragmentFactory{
  ..

  ..
  public Fragment createInstanceIfNeeded(Fragment currentFragment, int itemId){


     if(itemId ==...){
        if (!(currentFragment instanceof ClipsFragment)) {
          return new ClipsFragment();
       }
     }
     else if(itemId == ..){
         if (!(currentFragment instanceof DownloadFragment)) {
          return new DownloadFragment();
       }
     }
     return currentFragment;
   }
..
}

The instanceof is needed in your case, because you wish change the implementation if it not which you expect. If you want to avoid it, you should have a public method getType() in Fragment which returns a String or an Enum and which will be implemented by each children class. But in a some way it's a instanceof like.

and in your client code you could do :

currentFragment = FragmentFactory.createInstanceIfNeeded(item.getItemId());

if (currentFragment==null){
   return false;
}

fragmentTransaction.replace(R.id.container,
            currentFragment).commit();
currentDrawerItem = item.getItemId();

return true;

Personally, I don't like using switch case. I find it error-prone.

Is the interface really necessary? `Map` seems to be the only needed refactoring. Creating one `Processing` implementation for each `Fragment` class seems unnecessary. — OneCricketeer, Aug 20 '16 at 17:46
After you refactor which removes the duplication, you are right, it is an overhead since the custom process is light : just an instantiation. But as the author of the question would know how do the processing in a OOP fashion, I have taken as start point of this code and I did abstraction of the duplication to show a possible solution. But finally, I think it's interesting to edit my answer to show the solution by starting a code without duplication. — davidxxx, Aug 20 '16 at 17:56

Avoiding duplicate code blocks

2 Answers2