3
\$\begingroup\$

A colleague and I were discussing the following code today and had a disagreement over the return false in the catch (MalformedURLException e) block.

For some background information this method is in a class which is going through legacy data to be migrated into a new system, parsing the text, finding all of the <a> tags and re-writing some of those links depending on what the link is to. This method is determining if the link should be updated or not based on some criteria.

private boolean shouldUpdateAnchor(final String link, String replaceFrom, String replaceInto) {
    if (link == null) {
        return false;
    }

    if (!link.contains(URL_NODEREF_QUERY_PARAM)) {
        return false;
    }

    if (!link.contains(replaceFrom) && !link.contains(replaceInto)) {
        return false;
    }

    final URL url;
    try {
        url = new URL(link);
    } catch (MalformedURLException e) {
        return false;
    }

    final String query = url.getQuery();
    final NodeRef documentNodeRef = getNodeRefFromQueryString(query);
    final QName type = nodeService.getType(documentNodeRef);
    if (type == null) {
        return false;
    }

    if (!type.equals(ContentModel.KnowledgeCentreDocument.QNAME)) {
        return false;
    }

    return true;
}

One of us said that using exceptions for control flow is an anti-pattern and the exception should be thrown/allowed to propagate up and handled elsewhere, while the other said that the error being thrown is a by-product of the API (this is Java code which doesn't have any of the tryParse() methods of C#) which needn't be thrown any further as it can be handled here.

I've read these related questions but I'm not entirely sure they're applicable here as they're quite general.

To provide some context, here are the two methods which are called immediately before this one:

public final String transformLinks(final String contentString, String replaceFrom, String replaceInto) {
    if (StringUtils.isBlank(contentString))
        return contentString;

    final Document document = Jsoup.parse(contentString);
    final Elements anchors = document.getElementsByTag("a");
    for (final Element anchor : anchors) {
        handleAnchor(anchor, replaceFrom, replaceInto);
    }
    return document.body().toString();
}

private void handleAnchor(final Element anchor, String replaceFrom, String replaceInto) {
    final String linkHref = anchor.attr("href");
    if (!shouldUpdateAnchor(linkHref, replaceFrom, replaceInto)) {
        return;
    }

    final String newHref = linkHref.replace(replaceFrom, replaceInto);
    anchor.attr("href", newHref);
}
\$\endgroup\$
  • \$\begingroup\$ If the anchor should not be updated when the URL is malformed, this would be the correct way of handling it (in that case, throwing the exception would be using exceptions for control flow). If you want to do something else when the URL is malformed, this would obviously not be a good approach. \$\endgroup\$ – tim Mar 24 '15 at 19:22
  • \$\begingroup\$ That's exactly it - if the URL is malformed, don't do anything and don't notify anyone either - this is a best effort at migration. \$\endgroup\$ – Stuart Leyland-Cole Mar 24 '15 at 19:39
  • 4
    \$\begingroup\$ Could you also show us the method that actually updates the anchor if shouldUpdateAnchor() returns true? \$\endgroup\$ – 200_success Mar 24 '15 at 19:46
  • 1
    \$\begingroup\$ @200_success - I've updated my question with the preceding methods to add some context. \$\endgroup\$ – Stuart Leyland-Cole Mar 24 '15 at 21:34
1
\$\begingroup\$

About Exceptions

In general, exceptions should never be part of the anticipated program flow. Using exceptions to test conditions in the data, is wrong.

On the other hand, that is not what you are doing. You are taking an anchor/link from an HTML document, and I would expect that to always be a well-formed URL.

Additionally, a poorly-formed URL in the supplied context is not a program fault either, so propagating an exception is not useful. You will need to handle it somewhere else if you do.

My only suggestion is that you should be logging it somewhere... don't you need to know that you have HTML pages with broken links?

Concerns

You have some programming problems.... I am unable to tell if it is serious, it may be fine depending on your circumstances... consider:

if (!link.contains(replaceFrom) && !link.contains(replaceInto)) {
    return false;
}

... you return false if the link contains the replacement String, even if it also contains the to-replace string. Perhaps you should make the code just:

if (!link.contains(replaceFrom)) {
    return false;
}

As I say, there may be a business reason why you also need to check for the replaceInto, but, as it is at the moment, I believe you may be missing some replacements because the anchor contains both texts...

Style

You have the code:

final URL url;
try {
    url = new URL(link);
} catch (MalformedURLException e) {
    return false;
}

Code like this makes me itch ;-) , and not because of the return false, but because 'dangling declarations' of the url are a smell to me. Note that the type conditions can be simplified since they are boolean checks. Your try/catch code would be better written as:

try {
    final URL url = new URL(link);

    final String query = url.getQuery();
    final NodeRef documentNodeRef = getNodeRefFromQueryString(query);
    final QName type = nodeService.getType(documentNodeRef);

    return ContentModel.KnowledgeCentreDocument.QNAME.equals(type);

} catch (MalformedURLException e) {
    return false;
}

Note that the constant QNAME is not null, I assume, and that it's equals() method is presumably an equals-contract compatable implementation, which means it should return false for QNAME.equals(null);...

\$\endgroup\$
  • \$\begingroup\$ Interesting comments, thank you. Glad you share my thoughts about the use of exceptions. Maybe I failed to give my colleague enough context for him to come to the same conclusion. To address a couple of your points...The exception is actually logged, I accidently removed the logging line when I was removing some debug logging. Good catch about the contains() check, right now I can't think of a sensible reason for that...wood for the trees and all that. I'll speak to the original dev and double check his thinking. Also your refactored version is much nicer, thanks for taking the time to do it \$\endgroup\$ – Stuart Leyland-Cole Mar 25 '15 at 22:37

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.