search in java ArrayList

Question

I'm trying to figure out the best way to search a customer in an ArrayList by its Id number. The code below is not working; the compiler tells me that I am missing a return statement.

Customer findCustomerByid(int id){
    boolean exist=false;

    if(this.customers.isEmpty()) {
        return null;
    }

    for(int i=0;i<this.customers.size();i++) {
        if(this.customers.get(i).getId() == id) {
            exist=true;
            break;
        }

        if(exist) {
            return this.customers.get(id);
        } else {
            return this.customers.get(id);
        }
    }

}

//the customer class is something like that
public class Customer {
    //attributes
    int id;
    int tel;
    String fname;
    String lname;
    String resgistrationDate;
}

Answer 1

Others have pointed out the error in your existing code, but I'd like to take two steps further. Firstly, assuming you're using Java 1.5+, you can achieve greater readability using the enhanced for loop:

Customer findCustomerByid(int id){    
    for (Customer customer : customers) {
        if (customer.getId() == id) {
            return customer;
        }
    }
    return null; 
}

This has also removed the micro-optimisation of returning null before looping - I doubt that you'll get any benefit from it, and it's more code. Likewise I've removed the exists flag: returning as soon as you know the answer makes the code simpler.

Note that in your original code I think you had a bug. Having found that the customer at index i had the right ID, you then returned the customer at index id - I doubt that this is really what you intended.

Secondly, if you're going to do a lot of lookups by ID, have you considered putting your customers into a Map<Integer, Customer>?

Answer 2

The compiler is complaining because you currently have the 'if(exist)' block inside of your for loop. It needs to be outside of it.

for(int i=0;i<this.customers.size();i++){
        if(this.customers.get(i).getId() == id){
            exist=true;
            break;
        }
}

if(exist) {
    return this.customers.get(id);
} else {
    return this.customers.get(id);
}

That being said, there are better ways to perform this search. Personally, if I were using an ArrayList, my solution would look like the one that Jon Skeet has posted.