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.

What would your prefered loop type be for a case like this:

StringBuilder sb = new StringBuilder();
for(Integer id : idList) {
    sb.append("?,");
}

In short: In dependecy to the size of a List i want to add stuff to a String. The above example produces a "unused variable" warning.

I'm thinking of these alternatives:

for(int i=0; i<idList.size(); i++) {
    // ...
}

I don't really like all the extra typing there...

Iterator it = idList.iterator();
while(it.hasNext()) {
    // ...
    it.next();
}

The loop head itself looks nice, but i end up typing a lot of extra code (getting the iterator, moving to the next element...)

What i'd like to do would be something like that:

for(idList) {
    // ...
}

Is there a better style/What is the best kind of style for that kind of loop?

share|improve this question
add comment

5 Answers

up vote 15 down vote accepted

I would use a "normal" string repeating method:

String s = repeat("?,", idList.size());

Unfortunately Java doesn't have such a method in the standard API. So you could write one yourself:

String repeat(String s, count i) {
  StringBuilder sb = new StringBuilder();
  while (i-- > 0) {
     ab.append(s);
  }
  return sb.toString();
}

or use one provided by several "standard add-on APIs" such as Apache Commons' StringUtils.repeat().

share|improve this answer
1  
+1 That is a good alternative (or perhaps better) solution than using closures indeed! :) I was thinking way too technical hehe. :) –  Steven Jeuris Jun 23 '11 at 10:25
    
I don't dislike the answer, but I think the construct "while(i-- >0) is not as clear as a more often seen for(int i=0; i<count; i++).. This is more intuitive IMO. –  Eric-Karl Jul 22 '11 at 0:22
add comment

The second example is by far the clearest in expressing your intention. You want to do something an amount of times, in this case, idList.size().

for( int i = 0; i < idList.size(); ++i ) {
    // ...
}

In the third one you are basically just writing what the first example already generates, so that's definitely not an improvement.

You do have a point that a for loop is rather a big code construct to express just executing a piece of code an amount of times. Ruby's times operator is made exactly for this purpose. In other languages there are some code constructs available which could help you. Extension methods in C# allow you to create a similar operator as the times operator of Ruby.

For Java I don't believe a more concise syntax is possible since closures aren't supported in Java by default.


As in RoToRa's answer, a simple String helper class containing a Repeat function is most likely the cleanest solution for you. It doesn't provide the functionality to repeat any type or operation, but it does provide the concise syntax you are looking for.

share|improve this answer
    
I've heard noises that Java 7 will support closures :) –  Michael K Jun 23 '11 at 13:21
    
@Michael K: Java will have to if they don't want to lose programmers to other languages. ;p –  Steven Jeuris Jun 23 '11 at 19:31
add comment

I suggest to use Strings.repeat() from Google Guava library:

String result = Strings.repeat("?,", idList.size());
share|improve this answer
add comment

I would go with the first alternative and use @SuppressWarnings.

If you need this pattern more often, how about an object oriented solution:

public static class Repeat<T> implements Iterable<T> {
    private final T[] ts;
    private final int n;

    public Repeat(int n, T ... ts) {
        this.ts = ts;
        this.n = n;
    }

    public Iterator<T> iterator() {
        return new Iterator<T>() {
            int count = 0;   

            public boolean hasNext() {
                return count < n;
            }

            public T next() {
                if (! hasNext()) {
                    throw new NoSuchElementException();
                }
                return ts[(count++) % ts.length];
            }

            public void remove() {
                throw new UnsupportedOperationException("Not supported yet.");
            }

        };
    }
    public static <U> Repeat<U> repeat(int n, U ... us) {
        return new Repeat<U>(n, us);
    }
}

//usage
import static blabla.Repeat.*;
...
for(String s : repeat(idList.size(), "?,")) {
   sb.append(s);
}
share|improve this answer
    
So where exactly does this improve on the given example? I don't see any advantage (or extra reuseability) in using this class. –  Steven Jeuris Jun 23 '11 at 10:18
    
It's a nice OO approach (+1), but far too verbose for the presented problem (-1). Thus no vote in either direction from me :-) –  RoToRa Jun 23 '11 at 10:25
1  
As I said, I would consider this only when I had such a case multiple times, and the first alternative shown by the TO is completely okay. I added "cyclic" behavior in order to make it a little bit more reuseable. I don't understand @Steven Jeuris complaints, whose solution doesn't look very convinicing, by the way. And an Iterable is more flexible than @RoTaRa's solution, which solves the problem very concisely but is not reuseable at all. I don't say that everybody needs to actually like my solution, but I think nothing is wrong with it and the downvote is bullshit. –  Landei Jun 23 '11 at 11:34
1  
@Steven Jeuris: There is always a trade-off between reuseability and performance. I think it makes sense to look for the most flexible and general solution for a given problem, and having an iterator that gives back a fixed list of things is certainly useful. I think the performance hit isn't as bad as you think, and you don't seem to bother much about performance yourself when suggesting closures, which would be exactly as "bad" as an Iterable performance-wise. And again, your solution with a loop var and range checks and inc doesn't look nice to me. But I would never downvote it. –  Landei Jun 24 '11 at 7:48
1  
This "simplifies" for(int i=0; i<foo.size(); ++i) how? I'm all for the "don't repeat yourself" maxim but this is pushing it. The best part is "Not implemented yet" - are you planning to implement removal in the future? –  asveikau Jun 29 '11 at 6:46
show 2 more comments

I'd personally use the second version, i.e. the for loop because it is clearer. The first example looks like you've introduced an error by not using the id, the third adds two extra lines of code, but isn't too bad on the eyes.

I think I would also create a local cache of the size() so that you can avoid the function call on every iteration of the loop. But this is a compromise between speed, the size of the list, readability and whether the list size will change during the loop.

int size = idList.size();
for (int i = 0; i < size; ++i) {...}

If this is a heavily called inner loop the saving yourself a few tens of thousands of function calls to return back a "constant" can save some time. But if you do something like this it might be worth a comment to say why you did it "differently" to an normal expected way.

I like for loops because they are very self contained; you get start value, end condition and increment operation all on one line whereas a while loop requires hunting for the "++" somewhere within the while block itself.

I think keeping your intention simple and clearly seen is more important going forward when your code has to be maintained. Remember to code as if the person maintaining your code is serial axe murderer who know where you live!

share|improve this answer
    
While I totally agree with your answer, it doesn't add much more to the given ones. Consider expanding it a bit. –  Steven Jeuris Jun 24 '11 at 11:15
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.