Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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?

share|improve this question
4  
the second solution does something different than the original code. What if I have a car code that is FAQ02? That is contained in FAQ025,Q..., but it shouldn't count. I would use a static array (define it at the top of the class) and check if the car code is in it. –  tim Aug 5 at 11:45
    
Where does that list of codes come from in the first place? –  200_success Aug 6 at 4:16

6 Answers 6

up vote 10 down vote accepted

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.

share|improve this answer
    
Array solution looks fine to me. I can check Arrays.binarySearch(carCodeArray, key) in my if statement. Can you tell me how this is better than the Set solution Apart from being more readable? –  Lone Rider Aug 5 at 13:08
1  
It is best only for readability. I think it is the best way to insert a static list of strings in the source code. But if these strings are elsewhere (input? configuration file?) then things change... –  Emanuele Paolini Aug 5 at 13:11
1  
If you were in C# (just now noticed this was a java question), you can get the same readability with a HashSet using a C# collection initializer: readonly HashSet<string> carCodeSet = new HashSet() { "FAQ025", "QEF025", ... }. In my experience, it's better to use the HashSet in almost all cases. Even if you don't think you need the O(log n) look up afforded by a HashSet, you probably do. –  WorldMaker Aug 5 at 16:49

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.

share|improve this answer

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.

share|improve this answer
    
Your explicit solution fails - HashSet can't be constructed from an array. You can use Arrays.asList to convert the array into a collection, though. –  Pimgd Aug 7 at 12:07
    
@Pimgd corrected - thank you. –  Thumbnail Aug 7 at 13:10

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
        // to 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
    }
}
share|improve this answer

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.

share|improve this answer

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.

share|improve this answer
    
Sorry, but why would he chose a List over e.G. a HashSet? There's no actual benefit in using a List, except for the fact that you seem to be more comfortable with it... –  Vogel612 Aug 5 at 13:11
    
I am indeed more comfortable with using a List. A HashSet implements Collection so he may pass a HashSet if he so desires. But if the application does not require the speed of a HashSet, the benefits brought by it are meaningless and it's just as good as a List. I also find List to be more commonly used, so unless there's an actual need for a HashSet, I'll use List, even if only for the sense of familiarity. –  Alex M. Aug 5 at 13:17
4  
Keep in mind that Lists allow duplicate items and Sets do not... This is an additional factor you should take into consideration. furthermore using a list is totally overkill, the List interface provides an enormous amount of unneeded functionality. OP initially used Set. This has outlined two benefits (no dupes, minimal functionality). Performance isn't even the biggest problem I have with your suggestion. –  Vogel612 Aug 5 at 13:33
2  
I'd argue that code uniqueness shouldn't really be enforced by the underlying collection, but by an outside contract. I can't argue against the minimal functionality, I guess, though I do believe it's not enormous. –  Alex M. Aug 5 at 13:40
    
Exaggeration is my tool of choice ;) While the uniqueness should be enforced by an outside contract, IMO it's the incorrect decision to assume that it were. It's easier to explicitly force the uniqueness by using a set, instead of assuming uniqueness and using a list. –  Vogel612 Aug 5 at 13:50

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.