Naming guard clauses that throw exceptions

Question

I have a function evaluate() that parses a String for some variables and replaces them with their corresponding value:

public String evaluate() {
    String result = templateText;
    for (Entry<String, String> entry : variables.entrySet()) {
        String regex = "\\$\\{" + entry.getKey() + "\\}";
        result = result.replaceAll(regex, entry.getValue());
    }
    if (result.matches(".*\\$\\{.+\\}.*")) {    //<--check if a variable wasn't replaced
        throw new MissingValueException();
    }
    return result;
}

Since the evaluate() function now does 2 things, it first replaces the variables with the values AND check for any variables that weren't replaced, I thought about refactoring it to:

public String evaluate() {
    String result = templateText;
    for (Entry<String, String> entry : variables.entrySet()) {
        String regex = "\\$\\{" + entry.getKey() + "\\}";
        result = result.replaceAll(regex, entry.getValue());
    }
    checkForMissingValues(result);
}

private void checkForMissingValues(String result) {
    if (result.matches(".*\\$\\{.+\\}.*")) {
        throw new MissingValueException();
    }
}

Now is this a good name for the function checkForMissingValues? I mean it does check for missing values, but it also throws an exception.

I thought about renaming it to throwExceptionIfMissingValueWasFound(), but this name tells HOW the function does it more than WHAT the function is doing.

Is there a standard for naming such functions that check for a condition then throw an exception?

Answer 1

Why don't you call it guardAgainstMissingValues() it's simple and conveys the message directly.

Just ask yourself what does this function do? It guards against Missing values. You said it yourself that it's a guard clause, so why don't you name it accordingly.

In addition I think that the term guard clause implies a check in its implementation, so you won't have to explicitly mention the check in the function's name.

Answer 2

I usually use ensureThisOrThatAttribute, which, in combination with an explicit throws CustomMadeException declaration makes things clear enough.

However, more important than conforming to some alleged general standard is that you conform to your own code base's standard. If you don't have one yet, well, just pick a format now and stick with it.

Answer 3

I usually prefer a convention such as this:

static class Guard {
  public static void IsNotNull(value) {
    if(value == null) {
      throw new MyCustomGuardException();
    }
  }
}

Later on when using it:

public function DoSomething(DateTime startedAt) {
  Guard.IsNotNull(startedAt);
}

A more complete example of a fluent guard class can be found in the NetFX package here:

http://netfx.codeplex.com/SourceControl/changeset/view/89b72bee718c#Extensions/System/Guard/Source/content/netfx/System/Guard.cs

Answer 4

Your routine name seems perfectly fine to me. What's so bad about "check"?

I tend to break it up, though. So I have a routine that does the check/throw, plus functions that do the actual tests. You'd wind up with something like:

check(!hasMissingValues(result));

This makes it more than just a naming convention. You can call it "validate" or "enforce" or whatever, but the key it that check has a defined behavior.

(This of course assumes that the kinds of conditions you're checking don't need a bunch of custom exception info, but that is often the case.)

Answer 5

I don't think this warrants a separate method. I also noticed several things:

You create a Pattern object for each variable in your Map. Pattern object creation is expensive (requires parsing). You then evaluate the regex on the string with replaceAll. So if you have 100 variables that becomes 100 regexes parsed and created then string is scanned 100 times. That's inefficient.

Your regex to scan for missing variables is exceedingly expensive. It will scan each character in your string many many many times, especially if it contains ${.

What you need to do is flip the logic. Find strings ${x} in text and retrieve variables from your map to fill that, throwing an exception the first time you encounter a missing variable.

public String evaluate() {
    Matcher matcher = Pattern.compile("\\$\\{(\\.+?\\)}").matcher(templateText);
    StringBuilder result = new StringBuilder(templateText.length());
    while(matcher.find()) {
        String val = variables.get(matcher.group(1));
        if (val == null) throw new MissingValueException();
        matcher.appendReplacement(result, val);
    }
    matcher.appendTail(result);
    return result.toString();
}

This will search the string only once with only a single regex. It will fail on first missing variable. If you have 100 variables defined but only 1 appears in the template only 1 replace is done.

Answer 6

Well first of all, methods in C# follow the Pascal convention, so it should be Evaluate and not evaluate.

Second: It is natural for methods to throw exceptions in, well, exceptional scenarios so you DONT have to name your method to signify this.

Third: It is not normal to throw an exception due to an internal check (unless you have a specific need). Ideally the method should return a bool which states whether the check succeeded or failed. So the method signature should ideally be:

private bool IsMissingValues(String result) 

Next, a method name does not always have to explain every single detail of what it does, always read it in conjunction with the class in which it is defined.

Hence, String.Replace is sufficient; String.ReplaceThisStringWithAnother is overkill.

Speaking of which, your evaluate method should ideally be called Replace or something as that it what it essentially does.

Also private methods can be relatively less 'wordy' than public ones since no one else except you will see it (ok fine, that's debatable).

Answer 7

public String evaluate() {
    String result = replaceVariablesWithValues();
       // Extract the for loop code to the new method listed above

    if (resultContainsVariables(result)) {    
         // All variables should have been replaced
        throw new MissingValueException();
    }
    return result;
}

Above is my proposal for how to refactor.

1. The proposed evaluate function describes succinctly both the general functionality (replace variables with values) as well as the exception case (if the result contains variables, throw an exception).
2. The details of the regex evaluations should be extracted to helper functions because one should not need to mentally parse the regex to understand the contract and control flow of evaluate. Also, I suggest making the regex prefix and suffix constants.
3. I suggest keeping the throw statement in evaluate. It is not obvious what a "check" or guard function does, but knowing what such a function does is required to understand the contract of evaluate.