Parser and solver for percentage calculation problems

Question

I'm working on a coding challenge to help with regular expressions, and how to write better comments in my code. The class parses a percentage question, and can return the answer.

If anyone has any feedback on the general organization of my class, variable names, or whether or not my comments can be improved, it would be appreciated. My goal is that anyone reading my code can clearly understand what it does and how it works, keeping the 'maintenance' programmer in mind.

package com.percent.model;

import java.util.Arrays;
import java.util.Collection;
import java.util.regex.Matcher;
import java.util.regex.Pattern;

/**
 * Class represents a solver for a percentage question.
 * @author steve
 */
public class PercentageQueryPatternSolver {

    //? is x% of y
    private static final Pattern findIsPattern = Pattern.compile("(\\?) is (\\d+)% of (\\d+)");
    //x is ?% of y
    private static final Pattern findPercentPattern = Pattern.compile("(\\d+) is (\\?)% of (\\d+)");
    //x is y% of ?
    private static final Pattern findOfPattern = Pattern.compile("(\\d+) is (\\d+)% of (\\?)");

    /*Although the numeric values in the queryString passed to the constructor must be integers, we
     *store them as Doubles since the value that we are solving for may not be a whole number, and
     *we don't know which of the 3 values we are solving for in the beginning. When the query
     *is solved, we will remove any trailing .00 values to clean up the output.
     */
    private Double firstIsValue;
    private Double secondPercentValue;
    private Double thirdOfValue;

    //These are the valid forms that we expect the query string to take
    private Collection<Pattern> validQueryStringPatterns = Arrays.asList(findIsPattern,
                                                                         findPercentPattern,
                                                                         findOfPattern);

    /**
     *
     * @param queryString query to be parsed. must take one of the following forms:
     * ? is x% of y    (example: ? is 10% of 32)
     * x is ?% of y    (example: 12 is ?% of 62)
     * x is y% of ?    (example: 25 is 40% of ?)
     *
     * 'x' and 'y' must be whole numbers, and both values are required
     * '?' represents a value to be solved for, and must be entered as '?'
     *
     *  If the queryString does not take one of the above forms, an IllegalArgumentException is thrown
     */
    public PercentageQueryPatternSolver(String queryString){
        initialize(queryString);
    }

    //parses the queryString and populates the firstIsValue/secondPercentValue/thirdOfValue variables
    private void initialize(String queryString){
        boolean queryStringMatchesAValidQueryPattern = false;
        for(Pattern pattern : validQueryStringPatterns){
            final Matcher patternMatcher = pattern.matcher(queryString);
            if(patternMatcher.matches()){
                queryStringMatchesAValidQueryPattern = true;
                setQueryParameters(patternMatcher);
            }
        }
        if(!queryStringMatchesAValidQueryPattern) {
            throw new IllegalArgumentException(queryString + "does not match a known pattern.");
        }
    }

    //pulls the query parameters from the matcher and sets the respective attributes
    private void setQueryParameters(Matcher validatedPatternMatcher){
        firstIsValue = getDoubleValueOfGroupFromMatcher(validatedPatternMatcher, 1);
        secondPercentValue = getDoubleValueOfGroupFromMatcher(validatedPatternMatcher, 2);
        thirdOfValue = getDoubleValueOfGroupFromMatcher(validatedPatternMatcher, 3);
    }

    //returns the double value of a particular group from the matcher
    private Double getDoubleValueOfGroupFromMatcher(Matcher matcher, int groupNumber){
        String groupValue = matcher.group(groupNumber);
        return "?".equals(groupValue) ? null : Double.valueOf(groupValue);
    }

    //returns the inputDouble formatted for output purposes (remove trailing decimal if 0)
    private String getOutputFormattedDoubleValue(Double inputDouble){
        String output = String.valueOf(inputDouble);
        if (inputDouble == Math.floor(inputDouble)){
            output = String.valueOf(inputDouble.intValue());
        }
        return output;
    }

    /**
     *
     * @return the original queryString (used to construct this pattern) with the
     * missing value populated.
     */
    public String getSolvedPattern(){
        if(firstIsValue == null){
            firstIsValue = (thirdOfValue * secondPercentValue) / 100;
        }
        else if(secondPercentValue == null){
            secondPercentValue = (firstIsValue * 100) / thirdOfValue;
        }
        else if(thirdOfValue == null){
            thirdOfValue = (firstIsValue * 100) / secondPercentValue;
        }
        return getOutputFormattedDoubleValue(firstIsValue) + " is " +
               getOutputFormattedDoubleValue(secondPercentValue) + "% of " +
               getOutputFormattedDoubleValue(thirdOfValue);

        }

}

Answer 1

    //? firstIsValue x% of y
    private static final Pattern findIsPattern = Pattern.compile("(\\?) is (\\d+)% of (\\d+)");
    //x firstIsValue ?% of y
    private static final Pattern findPercentPattern = Pattern.compile("(\\d+) is (\\?)% of (\\d+)");
    //x firstIsValue y% of ?
    private static final Pattern findOfPattern = Pattern.compile("(\\d+) is (\\d+)% of (\\?)");

The first rule of writing good comments is not to tell what the code is doing but why it is doing it. E.g.

    // Define separate patterns depending on whether we are searching for 
    // the subject, 
    // the subject's complement, or 
    // the object of the preposition.  
    private static final Pattern subjectFinder = Pattern.compile("(\\?) is (\\d+)% of (\\d+)");
    private static final Pattern complementFinder = Pattern.compile("(\\d+) is (\\?)% of (\\d+)");
    private static final Pattern objectFinder = Pattern.compile("(\\d+) is (\\d+)% of (\\?)");

Now we can easily see that the reason why we have three different patterns. It's because we might be looking for three different things.

I don't try to restate what the code is doing. This is because I had no idea what ? firstIsValue x% of y meant. I had to read the code to understand the comment, which is completely backwards. Assume that the reader knows enough to understand what the code does directly. Comment to tell them what the code does indirectly.

I changed the names to ones that I found clearer. If you like Hungarian Notation, which is common in Java, you can add Pattern to each name. I usually prefer to leave it off, with exceptions where I find it useful. If you read the linked article, I would prefer Apps Hungarian names to Systems Hungarian.

The new names describe the parts of the "sentence" that is being patterned. Sometimes, it is worth doing a little research to come up with better names. For example, I didn't remember that the noun after the "is" (or other linking verb) is called the subject's complement. I just remembered (vaguely) that it was called something different with a linking verb than with an active verb. Google assisted.

Good naming is often better commenting than comments.

Even better, consider unit tests. Names and comments can fall out of synch with the code. For example, your original name findOfPattern. If you change the "of" to "in", your code and name will be out of synch.

With unit tests, they will tell you if the code and the test are out of synch. Some of the tests will fail. You can then either make the code match the tests or in some cases, alter the tests to match the code.

Because of this characteristic of enforced synchronicity, unit tests often make for better comments than comments do.

To reiterate, when you do use comments, please use them to tell the reader why you are doing something. You should very rarely have code so obfuscated that you need to explain what you are doing. The code itself should tell us what the code is doing. The comment only needs to add why.