Searching for a specific object in an ArrayList [closed]

Question

Is this an efficient way to search for a specific object in an ArrayList?

Product is the superclasS. Mobo, CPU, RAM, GPU... are the subclasses. ArrayList(called stock) stores Product objects. I consider that they key i am searching is found when class and the attribute "model"(you can see the getter of the attribute in my code) are matching.

public static boolean checkAvailability(ArrayList<Product> stock, Product product) {
    boolean found = false;
    for(Product p: stock) {
        if(p instanceof Mobo && (product.getModel() == p.getModel())) {
            found = true;
            break;
        };
        if(p instanceof CPU && (product.getModel() == p.getModel())) {
            found = true;
            break;
        };
        if(p instanceof GPU && (product.getModel() == p.getModel())) {
            found = true;
            break;
        };
        if(p instanceof RAM && (product.getModel() == p.getModel())) {
            found = true;
            break;
        };
        if(p instanceof Monitor && (product.getModel() == p.getModel())) {
            found = true;
            break;
        };
        if(p instanceof Keyboard && (product.getModel() == p.getModel())) {
            found = true;
            break;
        };
        if(p instanceof Mouse && (product.getModel() == p.getModel())) {
            found = true;
            break;
        };
        if(p instanceof Printer && (product.getModel() == p.getModel())) {
            found = true;
            break;
        };
    };
    return found;
}

Answer 1

Your code is very wet (the opposite of DRY). It repeats a lot. Let's start with what you're trying to achieve:

1. You have an ArrayList of Products.
2. You want checkAvailability to return true if any Product within the given ArrayList has the same model and type of the given Product.

The way you've implemented this, with instanceof checks, is IMO not ideal. Instead, let's modify the Product class to have a custom equals() method to compare equality of Products; we can store the equality logic inside of the Product class so it's reusable.

The equals() method on an Object, by default, will act similar to == - that is, it will compare referential equality, but it's a common idiom to override this method to compare logical equality instead.

class Product {
    ...

    @Override()
    public boolean equals(Object other) {
        // This is unavoidable, since equals() must accept an Object and not something more derived
        if (other instanceof Product) {
            // Note that I use equals() here too, otherwise, again, we will check for referential equality.
            // Using equals() here allows the Model class to implement it's own version of equality, rather than
            // us always checking for referential equality.
            Product otherProduct = (Product) other;
            return otherProduct.getModel().equals(this.getModel());
        }

        return false;
    } 
}

Of course, this does lose the instanceof checking in your checkAvailability method, but you can easily reintroduce something similar by overriding equals() in the subclasses of Product if you wanted to reintroduce this check (Spoiler: You probably don't! Don't compare types unless you have to). At the very least, try to move equality logic to an equals() method on a class rather than comparing equality outside of the class. Encapsulation and reusability is always nice!

With this override, we can simplify checkAvailability to the following:

public static boolean checkAvailability(ArrayList<Product> stock, Product product) {
    for (Product p : stock) {
        if (p.equals(product)) {
            return true;
        }
    }

    return false;
}

Which can be simplified to use Java streams if you'd like.

public static boolean checkAvailability(ArrayList<Product> stock, Product product) {
    return stock.stream()
                .anyMatch(p -> p.equals(product));
}

Or, even better, we can make the required type less specific:

public static boolean checkAvailability(Collection<Product> stock, Product product) {
    return stock.stream()
                .anyMatch(p -> p.equals(product));
}

As an aside: it is generally good practice to reimplement hashCode() if you reimplement equals(). Without having more knowledge of your solution, it's difficult for me to tell you how to do this, but I would recommend you do it if you end up using my suggestions.

Another side note: If you find yourself making lots of comprehensions like this, I would recommend having checkAvailability accept a Stream<Product> instead of Collection<Product> so as to avoid creating multiple streams.

Answer 2

1. there is a redundant semi-colon ';' at the end of every if and at the end of the loop (basically after closing curly braces)

2. code to the inteface: the method should accept an argument of List type. that way callers may choose which list implementation to use (e.g. LinkedList)

3. the boolean variable is redundant. you can immediately return true if match is found, otherwise return false

       if(p instanceof Mobo && (product.getModel() == p.getModel())) {
           return true;
       }
       ...
   }  // closing the for loop
   return false;
   

4. the sequence of if questions can easily be collapsed into one condition:

   if ((p instanceof Mobo || p instanceof CPU || ...) && product.getModel() == p.getModel()) {
       return true;
   }
   

5. but instead of questioning against individual types, you can put them all in a collection and query on containment:

   private static Set<Class<? extends Product>> productTypes;  // class variable
   static {  // static constructor
      productTypes.add(Mobo.class);
      productTypes.add(CPU.class);
   }
   
   // inside the method        
   if (productTypes.contains(p.getClass()) && product.getModel() == p.getModel()) {
       return true;
   }
   

Answer 3

You ignored my comment asking for clarification, so I'll just assume that I was right in thinking that your code doesn't actually do what you intend it to do. You write that, for a match, the class and model needs to match, but your code compares only the model of the search key with that of the items in the stock. It doesn't check whether an item from the stock has the same class as the search key, it just ascertains that the class of the stock item is one of several classes, regardless of whether this class is also the class of the search key.

In fact, your verbal description of the code is very simple, and the translation into code should not be much more complex:

public static boolean checkAvailability(List<Product> stock, Product product) {
    for(Product p: stock) {
        if (product.getModel() == p.getModel()
                && product.getClass() == p.getClass()) {
            return true;
        }
    }
    return false;
}

You can also use a stream instead:

public static boolean checkAvailability(List<Product> stock, Product product) {
    return stock.stream().anyMatch((Product p) ->
            product.getModel() == p.getModel()
                    && product.getClass() == p.getClass());
}

Note that, unlike instanceof, this only checks the classes for equality and returns false if one class is a subclass of the other. If you want to consider subclasses as well, you can use Class.isAssignableFrom(Class).

You should also be aware that, unless Product.getModel() returns a primitive type, you are comparing the models based on reference equality rather than comparing their contents. I don't know what type Product.getModel() returns, but even if it only returns a simple String, == will not consider the contents of the Strings but only check whether the two variables reference the same object (which might not necessarily be the case even if the String's contents are identical), so you would need to use equals(Object) if you want to compare the contents of the two Strings.

Answer 4

The instanceof keyword is usually a sign of bad inheritance (Liskov's substitution principle) and I do not recommend to use that.

Beside that, I do not understand that data structure of your stock information.

Why not something like this?

boolean stockAvailable(Map<ProductId, Integer> stock, Product product) {
    if(!stock.contains(product.getProductId()) { // ... guard
        return false;
    }

    return stock.get(product.getProductId()) > 0;
}