2
\$\begingroup\$

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;
}
\$\endgroup\$
  • 2
    \$\begingroup\$ "efficient" in what way? performance? memory? lines-of-code? \$\endgroup\$ – Sharon Ben Asher Apr 11 '18 at 10:44
  • \$\begingroup\$ @SharonBenAsher Anything. Whatever i can do to improve it in any way. \$\endgroup\$ – Κωνσταντίνος Κορναράκης Apr 11 '18 at 10:48
  • 7
    \$\begingroup\$ soo ... why are you checking the specific type of the product you're currently examining? \$\endgroup\$ – Vogel612 Apr 11 '18 at 11:06
  • 10
    \$\begingroup\$ Do you need to check the types of products? Is it not sufficient to check getModel()? \$\endgroup\$ – Dan Apr 11 '18 at 11:10
  • \$\begingroup\$ When you write in your description that the classes need to match, what exactly do you mean? I would have thought that it means the class of the search key needs to match the class of the item in the stock, is this correct? Because that's not what your code does, and I'm not sure if this is a mistake, or whether I have just misunderstood your description. \$\endgroup\$ – Stingy Apr 11 '18 at 21:38
12
\$\begingroup\$

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.

\$\endgroup\$
  • 9
    \$\begingroup\$ If you already use equals(), it is sufficent to check stock.contains(product) as long as the collection does not rely on hashCodes, i.e. the collection is a list. After implementation of hashCode() this will work for any collection. \$\endgroup\$ – mtj Apr 11 '18 at 11:45
  • \$\begingroup\$ it is beyond good practice to override both hashcode and equals if you ever intend storing your items in things like hashmap or hashset, as you could end up with multiple items that are all equal in the set/map but have different object identity. stackoverflow.com/questions/2265503/… \$\endgroup\$ – WendyG Apr 11 '18 at 15:33
  • 3
    \$\begingroup\$ The most generic form of the last signature could/should be Collection<? extends Product> (so that you could pass in a ArrayList<RAM>, for example) \$\endgroup\$ – Marco13 Apr 11 '18 at 17:20
  • \$\begingroup\$ You can simplify the stream further: return stock.stream().anyMatch(product::equals); \$\endgroup\$ – Eric Stein Apr 12 '18 at 13:17
8
\$\begingroup\$
  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;
    }
    
\$\endgroup\$
  • \$\begingroup\$ In case some of the classes can be supertypes of p.getClass() I would recommend productTypes.stream().anyMatch(c -> c.isAssignableFrom(p.getClass())) \$\endgroup\$ – Simon Forsberg Apr 11 '18 at 11:46
2
\$\begingroup\$

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.

\$\endgroup\$
2
\$\begingroup\$

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;
}
\$\endgroup\$
  • \$\begingroup\$ I'd say that's an outdated viewpoint. It's a valid inversion (thought not likely warranted here) but Java makes it ugly. Visitors are far worse however. \$\endgroup\$ – Aluan Haddad Apr 11 '18 at 17:02
  • \$\begingroup\$ @AluanHaddad: Nah, it ain't. If you introduce a new subtype, you have to change other code. If you introduce a subtype which extends a subtype, it gets even worse and very error prone. That's Liskov and SRP violated... \$\endgroup\$ – slowy Apr 12 '18 at 8:03
  • \$\begingroup\$ only if you don't handle the base case \$\endgroup\$ – Aluan Haddad Apr 12 '18 at 14:07
  • \$\begingroup\$ don't you miss a ! in that if? if( ! stock.contains(...){ return false; } \$\endgroup\$ – Imus Apr 13 '18 at 11:12
  • \$\begingroup\$ @Imus : Oops, yeah, fixed it, thanks for the hint. \$\endgroup\$ – slowy Apr 13 '18 at 15:18

Not the answer you're looking for? Browse other questions tagged or ask your own question.