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).
-
2\$\begingroup\$ Related: Removing last comma problem \$\endgroup\$Simon Forsberg– Simon Forsberg2014年09月27日 18:20:25 +00:00Commented Sep 27, 2014 at 18:20
4 Answers 4
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) {
//...
}
-
1\$\begingroup\$ Excellent answer, don't think I could have said it better myself. \$\endgroup\$Simon Forsberg– Simon Forsberg2014年09月27日 18:38:59 +00:00Commented Sep 27, 2014 at 18:38
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?
-
2\$\begingroup\$ Good answer, especially the part about how difficulty with naming indicates a problem with the design. \$\endgroup\$Ben Aaronson– Ben Aaronson2014年09月27日 19:06:23 +00:00Commented Sep 27, 2014 at 19:06
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();
-
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– Leonid Semyonov2014年09月27日 17:31:43 +00:00Commented Sep 27, 2014 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– Simon Forsberg2014年09月27日 18:18:55 +00:00Commented Sep 27, 2014 at 18:18
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();
}
-
\$\begingroup\$ The
blankShot
/shoot
methods shouldn't be part of theThing
, it should rather beblankShot(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– Simon Forsberg2014年09月28日 15:00:43 +00:00Commented Sep 28, 2014 at 15:00