Validating attributes of a page in Adobe Experience Manager

I am reviewing the following code written by a fellow developer. I am not an java expert but IMHO i did not feel this is efficient use of exceptions — the reason I feel so is:

• The below code throwing exception — seems like flow control. Since we are just using it add a attribute error attribute to PageErrorRequestAttrUtil class — would this be better done using a method instead of throwing a exception for readability?

• The below code is not really defining a API that it is actually expected to throw exception informing the caller something went wrong.

I wanted a second opinion to make sure I am interpreting this incorrectly.

The first class below is invoked in context of AEM/Sightly and returns a java backed object for front end code. In this example the class is checking if a AEM page has certain attributes set and second class is adaptable of request and is used to display helpful messages to the user/author of the page what is missing.

Class 1

``
public class HrefLangUtil extends WCMUsePojo {
private static final Logger log = LoggerFactory.getLogger(HrefLangUtil.class);

private String hrefLangMultiLine;
private List hrefLangMapList;
private String err;

final private String COMMA = ",";
final private String NEW_LINE = "\\n";
final private static String validUrlCharRegex = "[\\w\\Q!\"#$%&'()*+,-./:;?@[\\]^_{|}~\\E]+";
final private String ERROR_MESSAGE = "The following authored hreflang path and code value pair is invalid: ";
final private String HREF_LANG_MULTI_LINE = "hrefLangMultiLine";
final private String HREF_LANG_ERROR_MESSAGE = "hrefLangErrorMessage";
final private String HREF_LANG_PATH = "hrefLangPath";
final private String HREF_LANG_CODE = "hrefLangCode";

@Override
@SuppressWarnings("unchecked")
public void activate() throws Exception {
ValueMap pageProperties = getPageProperties();
hrefLangMultiLine = pageProperties.get(HREF_LANG_MULTI_LINE, StringUtils.EMPTY);

A few things out of the way first:

• Yes the exception handling here is rather "fishy"

• The API does not throw an Exception

Next let's talk about the nitpicks:

• "for (String hrefLang : hrefLangLine ){" is usually written as for (String hrefLang : hrefLangLine) {

• "}catch (Exception e) {" is usually } catch (Exception e) {

• I usually have final after private because visibility is usually more important. Interestingly the Logger instance is declared as private static final all the other constants are not even declared static

Now that we have all that stuff out of the way that really should be done by some automatic formatter, we can get to the real meat of the code.

First let's talk about @SuppressWarnings. It is generally considered best practice to keep scopes as small as possible. That goes for try-blocks, variable scopes and (who'd think) warning-suppression.

I'm not pasting the code into my IDE, but IIUC the @SuppressWarnings is only needed for pageProperties#get in activate and for the assignment of pageLevelErrorMessages from requestAttr#get in putPageErrorAttr. The latter of those may even be unnecessary because you're casting to a rawtype. It should be fixable by instead casting to (Map) to avoid the rawtype.

Now let's take a stab at those exceptions.

As already noted at the start: You're completely correct in thinking those problematic. Let's ignore the fact that they're used for control flow for a second and talk about throw new Exception([..]);.

This is really really bad. For starters, it's generally accepted best practive in java to where possible catch the most specific exception. So hypothetically an IllegalArgumentException should be caught where expected instead of just "Pokémon-catching" Exception.

By extension this means you should always throw the most specific exception. If I understand the code correctly this means throwing an IllegalArgumentException would be the more semantically correct approach here.

Instead of throwing exceptions, "standard" flow-control is more than enough to solve the problems here, though:

try{
            for (String hrefLang : hrefLangLine ){
                if(!hrefLang.contains(COMMA) || hrefLang.split(COMMA).length > 2 || !hrefLang.matches(validUrlCharRegex)){
                    throw new Exception(ERROR_MESSAGE + hrefLang);
                }
                Map hrefLangMap = new HashMap<>();
                hrefLangMap.put(HREF_LANG_PATH, hrefLang.split(COMMA)[0]);
                hrefLangMap.put(HREF_LANG_CODE, hrefLang.split(COMMA)[1]);
                hrefLangMapList.add(hrefLangMap);
            }
        }catch (Exception e){
            err = e.getMessage();
            log.error(err);
            SlingHttpServletRequest request = getRequest();
            RequestAttr requestAttr = new RequestAttr(request);
            PageErrorRequestAttrUtil.putPageErrorAttr(HREF_LANG_ERROR_MESSAGE, err, requestAttr);
        }

This becomes:

for (String hrefLang : hrefLangLine) {
    String[] parts = hrefLang.split(COMMA);
    if (!hrefLang.contains(COMMA) || parts.length != 2 || !hrefLang.matches(validUrlCharRegex)) {
        err = ERROR_MESSAGE + hrefLang;
        log.error(err);
        // putting error on the page
        break;
    }
    // map stuff
}

I've been so keen as to extract the hrefLang.split(COMMA) into a variable to reduce unneeded calculations (and make the code a bit easier to read). In addition this doesn't use Exceptions, which makes the try-catch unnecessary and reduces the level of indentation by one.

I could also see the if-condition extracted into a separate method, but that might be overkill.

The other exception site can be dealt with exactly the same way. Just move the contents of the catch block to where the exception was thrown and remove the try-catch block.

On a closing note: The javadoc of putPageErrorAttr is apparently out of date. Returning an Object but having @return String doesn't seem correct.

StackExchange Code Review Q#142522, answer score: 2