Returning a different but similar “increasing” string depending on instance argument

Posted on

Problem

I have to work with a function that takes an instance of a class as an input, and depending on that instance, I have to return a specific string. It works, but it has a lot of repetitive code. How can I make it more efficient?

private getDescription(ScreenViewer o) {  
    String output = "";      
    if(Utils.isTotalPagesVisible()) {  
       String total = 5;  
       if(o instance of ScreenViewerPage1) {  
           output = " 1 out of " + total;  
       }  
       if(o instance of ScreenViewerPage2) {  
           output = " 2 out of " + total;  
       }   
       if(o instance of ScreenViewerPage3) {  
           output = " 3 out of " + total;  
       }   
       if(o instance of ScreenViewerPage4) {  
           output = " 4 out of " + total;  
       }  
       if(o instance of ScreenViewerPage5) {  
           output = " 5 out of " + total;  
       }  
    }  
    else {  
        String total = 3;  
       if(o instance of ScreenViewerPage1) {  
           output = " 1 out of " + total;  
       }  
       if(o instance of ScreenViewerPage2) {  
           output = " 2 out of " + total;  
       }   
       if(o instance of ScreenViewerPage3) {  
           output = " 3 out of " + total;  
       }   
    }  
   return "Description:" + output;  
}

Solution

The easiest way is to add a method to the ScreenViewer class which returns the page value, eg:

public abstract class ScreenViewer {
    public abstract int getPageCount();
}

public class ScreenViewerPage1 {
    @Override
    public getPageCount() {
        return 1;
    }
}

private getDescription(ScreenViewer o) {    
    if(Utils.isTotalPagesVisible()) {  
        int total = 5;          
    } else {  
        int total = 3;  
    } 
    return "Description:" + o.getPageCount() + " out of " + total ;  
}

But I would rethink your whole approach. What if you want to add ScreenViewerPage6? It’s hard to say how exactly your code could be rewritten without knowing how the classes look, but I would assume that it should be possible with lists, etc.

Misc

  • Your indentation is off.
  • o is not a good variable name.
  • use else if when appropriate.
  • I would not hardcode magic numbers such as 5 and 3.

@tim’s answer is better, but an approach that will work without the conditions being instance of is

private getDescription(ScreenViewer o) {
    String number;

    if (o instanceof ScreenViewerPage1) {
        number = "1";
    }
    else if (o instanceof ScreenViewerPage2) {
        number = "2";
    }
    else if (o instanceof ScreenViewerPage3) {
        number = "3";
    }

    if (Utils.isTotalPagesVisible()) {
        String total = "5";

        if (o instanceof ScreenViewerPage4) {
            number = "4";
        }
        else if (o instanceof ScreenViewerPage5) {
            number = "5";
        }
    }
    else {
        String total = "3";
    }

    return "Description: " + number + " out of " + total;
}

This approach will work in situations where you’re not dealing with instance of.

Note that this assumes that o will always be an instance of one of these classes. If not, you’d need to change the return condition. Perhaps

    return number == null ? "Description:" : "Description: " + number + " out of " + total;

If you can also rely that o will never be a ScreenViewerPage4 or ScreenViewerPage5 if Utils.isTotalPagesVisible() is false, you can put the entire thing in the if/else if block.

Leave a Reply

Your email address will not be published. Required fields are marked *