4
\$\begingroup\$

I wrote a class, which intends to work with regular expression (as a String or Pattern object) to get capturing groups of given regex, but it operate on regex. I wrote it to implement in my regex app and it is working quite well. I wanted to make it simple to implement, reusable, and as much as possible easy to read and understand, to make it like public API. I would be grateful if someone would review it from point of view of:

  1. readability - is it easy to grasp what given method is for?
  2. reusability - is it easy to use/implement for others? Are there some obviuos flaws I don't know, etc.,
  3. maintaining - any visible bad practice for writing 'public' classes?
  4. hypothetical modification - is there something which could make implementing changes in class in future difficult for hypothetical user?

I aware that this is small class (but still sorry for long code), so I will be grateful for any comments on above topics, as also for any comments about code. I added main() method, so it is easy to compile and test, also it shows basic methods:

import java.util.*;
import java.util.regex.Matcher;
import java.util.regex.Pattern;

/**
 * Tool for interpretation of the literal content of given regular expression. It performs analysis of capturing
 * abilities of this regex
 */
public class Metamatcher {
    private String pattern;
    private TreeMap<Integer,Integer> groupsIndices;
    private HashMap<String, Integer> namedGroupsIndex;

    public Metamatcher(){
        pattern = "";
        namedGroupsIndex = new HashMap<String, Integer>();
        groupsIndices = (TreeMap<Integer,Integer>)getGroups();
    }

    public Metamatcher(String pattern){
        this.pattern = pattern;
        namedGroupsIndex = new HashMap<String, Integer>();
        groupsIndices = (TreeMap<Integer,Integer>)getGroups();
    }

    public Metamatcher(Pattern pattern){
        this.pattern = pattern.toString();
        namedGroupsIndex = new HashMap<String, Integer>();
        groupsIndices = (TreeMap<Integer,Integer>)getGroups();
    }

    /**
     * @param group ordinal number of group
     * @return starting index of a fragment of pattern, which contain group capturing
     */
    public int start(int group){
        List<Integer> indices = new ArrayList<Integer>(groupsIndices.keySet());
        indices.add(0,0);
        return indices.get(group);
    }

    /**
     * @param name name of group
     * @return starting index of a fragment of pattern, which contain named group capturing, or -1 if there is
     * no such named group
     */
    public int start(String name){
        return namedGroupsIndex.containsKey(name) ? start(namedGroupsIndex.get(name)) : -1;
    }

    /**
     * @param group ordinal number of group
     * @return ending index of a fragment of pattern, which contain group capturing
     */
    public int end(int group){
        List<Integer> indices = new ArrayList<Integer>(groupsIndices.values());
        indices.add(0,pattern.length());
        return indices.get(group);
    }

    /**
     * @param name name of group
     * @return endig index of a fragment of pattern, which contain named group capturing, or -1 if there is no such
     * named group
     */
    public int end(String name){
        return namedGroupsIndex.containsKey(name) ? end(namedGroupsIndex.get(name)) : -1;
    }

    /**
     * @param group ordinal number of group
     * @return String object containing fragment of regular expression which capture given group
     */
    public String group(int group){
        return pattern.substring(start(group), end(group));
    }

    public String group(String name){
        return (namedGroupsIndex.containsKey(name) ? group(namedGroupsIndex.get(name)) : null);
    }

    /**
     * @return number of capturing groups within given regular expression
     */
    public int groupCount(){
        return groupsIndices.size();
    }

    /**
     * @return information about Metamatcher object: group count and groups,
     */
    public String toString(){
        StringBuilder result = new StringBuilder();
        result.append("Groups count: ")
                .append(groupCount())
                .append("\n");
        for(int i = 0; i <= groupCount(); i++){
            result.append("group(")
                    .append(i).append(") ")
                    .append(start(i))
                    .append("-")
                    .append(end(i))
                    .append("\t")
                    .append(group(i))
                    .append("\n");
        }
        return result.toString();
    }

    /**
     * Changes the regular expression that this Metamatcher uses to a given String object
     * @param pattern String consisting regular expression
     * @return same object Metamatcher
     */
    public Metamatcher usePattern(String pattern){
        this.pattern = pattern;
        groupsIndices = (TreeMap<Integer,Integer>)getGroups();
        return this;
    }

    /**
     * Changes the regular expression that this Metamatcher uses to a regular expression used by given Pattern object
     * @param pattern Pattern class object
     */
    public void usePattern(Pattern pattern){
        this.pattern = pattern.toString();
        groupsIndices = (TreeMap<Integer,Integer>)getGroups();
    }

    /**
     * @return the regular expression from which this Metamatcher use.
     */
    public String getPattern(){
        return pattern;
    }

    /**Returns start(key) and end(value) indices within Map object
     * @return Map beginning and ending indices of captured groups of given regular expression
     */
    Map<Integer,Integer> getGroups(){
        String copy = pattern;
        Pattern pattern = Pattern.compile("\\([^\\(\\)]+\\)");
        Matcher matcher = pattern.matcher(copy);
        Map<Integer,Integer> temp = new TreeMap<Integer,Integer>();

        while(matcher.find()){
            if(isCapturingGroup(matcher.group(0))){
                temp.put(matcher.start(), matcher.end());
            }
            if(isNamedGroup(matcher.group(0))){
                namedGroupsIndex.put(getNamedGroup(matcher.group(0)),matcher.start());
            }
            copy = copy.substring(0,matcher.start()) + replaceWithSpaces(matcher.group(0)) + copy.substring(matcher.end());
            matcher.reset(copy);
        }

        return temp;
    }

    /**
     * @param fragment of regular expression, enclosed by brackets
     * @return true if given String consist regular expression which capture groups
     */
    boolean isCapturingGroup(String fragment){
        return fragment.matches("((?<!\\\\)\\((?!\\?[<]?[:=!>])[^\\(\\)]+\\))");
    }

    /**
     * @param fragment of regular expression, enclosed by brackets
     * @return true if given String consist regular expression with capturing named groups
     */
    boolean isNamedGroup(String fragment){
        return fragment.matches("\\(\\?<[A-Za-z0-9]+>[^)]+\\)");
    }

    /**
     * Extracts a name of named capturing group
     * @param fragment of regular expression
     * @return name of capturing group from within of given fragment
     */
    String getNamedGroup(String fragment){
        Matcher matcher = Pattern.compile("(?<=<)[a-zA-Z0-9]+?(?=>)").matcher(fragment);
        matcher.find();
        return matcher.group(0);
    }

    /**
     * Provide a filler String composed of spaces, to replace part enclosed by brackets
     * @param part String containing capturing group of regex, starting and ending with brackets,
     * @return String composed of spaces (' '), with length of part object,
     */
    String replaceWithSpaces(String part){
        String filler = "";
        for(int i = 0; i < part.length(); i++){
            filler += " ";
        }
        return filler;
    }

    public static void main(String[] args){
        Metamatcher matcher = new Metamatcher();
        String[] patterns = {"(a(b(c))d)(e(fg(h)ij))",
                "^([_A-Za-z0-9-]+)(\\.[_A-Za-z0-9-]+)*@[A-Za-z0-9]+\n(\\.[A-Za-z0-9]+)*(\\.[A-Za-z]{2,})$",
                "\\((\\d+\\.\\d+)\\s(\\d+\\.\\d+)",
                "(?<name>[a-z])+"};
        for(String pattern : patterns) {
            matcher.usePattern(pattern);
            System.out.println(matcher.toString());
            System.out.println("matcher.groupCount() : " + matcher.groupCount());
            System.out.println();
        }
        System.out.println("matcher.group() : " + matcher.group("name"));
    }
}

As I said above, class is for analyzing capturing groups within given regex. However for a short introduction, it has methods like:

  • start(int group) - returns beginning index of group fragment within regex,
  • end(int group) - returns ending index of group fragment within regex,
  • group(int group) - return String with fragment resposible for matching given group()
  • usePatter(String patter) - for changing analyzed regex,
  • patter - return String with used regex,

etc.

\$\endgroup\$
4
\$\begingroup\$

Before getting to your specific questions, let me just get a few nit-picky things out the way.

At the very beginning, in the imports, you have this:

import java.util.*;
import java.util.regex.Matcher;
import java.util.regex.Pattern;

but why not just have:

import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.TreeMap;
import java.util.regex.Matcher;
import java.util.regex.Pattern;

Those are all of the dependencies in your program, and it's best to just write them out so everyone knows where each package is specifically. Many IDEs do this automatically now-a-days, so it's not hard to do.

Your class's name is Metamatcher. Again, this is kind of picky, but the naming of class would look a bit better to developers if it was MetaMatcher. The name itself doesn't really describe what's happening in the class either. This area is up to you, but perhaps something like RegCount or RegexUtil would be better, based on how much you plan to expand its functionality.

When writing the code out, please avoid the habit of something like this:

public void HelloWorld(){ ...

Just adding that extra space helps the visual appeal, and is kind of standard among JVM (and OOP) developers:

public void HelloWorld() { ...

Spacing can be good for some standard function calls in general as well, like with the for and while loops. I noticed that you didn't have this in a couple places.

When documenting your code, the standard for JavaDocs looks something like this:

/** Description of myMethod(int a, String b)
 * 
 * @param a         Description of a
 * @param b         Description of b
 * @return          Description of c
 */

When done this way, the description will actually appear alongside the function's parameters, which is a big help in IDEs like Eclipse.

Now, to actually attend to your questions:

Readability For this one, most of my nit-picking should help clean up the code. As for what your code actually does, your documentation lists none of that. Yes, we get what the class is supposed to be used for and what the functions do, but apart from that we don't know what the code does solely from your documentation. If I were a newb, I wouldn't touch this. Instead, I'd most likely just scrounge through the Apache libraries to get what I need.

Reusability Again, this is kind of answered by my nit-picking and previous answer about readability. At the end of the day, if developer's can't tell the purpose of the program through anything other than spending time to chug down the raw code, they probably won't want to take the time to bother. This is why documentation has been under a heavy magnifying glass recently, because even though we all hate it, it's kind of a necessary evil.

Maintaining I'm not quite sure what you mean specifically, but just taking what your question after this title is, that I can answer. You seem to be just fine in terms of design, just always remember your nomenclature though. Also, instead of doing this excessively in your constructors:

namedGroupsIndex = new HashMap<String, Integer>();
groupsIndices = (TreeMap<Integer,Integer>)getGroups();

You can just put them directly at the variables, since you're only making a new instance of them:

private TreeMap<Integer, Integer> groupsIndices = (TreeMap<Integer, Integer>) getGroups();
private HashMap<String, Integer> namedGroupsIndex = new HashMap<String, Integer>();

Note the spacing :)

Go look at @Emz answer here, I'd highly recommend that.

Hypothetical Modification When you say "hypothetical," you open the door to an endless world of opportunity. It all depends on what the program is designed to do. Only focus on what the program does, and how to do it better. If you have troubles on that specifically, then CR and SO will be of great help!

I went ahead and fixed your replaceWithSpaces function, as you can use String.format to do what you want:

/**
 * Provide a filler String composed of spaces, to replace part enclosed by
 * brackets
 * 
 * @param part
 *            String containing capturing group of regex, starting and
 *            ending with brackets,
 * @return String composed of spaces (' '), with length of part object,
 */
String replaceWithSpaces(String part) {
    return String.format("%1$" + part.length() + "s", part);
}

This pads the string up to part.length to the right. Since your string is empty, it doesn't matter which direction the pad is going.

Hope this helps!

\$\endgroup\$
  • 1
    \$\begingroup\$ Concerning the first remark, please note that adding import java.util.* will not automatically import java.util.regex.*, because cross-packages imports are not possible. In general, one should be cautious when using bulk import with '*'. \$\endgroup\$ – Antot Sep 25 '15 at 5:57
  • \$\begingroup\$ @Antot: Thanks! I was tapering off of a Diet Pepsi and code binge, so there should be a few improvements now! \$\endgroup\$ – T145 Sep 25 '15 at 16:06
3
\$\begingroup\$

Very small addition:

As you never re-declare namedGroupsIndex, your HashMap member.

private HashMap<String, Integer> namedGroupsIndex;

You can use the final keyword, mostly to emphazise that it never gets re-declared. You can also initialize it in the class itself. Removing those lines from the constructors.

private final Map<String, Integer> namedGroupsIndex = new HashMap<String, Integer>();
\$\endgroup\$
3
+50
\$\begingroup\$

There are several things I would suggest addressing in your class.

API

  1. The class should be final, because you haven't designed for extension.
  2. The default-scope methods should be private.
  3. The documentation needs some attention. Some public methods don't discuss what input values are invalid, or what the results of those inputs would be. It could also use some fleshing out, and some examples of use at the class level would be nice.
  4. From a type safety standpoint, it is much better to consider the Pattern as the type that the class works with, rather than a String. It's probably OK to accept a String into the constructor, but the class should really be designed to work with Patterns.
  5. I would highly recommend not making instances reusable. It adds unnecessary complexity and makes it easier for bugs to sneak in. There's virtually no benefit to allowing changes to the pattern - just make a new Metamatcher instance.
  6. start(int) and start(string) return inconsistently if the input is invalid - either ArrayIndexOutOfBoundsException or -1. They should do the same thing when bad input comes in.

Implementation

  1. You have a bug - namedGroupIndex does not get clear()ed when you change the Metamatcher's pattern. See 4, above.
  2. It's decidedly suboptimal to have getGroups() return the value of one map and compute the other as a side effect. It's counterintuitive and violates the Principle of Least Surprise.
  3. Code to interfaces (Map, SortedMap), not implementations (HashMap, TreeMap).
  4. If you're serious about testing, write a JUnit suite. Test the class, not each method, as the unit. Your main method isn't currently comprehensive and can't be made such cleanly.
  5. For performance reasons, patterns should be compiled where possible.
  6. Prefer using StringBuilder to String concatenation (c.f. replaceWithSpaces). The use of String.format() in this particular case is even better.
  7. toString()
    1. If toString() becomes a performance problem, and instances are not reusable, you can cache toString().
    2. Newlines in toString() might make some log file readers unhappy. It's only really an issue of you have external clients.
    3. It might be cleaner if the groups are tabbed in - it's easier to see that all those lines belong to the definition of the object.
  8. It's inefficient to use Map#get() right after Map#containsKey(), because you're doing 2 hash lookups. Just call get() and check for null.
  9. Avoid * imports. Explicitly identify the types you want to use.

Consider the alternate implementation below for some ideas. Note that it does not address the inconsistent responses to invalid inputs.

import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.SortedMap;
import java.util.TreeMap;
import java.util.regex.Matcher;
import java.util.regex.Pattern;

/**
 * Tool for interpretation of the literal content of given regular expression. It performs analysis of capturing
 * abilities of this regex
 */
public final class Metamatcher {

    private static final Pattern IS_GROUP = Pattern.compile("\\([^\\(\\)]+\\)");
    private static final Pattern IS_CAPTURING_GROUP = Pattern.compile("((?<!\\\\)\\((?!\\?[<]?[:=!>])[^\\(\\)]+\\))");
    private static final Pattern IS_NAMED_GROUP = Pattern.compile("\\(\\?<[A-Za-z0-9]+>[^)]+\\)");
    private static final Pattern MATCH_NAMED_GROUP = Pattern.compile("(?<=<)[a-zA-Z0-9]+?(?=>)");

    private final Pattern pattern;
    private final SortedMap<Integer, Integer> groupIndices = new TreeMap<Integer, Integer>();
    private final Map<String, Integer> namedGroupsIndex = new HashMap<String, Integer>();

    public Metamatcher(final Pattern pattern) {
        this.pattern = pattern;
        Objects.requireNonNull(this.pattern, "The pattern must be non-null");

        this.buildMetadata();
    }

    public Metamatcher(final String pattern) {
        this(Pattern.compile(pattern));
    }

    /**
     * @param group ordinal number of group
     * @return starting index of a fragment of pattern, which contain group capturing
     */
    public int start(final int group) {
        final List<Integer> indices = new ArrayList<Integer>(this.groupIndices.keySet());
        indices.add(0, 0);
        return indices.get(group);
    }

    /**
     * @param name name of group
     * @return starting index of a fragment of pattern, which contain named group capturing, or -1 if there is
     * no such named group
     */
    public int start(final String name) {
        final Integer index = this.namedGroupsIndex.get(name);
        return (index == null) ? -1 : this.start(index.intValue());
    }

    /**
     * @param group ordinal number of group
     * @return ending index of a fragment of pattern, which contain group capturing
     */
    public int end(final int group) {
        final List<Integer> indices = new ArrayList<Integer>(this.groupIndices.values());
        indices.add(0, this.pattern.toString().length());
        return indices.get(group);
    }

    /**
     * @param name name of group
     * @return endig index of a fragment of pattern, which contain named group capturing, or -1 if there is no such
     * named group
     */
    public int end(final String name) {
        final Integer index = this.namedGroupsIndex.get(name);
        return (index == null) ? -1 : this.end(index.intValue());
    }

    /**
     * @param group ordinal number of group
     * @return String object containing fragment of regular expression which capture given group
     */
    public String group(final int group) {
        return this.pattern.toString().substring(this.start(group), this.end(group));
    }

    public String group(final String name) {
        final Integer index = this.namedGroupsIndex.get(name);
        return (index == null) ? null : this.group(index.intValue());
    }

    /**
     * @return number of capturing groups within given regular expression
     */
    public int groupCount() {
        return this.groupIndices.size();
    }

    /**
     * @return information about Metamatcher object: group count and groups,
     */
    @Override
    public String toString() {
        final StringBuilder result = new StringBuilder();
        result.append("Groups count: ")
                .append(this.groupCount())
                .append("\n");
        for (int i = 0; i <= this.groupCount(); i++) {
            result.append("group(")
                    .append(i)
                    .append(") ")
                    .append(this.start(i))
                    .append("-")
                    .append(this.end(i))
                    .append("\t")
                    .append(this.group(i))
                    .append("\n");
        }
        return result.toString();
    }

    /**
     * @return the regular expression from which this Metamatcher use.
     */
    public Pattern getPattern() {
        return this.pattern;
    }

    /**
     * Returns start(key) and end(value) indices within Map object
     * @return Map beginning and ending indices of captured groups of given regular expression
     */
    private void buildMetadata() {
        String input = this.pattern.toString();
        final Matcher matcher = IS_GROUP.matcher(input);
        while (matcher.find()) {
            final String firstGroup = matcher.group(0);
            if (this.isCapturingGroup(firstGroup)) {
                this.groupIndices.put(matcher.start(), matcher.end());
            }
            if (this.isNamedGroup(firstGroup)) {
                this.namedGroupsIndex.put(this.getNamedGroup(firstGroup), matcher.start());
            }

            input = input.substring(0, matcher.start())
                    + this.replaceWithSpaces(firstGroup)
                    + input.substring(matcher.end());
            matcher.reset(input);
        }
    }

    /**
     * @param fragment of regular expression, enclosed by brackets
     * @return true if given String consist regular expression which capture groups
     */
    private boolean isCapturingGroup(final String fragment) {
        return IS_CAPTURING_GROUP.matcher(fragment).matches();
    }

    /**
     * @param fragment of regular expression, enclosed by brackets
     * @return true if given String consist regular expression with capturing named groups
     */
    private boolean isNamedGroup(final String fragment) {
        return IS_NAMED_GROUP.matcher(fragment).matches();
    }

    /**
     * Extracts a name of named capturing group
     * @param fragment of regular expression
     * @return name of capturing group from within of given fragment
     */
    private final String getNamedGroup(final String fragment) {
        final Matcher matcher = MATCH_NAMED_GROUP.matcher(fragment);
        matcher.find();
        return matcher.group(0);
    }

    /**
     * Provide a filler String composed of spaces, to replace part enclosed by brackets
     * @param part String containing capturing group of regex, starting and ending with brackets,
     * @return String composed of spaces (' '), with length of part object,
     */
    private String replaceWithSpaces(final String part) {
        return String.format("%" + part.length() + "s", " ");
    }
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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