Getting filtered autolist

Question

Is there a better way to write the following code fragment?

I can't have more than 10 conditional statement in my method as it gives cyclomatic complexity.

private List<AutoDto> getFilteredAutoList() 
{
    List<AutoDto> autoList = getAutoListForCoverage();
    List<AutoDto> filteredAutoList = new ArrayList<AutoDto>();

    for(AutoDto auto : autoList)
    {
        if("FAQ025".equals((auto.getCarCode()))
                ||"FAQ025".equals((auto.getCarCode()))
                ||"QEF025".equals((auto.getCarCode()))
                ||"QEF037AB".equals((auto.getCarCode()))
                ||"FAQ037AB".equals((auto.getCarCode()))
                ||"FAQ045".equals((auto.getCarCode()))
                ||"QEF045".equals((auto.getCarCode()))
                ||"NBEF028".equals((auto.getCarCode()))
                ||"OPCF028".equals((auto.getCarCode()))
                //20 more OR conditions
                )
        {
            filteredAutoList.add(auto);
        }
    }
    return filteredAutoList;
}

The following two approaches came to my mind:

1. Using HashSet

   private List<AutoDto> getFilteredAutoList() 
   {
       List<AutoDto> autoList = getAutoListForCoverage();
       List<AutoDto> filteredAutoList = new ArrayList<AutoDto>();
   
       Set<String> carCodeSet = getCarCodeSet();
   
       for(AutoDto auto : autoList)
       {
           if(carCodeSet.contains(auto.getCarCode()))
           {
               filteredAutoList.add(auto);
           }
       }
       return filteredAutoList;
   }
   
   
   private Set<String> getCarCodeSet()
   {
       Set<String> carCodeSet = new HashSet<String>();
   
       carCodeSet.add("FAQ025");
       carCodeSet.add("QEF025");
       carCodeSet.add("QEF037AB");
       carCodeSet.add("FAQ037AB");
       carCodeSet.add("FAQ045");
       carCodeSet.add("QEF045");
       carCodeSet.add("NBEF028");
       carCodeSet.add("OPCF028");
       //20 more codes to be added
   
       return carCodeSet;
   }
   

2. Using String contains method

   private List<AutoDto> getFilteredAutoList1() 
   {
       List<AutoDto> autoList = getAutoListForCoverage();
       List<AutoDto> filteredAutoList = new ArrayList<AutoDto>();
   
       String carCodeString = "FAQ025,QEF025,QEF037AB,FAQ037AB,FAQ045,QEF045,NBEF028,OPCF028";
       for(AutoDto auto : autoList)
       {
           if(carCodeString.contains(auto.getCarCode()))
           {
               filteredAutoList.add(auto);
           }
       }
       return filteredAutoList;
   }
   

Which approach should I follow and why?

Answer 1

Why don't you simply use an array of strings, as in:

String carCodeSet[] = {"FAQ025","QEF025","QEF037AB", /*...*/ };

this seems to me cleaner. Of course the HashSet would have better performance if the list of codes is very long... but maybe for 20 items a linear search on the array is not bad.

The String.contains method poses some problems in the fact that you cannot have the separator (comma) in your strings and, more importantly, that any sub-string would match: for example the code "FAQ" would be found as a sub-string. These might not be an issue in your actual use case, but is something which I would prefer not to have in my code.

Answer 2

Use Collection.contains.

That reduces your if statement to the following:

if ( filterCarCodes.contains(auto.getCarCode()) ){
    //stuff
}

You can initialize the Collection once with Arrays.asList, using a String array. Then declare it as a static final class variable and you're pretty much set. An alternative is the HashSet you described - via Arrays.asList you'd have a filled Collection, which is one of the constructors for a HashSet with contents: HashSet(Collection<? extends E> c). You'll pay for the double conversion, but it should be faster than directly using a List.

Answer 3

Just a note for the curious.

String comparisons are memory comparisons which are slow as you have to check all bytes in the source operands for a successful or near-successful match. But they can also exit early.

So comparing one \$k\$ character string to \$n\$ strings of \$k\$ characters is anywhere between \$k\$ (match on first) and \$k\cdot n\$ character comparisons (all strings differ on last character). The JIT can optimize memory comparison to use native words instead of chars in loop unrolling.

The array approach suffers from the same problem.

Using a HashSet computes the hashes once for all \$n\$ strings as a pre-processing step. Then comparing one \$k\$ character string is equivalent to computing the hash for the string, which is \$\mathcal{O}(k)\$ character operations and then a lookup in the hash table is just an indexing operation, \$\mathcal{O}(1)\$.

So ignoring the setup cost, the runtime improvement would be \$\mathcal{O}(nk) \rightarrow \mathcal{O}(k)\$ which seems like a good deal to me.

Answer 4

Look at the algorithm: the codes work as a set, so use a set.

Your program is then clearer, shorter, and less prone to error. For example, you've inadvertantly (I assume) repeated the first code: harder to do if the codes are all lined up ready to be marched into a set constructor.

You have also repeatedly evaluated auto.getCarCode(). The performance hit is no doubt trivial, but you have added a lot of superfluous tokens for the human eye to scan. And how do you make all these condition lines? You copy the first one, and paste, paste, paste. Enough said.

---

How best to do this? This answer, suggests (untested - corrections welcome) something like:

private static String [] carCodeArray =
  {"FAQ025", "QEF025", "QEF037AB", "FAQ037AB", "FAQ045", 
   "QEF045", "NBEF028", "OPCF028" /* ... */};

private static Set<String> carCodeSet = new HashSet<String>(Arrays.asList(carCodeArray));

private List<AutoDto> getFilteredAutoList (List<AutoDto> autoList) 
{
    List<AutoDto> filteredAutoList = 
      autoList
      .stream()
      .filter(auto -> carCodeSet.contains(auto.getCarCode())
      .collect(Collectors.toList());

    return filteredAutoList;
}

I've taken the liberty of absorbing the undefined getAutoListForCoverage() method into the autoList argument.

---

I'm worried that this set of codes is to be burned into the program. It should surely be read in from a configuration file. Is changing the set of codes a program change, with all the testing that entails? I hope not.

---

Afterthought

How much more concisely this can be expressed in a functional language. In Clojure, for example, after

(def car-codes #{"FAQ025" "QEF025" "QEF037AB" "FAQ037AB" 
                 "FAQ045" "QEF045" "NBEF028" "OPCF028" #_( ... ))

... it can reduce to something like

(partial filter (comp car-codes :car-code))

... which will work on any collection of things with a :car-code field.

Answer 5

Follow Through with the Object Oriented Answers

Some of the answers I've read call for a collection, and implementing a collection is exactly the right object oriented answer. BUT it's not the right kind of collection.

EDIT explicitly address cyclomatic complexity

• Building data structure is worthwhile. The OP's specific concern is solved with a collection.

• A business-meaningful collection significantly reduces a client's workload, e.g. complexity

  • Good adherence to single responsibility principle naturally reduces this metric score. "Good OO manages complexity"
  • a client does not have to parse a generic string to determine if it's "the right kind of string."
  • Client code does not end up as a cascade of nested conditionals in an attempt to assert/ascertain an object's state before calling a method.

• Necessary structure and logic is distributed, becomes less nested, with a naturally lower complexity score.

• The classes below do far more validation and error checking yet the cyclomatic complexity is low.

end edit

I humbly suggest a class as shown below.

• The autoCode is defined - that's how I see things - by virtue of containment in a class with business domain meaning. "FAQ025" is just a string. AutoDto.autoCode is an Auto Code.

• A generic collection of generic strings falls short. It means nothing. Well, it does mean clients will be forever transforming strings to string collections to AutoCode objects and back again.

• Consistency. Ease of Maintenance.

  • string structures are forced to AutoCode objects in the constructors.
  • Now all we have to code for is AutoCode objects.
  • Up-front input validation; avoid the "could be null, could be empty" string quandary. We don't blow up just because a reference is null or a string is empty.

• Single Responsibility Principle.

  • The task at hand is a natural fit for a collection iteration. And GetAutoCode() is in the right class, not a "utility" method floating around in CodeBase Land somewhere.
  • AutoCode knows what it's equal to. The client does not have to know AutoCode implementation details and does not need to know how to test for equality.

• Re-usability. With the GetAutoCode method in the proper class it is by design re-useable.

• DRY, Don't Repeat Yourself. Without the class encapsulation we force every client to replicate the data setup.

• Decouple from client code (optional)

  • If the AutoCode values are coming to you in some odd-ball structure write a separate class that plucks out the particulars and passes it to AutoCodeCollection constructors in the expected form.
  • IMHO it's a judgement call; not really needed unless the odd-ball structure is really complex and we have several classes of this nature.

Warning this is C#, but you get the idea

public class AutoDto {
    public AutoDto (string aCode) {
        this.autoCode = aCode?? string.Empty; // if aCode is null, use string.Empty;
    }

    protected string autoCode;
    public string GetAutoCode() { return this.autoCode; }

    public override bool Equals (object other) {
        if (other == null) return false;
        if(!other is AutoCode) return false;
        if(this.autoCode == other.autoCode) return true;
        return false;
    }
}

public class AutoDtoCollection : List<AutoDto> {
    public AutoDtoCollection(HashTable autoCodeValues) {}
    public AutoDtoCollection(List<string> autoCodeValues) {}

    // if you must accept a string
    public bool Contains (string testCode) {
        if (string.IsNullOrWhitespace(testCode)) return false;
        AutoCode tempObject = null;

        // don't implement AutoCode.Equals a second time
        // by violating SRP and making this collection class
        // do the equal-izing.
        foreach (AutoCode mycode in this) {
            tempObject = new AutoCode(testCode);
            if ( mycode.Equals(testCode)) return true;
        }

        return false;
    }

    public override bool Contains (object other) {
        if (other == null) return false;

        if (! other is AutoDto) return false;

        // we overrode Equals so we're comparing on the autocode property
        if (this.Contains(other))  return true; 

        return false;
    }

    public override bool Add(string aCode) {
        if(string.IsNullOrWhiteSpace(aCode)) return false;
        if(this.Add(new AutoCode(aCode))  // allows duplicates
            return true;
        else
            return false;
    }

}

public class AutoCodeCollectionFactory {
    public AutoCodeCollection Create (HashTable theCodz) {
        foreach (var thing in theCodez {
            // property extraction sound effects here
         }
    }

    public AutoCodeCollection Create (SomeCustomeClass theCodz) {
       // more fussing about to extract what we need
    }
}

Answer 6

I would do it like this.

private List<AutoDto> getFilteredAutoList(Collection<String> carCodes) 
{
        List<AutoDto> autoList = getAutoListForCoverage();
        List<AutoDto> filteredAutoList = new ArrayList<AutoDto>();
        for (AutoDto auto : autoList)
        {
            if (carCodes.contains(auto.getCarCode()))
            {
                 filteredAutoList.add(auto);
            }           
        }
        return filteredAutoList;
}

You can pass whatever collection of car codes you want to the filtering function. If you want your codes to be unique, use a set, as it does not allow duplicates.