3
\$\begingroup\$

I am trying to validate that when given two lists,

List<String> models;
List<String> titleOfVehicle;

that one of the models must be contained in each of the titleOfVehicle.

For example, given the following:

models = {Accord, Civic, Element}
titleOfVehicle1 = {2014 Honda Accord, 2011 Toyota Camry}
titleOfVehicle2 = {2013 Honda Accord, 2015 Honda Element, 2011 Honda Civic}

then when comparing models and titleOfVehicle1 it will fail since 2011 Toyota Camry does not contain one of the models provided. However, titleOfVehicle2 will pass since it contains a model for each vehicle titles.


Here is my code:

public boolean compareTwoList(List<String> models, List<String> titleOfVehicles) {
    boolean flag = false;

    for(int i = 0; i < titleOfVehicles.size(); i++) {
        flag = false;
        int k = 0;
        for(int j = 0; j < models.size(); j++) {
            if(titleOfVehicles.get(i).contains(models.get(j))) {
                flag = true;
                break;
            } else if(k == models.size() - 1 && flag == false) {                        
                return false;
            }    
            k++;
        }
    }    
    return flag;
}
\$\endgroup\$
4
\$\begingroup\$

Your code is more complicated than it needs to be, but it gets the job done.

The awkward part of your code is the use of the various index values i, j, and k. Really, you don't need them at all. If you rename things a bit, and use "enhanced-for" loops, it becomes:

public boolean compareTwoList(List<String> models, List<String> titleOfVehicles) {

    for(String title : titleOfVehicles) {
        boolean found = false;
        for(String model : models) {
            if(title.contains(model)) {
                found = true;
                break;
            }
        }
        if (!found) {
            return false;
        }
    }    
    return true;
}

Note that the logic is essentially the same, but you focus on the important things. The found variable is a better name, and it's scope is limited to inside the outer for-loop. The hard-to-understand k loop-terminator is removed.

Note, using streams, and a regular expression, would actually be more compact solution, but may not be as readable... I played with the stream version and a regex, and got:

public static boolean compareTwoList(List<String> models, List<String> titleOfVehicles) {

    String pattern = models.stream()
          .map(Pattern::quote)
          .collect(Collectors.joining("|", ".*(", ").*"));

    Pattern re = Pattern.compile(pattern);

    return titleOfVehicles.stream()
        .allMatch(t -> re.matcher(t).matches());

}

(which you can see working here: https://ideone.com/8sqKu7 )

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

The current code works but it is very awkward, as rolfl pointed out. I would add that you should never compare booleans with true or false, like what you're doing in:

} else if(k == models.size() - 1 && flag == false) {                        
    return false;
}

Instead, have

} else if (k == models.size() - 1 && !flag) {                        
    return false;
}

You can actually accomplish this task in a single, clear and easy line using the Stream API:

public boolean compareTwoList(List<String> models, List<String> titleOfVehicles) {
    return titleOfVehicles.stream().allMatch(t -> models.stream().anyMatch(t::contains));
}

This does exactly what is written: it returns whether all elements in the given titles contains any of the given models.

Both allMatch and anyMatch are short-circuiting operations, so it will behave exactly like your current code.

\$\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.