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
and3
.
@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.