4
\$\begingroup\$

I have been staring at this code for awhile now and I am thinking there is a way to optimize it (namely the if-else statement with the for-loops).

Additionally, this needs to be done with an array list as that is my assignment requirement. I have all my code working, and it is not due for a few days, so I am looking for ways to improve it while extending my learning beyond the classroom.

The numberOfContacts variable is fed into the method from the main method, which gets the count from another method that reads through a text file to get the number of contacts.

/**
 * search method searches through the contact list for a given criteria
 * and displays all results.
 * @param searchString is type String.
 * @param type is type String.
 * @param numberOfContacts is type int.
 * @param contacts is type Person[].
 */
public static void search(String searchString, String type, int numberOfContacts, Person[] contacts) {
    // Initialize variables for results
    int found = 0;
    int[] results = new int[numberOfContacts];

    // Determine the type of search
    if (type.equals("name")) {
        // Search by name
        for (int x = 0; x < numberOfContacts; x++) {
            if (contacts[x].getName().contains(searchString)) {
                results[found] = x;
                found++;
            }
        }
    } else {
        // Search by phone
        for (int x = 0; x < numberOfContacts; x++) {
            if (contacts[x].getPhone().contains(searchString)) {
                results[found] = x;
                found++;
            }
        }
    }

    // Display the search results
    System.out.println("\n\t**************");
    System.out.println("\tSearch Results");
    System.out.println("\t**************");
    System.out.println("Found " + found + " results containing \"" + searchString + "\":");
    System.out.println();

    if (found > 0) {
        for (int x = 0; x < found; x++) {
            System.out.println(contacts[results[x]].getName() + "\t" + contacts[results[x]].getPhone());
        }
    }

    System.out.println("\n\n\n");
}
\$\endgroup\$
2
\$\begingroup\$

If you are limited to use only Arrays, you can create a interface like this

public static interface Checker {
    public boolean check(Person p, String searchString);
}

Create a Checker for every type of comparison you want.

And compare with the Checker

for (int x = 0; x < numberOfContacts; x++) {
    if (checker.check(contacts[x], searchString)) {
        results[found] = x;
        found++;
    }
}

You can see a working example here (with your classes and methos): http://ideone.com/2C6kei

Note: With this method is easy to expand the search criteria (as you see in the example, with a anywhere Checker).

\$\endgroup\$
1
\$\begingroup\$
  1. You should clarify what your homework statement exactly says: a Java "array list" is something like

    List<Person> persons = new ArrayList<Person>()
    

    while Person[] persons is a Java "array".

  2. Java is not Fortran or C; there is no need for the argument int numberOfContacts, so you can just call contacts.length to get the size of an array. If you use an ArrayList, it is contacts.size() instead.

  3. When you print out the results, there is no need for if (found > 0) since the for-loop will not be executed a single time if there is nothing to execute.

    Other than that the algorithm looks fine; I don't see much to optimize.
    Optional suggestions:

  4. Instead of using an if-statement to check the type, you can look into the Java switch-statement. That would be useful especially if you many types: "name", "phone", "address", "age", etc.

  5. Instead of using hardcoded strings for the type, look into the enum:

    public enum ContactField {
        NAME, PHONE;
    }
    

    and instead of passing an argument of type String, use the type ContactField. The switch-statement works with String and enum.

SOME EDITS ONE YEAR LATER:

  1. In the for-loop, use int i instead of int x. It is just a convention, but everyone uses it.

  2. "Separation of concern": have your search method only return the list of matches. If you want to print them, print the returned list elsewhere. You want to break your code in small methods so that each one can be reused. For example, you might use your search method in a web server framework and you would need to send the matches over to the user's web page instead of printing them to the console.

Since Java 8 came out, you could instead use:

public Stream<Person> searchByName(String name) {
    return Arrays.stream(persons).filter(person -> person.getName().contains(targetName));
}

public Stream<Person> searchByPhone(String phone) {
    Arrays.stream(persons).filter(person -> person.getPhone().contains(targetPhone));
}

and then you can whatever you want with that stream. For example,

personsStream.forEach(System.out::println);

However, since your instructor asked you to use arrays, it will probably take another 15 years before he/she switches over to the Java 8.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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