Parsing length of work experience using Composite design pattern

Question

I want to get a number from given text (if it exists) which is about work experience in months. I assumed that number is in months when in the text one writes for example "27 miesięcy" or "27 miesiecy" or "27miesięcy" or "27miesiecy, and in years if one writes "3 lat" or "3lat".

Class ExperienceAlg0 is mainly responsible for that. I wanted to make it flexible and open for adding another rules/algorithms "how to find the number of months in the text". I tried to use composite design pattern.

The algorithm works well, but is the design OK according to OOP and Uncle Bob?

import java.util.ArrayList;
import java.util.List;

import org.junit.Test;

import junit.framework.TestCase;

public class TestExperienceAlg0 extends TestCase {

/* 
**
** this test class unit test show how app works 
**
*/
    @Test
    public void testCalc1() {
        // Given:
        String text = "programuję od 5 lat.";

        // When:
        List<ExperiencePattern> patternList = new ArrayList<>();
        patternList.add(new ExperiencePatternMonths());
        patternList.add(new ExperiencePatternYears());
        ExperienceAlg0 alg = new ExperienceAlg0(patternList);
        int result = alg.calc(text);

        // Then:
        assertEquals(0, result);
    }
}

---

public interface ExperiencePattern {

    public int getPattern(String text);

}

---

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

public class ExperiencePatternMonths implements ExperiencePattern{

    @Override
    public int getPattern(String text) {
        int result = 0;
        Pattern p = Pattern.compile("\\d+miesi[e|ę]cy|\\d+ miesi[e|ę]cy");
        Matcher m = p.matcher(text);

        while (m.find()) {
            String tmp = m.group();
            tmp = tmp.replaceAll("\\D", "");
            int x = Integer.parseInt(tmp);
            if (x < (40 * 12)) { // assuming that the max experience is 40
                                    // years.
                result = x;
                ;
            }
        }
        return result;
    }



}

---

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

public class ExperiencePatternYears implements ExperiencePattern{

    @Override
    public int getPattern(String text) {
        int result = 0;
        Pattern p = Pattern.compile("\\d+lat|\\d+ lat");
        Matcher m = p.matcher(text);

        while (m.find()) {
            String tmp = m.group();
            tmp = tmp.replaceAll("\\D", "");
            int x = Integer.parseInt(tmp);
            if (x < 40) { // assuming that the max experience is 40 years.
                result = x * 12;
            }
        }
        return result;
    }



}

---

public interface Algorithm <T> {

    public T calc(String textToLower);

}

---

import java.util.ArrayList;
import java.util.Collections;
import java.util.List;

public class ExperienceAlg0 implements Algorithm<Integer> {

    List<ExperiencePattern> listPatterns;

    public ExperienceAlg0(List<ExperiencePattern> list){
        listPatterns = list;
    }

    @Override
    public Integer calc(String text) {
        List<Integer> result = new ArrayList<>();

        for(ExperiencePattern ep : listPatterns){
            result.add(ep.getPattern(text));
        }

        Collections.sort(result, new DescendingComparator());
        return result.get(0);
    }




}

---

import java.util.Comparator;
public class DescendingComparator implements Comparator<Integer>{

    @Override
    public int compare(Integer x1, Integer x2) {

        return x2 - x1;
    }



}

Answer 1

You wrote this:

    Pattern p = Pattern.compile("\\d+miesi[e|ę]cy|\\d+ miesi[e|ę]cy");

but it would be more natural to express it without alternation:

    Pattern p = Pattern.compile("\\d+\\s*miesi[e|ę]cy");

I wonder if you maybe wanted to throw in a CASE_INSENSITIVE | UNICODE_CASE flag.

public interface ExperiencePattern {

Many projects would choose to spell this IExperiencePattern.

DRY - don't repeat yourself. It seems odd that you have a pair of copy-n-pasted identical classes with different Pattern constants, rather than one class that accepts a Pattern parameter.

        tmp = tmp.replaceAll("\\D", "");

Thank you for teaching me about the "non-digit" pattern. But the more natural way to express your intent would be to delete this, and put a capture group in your regex:

    Pattern p = Pattern.compile("(\\d+)\\s*miesi[e|ę]cy");

Then Integer.parseInt(m.group(1)) gives you exactly what you want.

        if (x < 40) {
            result = x * 12;

Ok, I guess in a straight refactor you would need to pass in two parameters, a Pattern and also maxAcceptableValue. But it's probably better to stick with a single parameter and let the caller do range checking.

public interface Algorithm <T> {

    public T calc(String textToLower);

Both of those identifiers are hopelessly vague. Give the Gentle Reader a clue, please. Also, adding some /** javadoc */ would help.

public ExperienceAlg0(List<ExperiencePattern> list){
    listPatterns = list;
}

It is customary in a ctor to explicitly mention "this": this.listPatterns = list;

    for(ExperiencePattern ep : listPatterns) {
        result.add(ep.getPattern(text));
    }

This is perfectly lovely. But generalizing to exactly 2 cases is maybe on the small side. Rather than this loop, I encourage you to hoist the range check test up to this location, which will feature a pair of nearly identical calls using each of your patterns.

    Collections.sort(result, new DescendingComparator());
    return result.get(0);

That's cool. I like it. Very clean way to discard chaff.

You asked if the design is OK according to OOP. Well, that would be easier to evaluate if you added one-sentence javadocs at top of each class, explaining the single responsibility of that class, especially for any interface classes. That would e.g. help a reader pin down which layer the range-checking belongs in.