4
\$\begingroup\$

Often I have to skip some actions on the first (or last) loop iteration. Now I need to append some arguments to a 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, the 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 code. And it's easy to make a mistake in a loop body like this.

I wrote 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 my code severely.

// 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();
}

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

\$\endgroup\$
8
\$\begingroup\$

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) {
    //...
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Excellent answer, don't think I could have said it better myself. \$\endgroup\$ – Simon Forsberg Sep 27 '14 at 18:38
6
\$\begingroup\$

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?

\$\endgroup\$
  • 2
    \$\begingroup\$ Good answer, especially the part about how difficulty with naming indicates a problem with the design. \$\endgroup\$ – Ben Aaronson Sep 27 '14 at 19:06
4
\$\begingroup\$

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();
\$\endgroup\$
  • 1
    \$\begingroup\$ 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 \$\endgroup\$ – Leonid Semyonov Sep 27 '14 at 17:31
  • 2
    \$\begingroup\$ I didn't even know that Uri.Builder existed and had a method for this, I just learned something new! \$\endgroup\$ – Simon Forsberg Sep 27 '14 at 18:18
0
\$\begingroup\$

Disclaimer: not a Java expert here.

Could you do it with an Iterator? Something like this:

ListIterator<Thing> it = list.listIterator();

// do something with first item
Thing t = it.next();

t.blankShot();

while ( it.hasNext() ) {
    t = it.next();
    // do something else
    t.shoot();
}
\$\endgroup\$
  • \$\begingroup\$ The blankShot/shoot methods shouldn't be part of the Thing, it should rather be blankShot(t). Other than that, your answer is quite similar to other previous answers. And also, if the list contains no elements, your current code will throw an exception. \$\endgroup\$ – Simon Forsberg Sep 28 '14 at 15:00

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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