Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have two loops which I am using to create a file, if the clientID in the client loop is the same as the id in the valuation loop then create a row. Then if the month value is the same as the previous month value we need to add up the values.

This all works fine but i was wondering if anyone can think of a better way?

        for (Clients cl : clientList) {
            //in loop so use stringbuilder
            StringBuilder sb = new StringBuilder();
            sb.append(cl.getId());
            sb.append(",");
            sb.append(cl.getFirstName());
            sb.append(" ");
            sb.append(cl.getLastName());
            sb.append(",");
            row = sb.toString();
            for (Valuations vl : valList) {
                year = vl.getYear();
                    //if we are on a new month
                    if (!month.equals(vl.getMonth()) && !month.equals("")) {
                        //add row and reset values
                        if(!vl.isNanError()){
                        data.add(row + month + "," + year + ",£" + cash);
                        }
                        cash = 0;
                        month = "";
                        done = false;
                    }
                    if (cl.getId().equals(vl.getClientId())) {
                        if (!done) {
                            row = sb.toString();
                            cash = 0;
                            month = vl.getMonth();
                            done = true;
                        }
                    }
                    cash += vl.getValue();
            }
        }
        //add the last row after the loop breaks.
        data.add(row + month + "," + year + ",£" + cash);
share|improve this question
add comment

1 Answer

up vote 2 down vote accepted

If you add a toString() method inside the client class you could, replace this code:

//in loop so use stringbuilder
            StringBuilder sb = new StringBuilder();
            sb.append(cl.getId());
            sb.append(",");
            sb.append(cl.getFirstName());
            sb.append(" ");
            sb.append(cl.getLastName());
            sb.append(",");
            row = sb.toString();

for just:

row = sb.toString();

Furthermore, you can append the second with the third if condition inside the second loop, this:

if (cl.getId().equals(vl.getClientId())) {
                    if (!done) {
                        row = sb.toString();
                        cash = 0;
                        month = vl.getMonth();
                        done = true;
                    }
                }

can became this:

  if (!done && cl.getId().equals(vl.getClientId()))
    {
                            row = sb.toString();
                            cash = 0;
                            month = vl.getMonth();
                            done = true;
    }

Another thing, in the above code the row = sb.toString instruction should it be remove? Since you are calculated the row right below the first loop:

row = sb.toString();
for (Valuations vl : valList) {
share|improve this answer
    
Thanks that's a great help, I'm trying to make the code as efficient as possible, I find it quite difficult when dealing with loops in loops lol –  KPZ Feb 11 '13 at 8:37
    
@KPZ Yes, especially if you have to permute all (clients) against all (Valuations). Maybe you could try to parallelize the outer loop. –  dreamcrash Feb 11 '13 at 14:43
    
Sorry I'm not sure what you mean by 'parallelize the outer loop'? –  KPZ Feb 11 '13 at 15:33
    
I am referring about using multi-thread. Each thread would take a range of clients to compute, of course you may have change quite a bit your code. –  dreamcrash Feb 11 '13 at 15:57
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.