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.

Often I have to skip some actions on the first (or last) loop iteration.

Now I need to append some arguments to an url and separate the arguments by the & character. For example:

http://api.openweathermap.org/data/2.5/weather?q=London&mode=xml&cnt=7

To do so code looks like this:

public static String prepareUrl(String url, List<? extends NameValuePair> arguments) {
    boolean firstIteration = true;
    StringBuilder urlBuilder = new StringBuilder(url);
    for (NameValuePair argument : arguments) {
        if (firstIteration) {
            firstIteration = false;
        } else {
            urlBuilder.append("&");
        }
        urlBuilder.append(argument.getName());
        urlBuilder.append("=");
        urlBuilder.append(argument.getValue());
    }
    return urlBuilder.toString();
}

But it's very annoying to write and a little difficult to read such a code. And it's easy to make a mistake in a loop body like this.

So I write the next class. But I need your help to name the class and its methods properly (I'm pretty bad in English). And please, criticize severely my code.

// I know that class name must be a noun.
// But I can't think of a good name.
// And it seems to me that a comparison with the blank cartridge is a good one.
public abstract class FirstCartridgeIsBlank implements Runnable {

    private boolean firstCartridge;

    public FirstCartridgeIsBlank() {
        firstCartridge = true;
    }

    public final void run() {
        if (firstCartridge) {
            firstCartridge = false;
            blankShot();
        } else {
            shot();
        }
    }

    protected void blankShot() {
        // the inheritors can override this method if they need
    }

    protected abstract void shot();

}

Now the method prepareUrl() looks like this:

public static String prepareUrl(String url, List<? extends NameValuePair> arguments) {
    final StringBuilder urlBuilder = new StringBuilder(url);
    FirstCartridgeIsBlank appendAmpersand = new FirstCartridgeIsBlank() {
        @Override
        protected void shot() {
            urlBuilder.append("&");
        }
    };
    for (NameValuePair argument : arguments) {
        appendAmpersand.run();
        urlBuilder.append(argument.getName());
        urlBuilder.append("=");
        urlBuilder.append(argument.getValue());
    }
    return urlBuilder.toString();
}

PS. I can't use the String.join() method from Java 8 because it's for Android app (i.e. Java 6).

share|improve this question
1  

3 Answers 3

Don't try to over-generalize early. This is a situation I've run into quite a few times for strings, but never anywhere else, so I would recommend you start with a string-specific solution, and only try to come up with something more general if it ever becomes necessary.

You mention String.join- is there any situation where you face this problem where String.join wouldn't solve your problem? It's quite possible that the problem you actually need to solve isn't "skipping some actions on the first loop iteration", it's "cleanly joining strings with a separator". If that's the case, then why not just reimplement String.join yourself, rather than creating your own very different way of achieving the same thing?

If you do need to generalize, then your solution could work, but its main problem is that now you're creating a class whose only purpose is to run a chunk of some arbitrary (possible private!) procedure, just because that chunk happened to follow a particular structure. If Java 6 supported functional programming, then a single class which was passed functions would have been acceptable, but the need to create a new class that inherits from FirstCartridgeIsBlank every time you want to use it seems like a design headache to me.

So I think instead of trying to create a class for this, it'd probably just be simpler to do it by extracting methods:

public static String prepareUrl(String url, List<? extends NameValuePair> arguments) {
    boolean firstIteration = true;
    StringBuilder urlBuilder = new StringBuilder(url);
    for (NameValuePair argument : arguments) {
        String argumentString = getQueryArgument(argument.getName(), argument.getValue(), firstIteration);
        firstIteration = false;
    }

    return urlBuilder.toString();
}

private static String getQueryArgument(String name, String value, Boolean includeSeparator) {
    //...
}
share|improve this answer
1  
Excellent answer, don't think I could have said it better myself. –  Simon André Forsberg 4 hours ago

So, you have probably heard that code duplication is bad, right? And that led you to want to abstractify this and make it a class?

I fear that this might be a case of German Overengineering (TM) (no offense if you are german).

You have received a good answer by @Ben but I'd like to extend a bit on that.


Composition over inheritance

Is it really necessary to extend a class to use these features? Wouldn't it just be easier to copy the code-pattern?

I don't think inheritance is necessary here, it would be better to supply two instances of Runnable here, one for the first iteration and one for the other iterations. This would also make it more easier to use for those who have Java 8 available.

Why does your current abstract class implement Runnable? Would you pass this to a thread? Would you pass it to any other method that requires a Runnable? I would not implement Runnable, and also consider naming your method something else, possibly iterate()

As shown by your prepareUrl implementation, it actually requires more code to use your class than to just copy the behavior that it does into the code.

About the naming of your method...

// I know that class name must be a noun.
// But I can't think of a good name.
// And it seems to me that a comparison with the blank cartridge is a good one.
public abstract class FirstCartridgeIsBlank implements Runnable {

A name that popped into my head was FirstIterationSpecializer or DoOnFirstIterationPerformer. Now especially that second name is really horrible, and would be better as doOnFirstIteration, which does sound more like a method than a class. So perhaps the reason why you haven't come up with a good name is that it's not really a good class?

share|improve this answer
    
Good answer, especially the part about how difficulty with naming indicates a problem with the design. –  Ben Aaronson 4 hours ago

No need to reinvent the wheel here. Just use the Uri.Builder class:

String url = "http://api.openweathermap.org/data/2.5/weather";
Uri.Builder builder = Uri.parse(url).buildUpon();

for(NameValuePair pair : pairs) {
    builder.appendQueryParameter(pair.getName(), pair.getValue());
}

return builder.toString();
share|improve this answer
1  
Thanks for help. But building url is only one of examples of such a loop iteration. I mean - "to skip the first loop iteration" in general –  Leonid Semyonov 5 hours ago
2  
I didn't even know that Uri.Builder existed and had a method for this, I just learned something new! –  Simon André Forsberg 5 hours ago

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.