Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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?

share|improve this question
    
I don't like it. It's not checking for missing values. It's checkForMissedVariables! –  Loren Pechtel Feb 26 '13 at 4:27
    
The tag says C# but I see a Java-style for[each] being used as well as Java-style braces and Java-style method naming conventions. –  Job Feb 26 '13 at 19:49

7 Answers 7

up vote 2 down vote accepted

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.

share|improve this answer

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.

share|improve this answer
    
A small addition that might be helpful: pick a format now, document it, and stick with it –  Joel Etherton Feb 25 '13 at 13:24

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

share|improve this answer
    
As a side thought, I would expect a method called IsNotNull to return a boolean and never except. –  Joel Etherton Feb 25 '13 at 13:23
1  
I would as well; however, this method is static and will be written "Guard.IsNotNull" and I would expect a method called "GuardIsNotNull" to throw an exception. –  Mike Feb 25 '13 at 13:27
    
You still have a method that is a boolean condition. It either is or it isn't. Throwing an exception when the true return is false doesn't really make sense to me. –  Joel Etherton Feb 25 '13 at 13:31
2  
I guess we'll have to agree to disagree. I'd love to see your answer to the question with the conventions you find to work the best. –  Mike Feb 25 '13 at 13:35
1  
I use Argument.ThrowIfNull(...) which is a bit more clear. –  Scott Whitlock Feb 26 '13 at 16:30

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

share|improve this answer

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.

share|improve this answer

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

share|improve this answer
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.
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.