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;
}

• victor hugo: One of the main Java Good Practices is to ALWAYS use braces even if the block has just one sentence to avoid problems like yours

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.

• Jon Skeet: I strongly suspect that code is still broken though (note the argument to customers.get at the end). The fact that it also still has an "if/else" where both branches have the same code is deeply suspicious too!
• Brandon E Taylor: I agree, Jon. Moreover, there are certainly better ways to perform the search (as several other posters have indicated).
• Spider: why is there an if else at the end of this? You could remove the check and just return this.customers.get(id); it does the same as having that if else block there plus remove an extra check that isnt needed in that code. the way i would do it would be to return true if it does exist and false if it doesnt, you can even remove the else part of the statement as once the if statement is true then it isnt going to continue executing anyway

Answer 3

Personally I rarely write loops myself now when I can get away with it... I use the Jakarta commons libs:

Customer findCustomerByid(final int id){
 return (Customer) CollectionUtils.find(customers, new Predicate() {
 public boolean evaluate(Object arg0) {
 return ((Customer) arg0).getId()==id;
 }
 });
}

Yay! I saved one line!

• Brian Agnew: You've saved one line for every time you write a find() function :-) That's not to be sneezed at!
• Spider: You might of "saved" one line of code everytime you write that but i bet you if you looked at how that find function works you will see that it is using a for loop. So one must ask them self have you really "save" any lines or added more lines to the underlining code? ;)
• Dan Gravell: @Spider Runtime execution is just one way to measure the code... there are other benefits to having less repeated code.

Answer 4

Customer findCustomerByid(int id){
 for (int i=0; i<this.customers.size(); i++) {
 Customer customer = this.customers.get(i);
 if (customer.getId() == id){
 return customer;
 }
 }
 return null; // no Customer found with this ID; maybe throw an exception
}

• waxwing: Moving out the if statement is not sufficient to fix the method. Your solution is preferred. +1

Answer 5

You're missing the return statement because if your list size is 0, the for loop will never execute, thus the if will never run, and thus you will never return.

Move the if statement out of the loop.

Answer 6

Even if that topic is quite old, I'd like to add something. If you overwrite equals for you classes, so it compares your getId, you can use:

customer = new Customer(id);
customers.get(customers.indexOf(customer));

Of course, you'd have to check for an IndexOutOfBounds-Exception, which oculd be translated into a null pointer or a custom CustomerNotFoundException.

Answer 7

In Java 8:

Customer findCustomerByid(int id) {
 return this.customers.stream()
 .filter(customer -> customer.getId().equals(id))
 .findFirst().get();
}

It might also be better to change the return type to Optional<Customer>.

Answer 8

I did something close to that, the compiler is seeing that your return statement is in an If() statement. If you wish to resolve this error, simply create a new local variable called customerId before the If statement, then assign a value inside of the if statement. After the if statement, call your return statement, and return cstomerId. Like this:

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;
 }
 int customerId;
 if(exist) {
 customerId = this.customers.get(id);
 } else {
 customerId = this.customers.get(id);
 }
 }
 return customerId;
}