4
\$\begingroup\$

Prospero is a URI scheme. There you have fields and values.

Could you check this code and give common suggestions or even test cases for JUnit tests?

(The documentation is in German.)

/**
 * Gibt einen Prospero-Parameter aus der URL zurück. Dabei wird nur der Prospero-Part geprüft.
 * 
 * @param url Die URL, darf nicht <code>null</code> sein.
 * @param name Der Name des Parameters, darf kein '=' enthalten, darf nicht <code>null</code>
 *            sein.
 * @return Den Wert des Parameters oder
 *         <dl>
 *         <dt><code>null</code></dt>
 *         <dd>wenn der Parameter ohne <code>=</code> angegeben wurde.</dd>
 *         <dt><code>error</code>-Parameter</dt>
 *         <dd>wenn kein solcher Parameter gefunden wurde</dd>
 *         </dl>
 */
public static String getProsperoParam(URL url, String name, String error) {
    String path = url.getPath();
    String[] split = path.split(";", 2);
    if (split.length == 2) {
        String params = split[1];
        for (String param : params.split("&")) {
            String[] paramParts = param.split("=", -1);
            if (paramParts.length == 1) {
                if (name.equals(paramParts[0])) {
                    return null;
                }
            }
            if (paramParts.length == 2) {
                if (name.equals(paramParts[0]))
                    return paramParts[1];
            }
            if (paramParts.length > 2) {
                if (name.equals(paramParts[0])) {
                    return param.substring(name.length() + 1);
                }
            }
        }
    }
    return error;
}

I successfully tested these cases:

assert URLTools.getProsperoParam(new URL("http://a:a@a.a.a/a.a?a=a#a"), "a", null) == null;
assert URLTools.getProsperoParam(new URL("http://a:a@a.a.a/a.a;a=4?a=a#a"), "a", null).equals("4");
assert URLTools.getProsperoParam(new URL("http://a:a@a.a.a/a.a;a=4=a?a=a#a"), "a", null).equals("4=a");
assert URLTools.getProsperoParam(new URL("http://a:a@a.a.a/a.a;a=4=a&m=3?a=a#a"), "m", null).equals("3");
String missingString = "Missing!";
assert URLTools.getProsperoParam(new URL("http://a:a@a.a.a/a.a;a=4=a&m=3?a=a#a"), "e", missingString) == missingString;
assert URLTools.getProsperoParam(new URL("http://a:a@a.a.a/a.a;e?a=a#a"), "e", "asdf") == null;
assert URLTools.getProsperoParam(new URL("http://a:a@a.a.a/a.a;e=?a=a#a"), "e", null).equals("");
\$\endgroup\$
  • \$\begingroup\$ you might want to mention what a Prospero is (and what Prospero-Parameter and Prospero-Part specifically are). I tried google, but the third result is already this question. It seems to be just a normal URL parameter? \$\endgroup\$ – tim Oct 15 '14 at 11:16
  • \$\begingroup\$ Do you actually need to distinguish the edge cases (parameter without value, empty value, value with = included) and can't just throw them out as invalid? \$\endgroup\$ – ferada Oct 15 '14 at 13:44
  • \$\begingroup\$ @ferada yes i need them. A empty value != missing value != missing key != not set. \$\endgroup\$ – Peter Rader Oct 15 '14 at 13:50
6
\$\begingroup\$

Check input

Your comments mention that url shouldn't be null, so it's best to make sure that it really isn't:

    if (url == null) {
        throw new IllegalArgumentException("url cannot be null");
    }

Nested if statements

        if (paramParts.length == 1) {
            if (name.equals(paramParts[0])) {
                return null;
            }
        }

You have this principle three times. It can be written as:

        if (paramParts.length == 1 && name.equals(paramParts[0])) {
            return null;
        }

Although I would rewritten your if statements like this:

        if (paramParts.length > 0 &&  name.equals(paramParts[0])) {
            switch(paramParts.length) {
                case 1:
                    return null;
                case 2:
                    return paramParts[1];
                default:
                    return param.substring(name.length() + 1);
            }
        }

And are you sure that paramParts.length == 2 (now case 2) is really needed? I think the other cases cover it.

Use early return

If you return early, you can reduce nesting:

if (split.length != 2) {
    return error;
}

Tests

The specification for prosperos says:

each field/value pair is separated from each other and from the rest of the URL by a ";" (semicolon).

You are never testing if this would work, and it wouldn't:

String prosperoParam = prospero.getProsperoParam(new URL("http://a:a@a.a.a/a.a;a=a;b=b"), "b", null);
assertEquals(prosperoParam, "b");

If you want to change the spec, I would comment on it in the Java doc (something like: See RFC 4157, but note that this implementation uses '&' instead of ';' for the separation of field/value pairs (it still does use ';' for the separation between the first field/value pair and the rest of the URL).

Also, sometimes you use == to compare strings in your tests, I would replace it with equals.

\$\endgroup\$
  • \$\begingroup\$ I realy need == on Strings(!), its not allowed to replace it with equals because i would not be able to check if the value is a real parameter-value or my error-value. \$\endgroup\$ – Peter Rader Oct 15 '14 at 12:28
  • 2
    \$\begingroup\$ @PeterRader, for IAE vs NPE, one school of thought is to consult what the Javadocs have to say for both exceptions. IAE is "thrown to indicate that a method has been passed an illegal or inappropriate argument", while NPE is "thrown when an application attempts to use null in a case where an object is required". It certainly seems that if null is an inappropriate method argument for your method, throwing IAE would be clearer. For more info: stackoverflow.com/questions/3881/… \$\endgroup\$ – h.j.k. Oct 15 '14 at 15:50
  • 1
    \$\begingroup\$ This is not an so important discussion, i can do it or i can let it be. If you guys gets upset, i'll accept it. Hey, if i like to become a ranter i should use Both: IAE(NPE). Even the implementation of .split( dont care about the more specific-qualified IAE(NPE) but i have to, am i something better?! Well i feel dirty if i throw a more-specific-qualified-exception without write it into the JavaDoc. But if i write it into the JavaDoc, it must match the Method's signature. And then Checkstyle will critizise me to have unchecked exceptions in the Method's signature. Life isnt Ponyhof. ;) \$\endgroup\$ – Peter Rader Oct 15 '14 at 16:07
  • 1
    \$\begingroup\$ @PeterRader you are right, even oracle handles this quite inconsistently. Matcher:usePattern does check, and throws an IAE, Scanner:hasNext does check, but throws a NPE. I didn't find any cases for public methods that don't check at all, but they may exist (I would prefer checking, some methods might produce side-effects otherwise). It seems that NPE is actually more common; there even exists a method for it: Objects:requireNonNull: This method is designed [for] parameter validation in methods \$\endgroup\$ – tim Oct 15 '14 at 16:52
  • 1
    \$\begingroup\$ @PeterRader sorry if my comment sounded like IAE must be used over NPE, there's really no right or wrong answer for this holy war within Java. :) Take whichever makes your method signature more consistent, and it seems like you have already justified your usage. All is good... \$\endgroup\$ – h.j.k. Oct 16 '14 at 6:05

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.