Logo Questions Linux Laravel Mysql Ubuntu Git Menu
 

Refactoring for loop with each iteration setting a different property

I've been shown the following Java code as part of a code review and it's quite frankly hideous. However, I'm at a loss as to what the best way to re-write it is.

We have a list that's always in the same order (0-5) and each index corresponds to a different property that is required to be set. Using groovy it would be easy as it would be myObject."setLine${i}" or similar but how can we achieve the same in plain Java?

    SomeObject myObject = new SomeObject();
    List<String> lines = new ArrayList<>(){{
    add("Line 1");
    add("Line 2");
    add("Line 3");
    add("Line 4");
    add("Line 5");
    add("Line 6");
}};

for(int i=0; i< lines.size(); i++){
    if(!StringUtils.isBlank(lines.get(i))){
        String line = lines.get(i);
        // line1
        if(i==0){
            myObject.setLine1(line);
        }
        // line2
        if(i==1){
            myObject.setLine2(line);
        }
        // line3
        if(i==2){
            myObject.setLine3(line);
        }
        // line4
        if(i==3){
            myObject.setLine4(line);
        }
        // line5
        if(i==4){
            myObject.setLine5(line);
        }
        //line6
        if(i==5){
            myObject.setLine6(line);
        }
    }
}

I know that for:each can provide us with an index but it's the "dynamic" method calling that I'm querying. Any suggestion welcome - I'm considering that the MyObject class should maybe have a method to take in a list of address strings so that it doesn't couple quite so tightly by exposing the number of lines to the caller (ie if we add/remove a "setLineX" method we don't want to have to update the caller).

like image 986
Al Sweetman Avatar asked Feb 11 '23 02:02

Al Sweetman


1 Answers

The first issue is in myObject. Having set methods for each line is, as you can see, tedious. How about something like this?

class MyObject{
    private Map<Integer,String> lines = new HashMap<>();

    public void setLine(int lineNumber, String line){
        lines.put(lineNumber,line);
    }

    public String getLine(int lineNumber){
        lines.get(lineNumber);
    }

}

Now you can simply do

for(int i=0;i<lineListSize;i++){
    myOjbect.setLine(i,lineList.get(i));
}

EDIT: As Fabian just pointed out, you could do the same with a List.

class MyObject{
    private List<String> lines = new ArrayList<>();

    public void setLine(int lineNumber, String line){
        lines.add(lineNumber,line);
    }

    public String getLine(int lineNumber){
        lines.get(lineNumber);
    }

}

EDIT2: I can never leave well-enough-alone. You can make the transfer even cleaner with

class MyObject{
    ... 
    // Collection, getter and setter omitted
    ...
    public void addAllLines(List<String> lines){
       // implementation varies depending on collection,
       // but List would simply be "this.lines.addAll(lines);" 
    }

    public List<String> getAllLines(){
       // Again, depends on the type of Collection used by 
       // this instance.
       // Probably want to return a copy if thread safety is an issue.
    }
}
like image 147
MadConan Avatar answered Feb 13 '23 05:02

MadConan