Searching an array list for a specific record in java

Question

Im writing a method to return a specific record in an array however it throws up two errors and Im not sure how to fix it. Can anyone explain what I'm doing wrong?

public String find(String searchName) 
{ // ERROR - MISSING RETURN STATEMENT
    Iterator<TelEntry> iterator = Directory.entries.iterator();
    boolean hasFound = false;
    while (iterator.hasNext()) 
    {
        TelEntry entry = iterator.next();

        if (entry.name.equalsIgnoreCase(searchName)) {
            return entry.name + entry.telNo;
            hasFound = true; // ERROR UNREACHABLE STATEMENT
        }

    }
    if (hasFound==false)
    {
        System.out.println("sorry, there is noone by that name in the Directory. Check your spelling and try again");
    }
}

Can anyone explain what I've done wrong?

if (!hasFound) throw new NoSuchElementException(); (and document it in the method documentation with an @throws clause) — ignis, Oct 27 '12 at 17:59

score 6 · Accepted Answer · answered Oct 27 '12 at 18:01

The basic problem you have is that when a match is not found, you have no return statement. Usually, a method will return null is such cases, but you may want to return searchName, or even the error message - it depends on what the intention/contract of the method is (not stated).

However, the other problem you have is that your code is way too complicated for what it's doing, especially the hasFound variable is completely useless.

Change your code to this, which does exactly the same thing but is expressed more elegantly:

public String find(String searchName) {
    for (TelEntry entry : Directory.entries) {
        if (entry.name.equalsIgnoreCase(searchName)) {
            return entry.name + entry.telNo;
        }
    }
    System.out.println("sorry, there is noone by that name in the Directory. Check your spelling and try again");
    return null; // or return "searchName", the error message, or something else
}

score 2 · Answer 2 · answered Oct 27 '12 at 17:50

2

return statement should be last staetement in a block. Change your code below:

if (entry.name.equalsIgnoreCase(searchName)) {
            hasFound = true; // ERROR UNREACHABLE STATEMENT
            return entry.name + entry.telNo;
        }

answered Oct 27 '12 at 17:50

kosa

65,990
13
130
167

+1 I had just started typing that and there was new answer notification :) – Amit Deshpande Oct 27 '12 at 17:52
2

If you really understood the code, you would notice that the `hasFound` variable has no good use in the method. – Luiggi Mendoza Oct 27 '12 at 17:54
@Nambari that would be the best advice. Also, note that the method won't compile even if you fix this line, because after the loop you have to return a `String`,and your answer doesn't fix that. – Luiggi Mendoza Oct 27 '12 at 17:57
SO community still confuses me. Should answer cover the issue raised? write a new program for OP? – kosa Oct 27 '12 at 17:58
Have you at least copy/paste OP code somewhere and test if your answer makes the code compile? – Luiggi Mendoza Oct 27 '12 at 18:01
@LuiggiMendoza: well , we(atleast me) can't compile everything and stand in OP shoes. There are lot of factors we are not aware of. I would like to restrict myself to resolving the issue(instead adding lot of suggestions). I re-read the question and agree that my answer should have covered missing return statement issue also (OP explicitly asked about it also). – kosa Oct 27 '12 at 18:08

score 2 · Answer 3 · answered Oct 27 '12 at 18:04

If a method is declared as returning a String, it must return a String, or throw an exception. Not returning anything is not acceptable. So you should decide what to do when the string is not found. You basically have two choices:

returning null
throwing an exception

Printing an error is not a good idea. Such a method shouldn't deal with the user interface. That's not its responsibility, and a method should only have one responsibility. Returning a string with an error message is not a good idea either: the caller would have no way to know if the returned string is the one which has been found, or an error string.

Moreover your code is overly complicated. It could be reduced to the following (assuming Directory.entries() implements Iterable, as it should):

public String find(String searchName) {
    for (TelEntry entry: Directory.entries()) {
        if (entry.name.equalsIgnoreCase(searchName)) {
             return entry.name + entry.telNo;
        }
    }
    return null;
}

I would change the return type, so, and make it return a TelEntry instance. Let the caller deal with the concatenation. It's not the responsibility of this method either.

@the OP: Whatever choice do you choose, remember to clearly state it in the method documentation. — ignis, Oct 27 '12 at 18:06

score 1 · Answer 4 · answered Oct 28 '12 at 10:34

1

Your code is incorrect : you cannot have instructions after a return in the same block: how could it be executed, since the function has returned ... ?

That's what the compiler is telling you : unreachable statement

answered Oct 28 '12 at 10:34

ARA

1,296
10
18

Mik378 · Answer 5 · 2012-10-27T17:58:12.837

0

Prefer return the string itself. Indeed, print it to screen directly violates SRP (Single Responsibilty principle and also avoid to return something as the method expects).

No need for boolean checker.

public String find(String searchName) { 
        Iterator<TelEntry> iterator = Directory.entries.iterator();
        while (iterator.hasNext()) {
            TelEntry entry = iterator.next();
            if (entry.name.equalsIgnoreCase(searchName)) {
                return entry.name + entry.telNo;
            }
        }
        return "sorry, there is noone by that name in the Directory. Check your spelling and try again";
}

edited Oct 27 '12 at 17:58

answered Oct 27 '12 at 17:53

Mik378

21,881
15
82
180

The provided code won't compile: unreachable statement. Test the provided code before posting it. – Luiggi Mendoza Oct 27 '12 at 17:55
2 seconds, I will compile it, I've just written it with notepad ;) – Mik378 Oct 27 '12 at 17:55
This violates the least surprise principle, and presumably the documentation of the method, which is far worse. – ignis Oct 27 '12 at 17:55
@Luiggi Mendoza That's done. You're right, for the "test it before" ;) – Mik378 Oct 27 '12 at 17:57

bancer · Answer 6 · 2012-10-27T18:19:53.900

Can anyone explain what I've done wrong?

My explanations are in comments below:

public String find(String searchName) 
    { // ERROR - MISSING RETURN STATEMENT
        Iterator<TelEntry> iterator = Directory.entries.iterator();
        boolean hasFound = false;
        while (iterator.hasNext()) 
        {
            TelEntry entry = iterator.next();

            if (entry.name.equalsIgnoreCase(searchName)) {
                return entry.name + entry.telNo;
                /*The "return" statement above stops executing
                the current method and transfers control to the 
                place from where "find" method was called. The
                statement below this line is NEVER executed.*/
                hasFound = true; // ERROR UNREACHABLE STATEMENT
            }

        }
        if (hasFound==false)
        {
            System.out.println("sorry, there is noone by that name in the Directory. Check your spelling and try again");
        }
        /*This method starts with "public String". That means that 
        it MUST return a String object or null when the method 
        finishes executing.
        The best place to return is here.*/
    }

Searching an array list for a specific record in java

6 Answers6