Deciding whether an anchor should be updated

Question

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.

• https://stackoverflow.com/questions/729379/why-not-use-exceptions-as-regular-flow-of-control
• https://softwareengineering.stackexchange.com/questions/189222/are-exceptions-as-control-flow-considered-a-serious-antipattern-if-so-why

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

Answer 1

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);...