-1

There are 3 different user input fields like Text box & Drop downs on the UI. The user can search a table with any of the three input fields or by providing different combinations of input fields to narrow the result set.I have used if else statements to implement my logic using these three fields, but the problem is the code looks very ugly with lot of if-else statements and more over if there is one more field added to the UI then I have to check for more combinations with if else statements. I really appreciate if someone throws light on how to follow best practices or better approach in these scenarios.

My Code:

      public boolean filterResults(UserInputs userInputs){

            String firstName = userInputs.getFirstName();
            String lastName = userInputs.getLastName();
            String phoneNumber = userInputs.getNumber();

                  //**search only by First Name**
                 if((firstName!=null && firstName!="") 
                    && (lastName==null || lastName=="" )
                   && phoneNumber==null || phoneNumber ==""){

                    //logic to narrow down the result set based on condition met input values
                     list =DAO.getService(firstName);
                      return true;
                       }else if((lastName!=null && lastName!="")// **search only by Last Name**
                                  && (firstName== null || firstName == "")
                                   && (phoneNumber==null || phoneNumber == "")){

                             //logic to narrow down the result set based on condition met input values
 list =DAO.getService(lastName);                          return true;
                       }else if((phoneNumber!=null && phoneNumber!="")// **Search Only by PhoneNumber**
                                               && (lastName==null || lastName=="" )
                                                && (firstName== null || firstName == "")){
        //logic to narrow down the result set based on condition met input values
                    list =DAO.getService(phoneNumber);                                                return true;
                               }else if((firstName!=null && firstName!="")// **Search by all params**.
                                      &&(lastName!=null && lastName!="")
                                       && (phoneNumber!=null && phoneNumber!="")){
        //logic to narrow down the result set based on condition met input values
                      list =DAO.getService(firstName, lastName, phoneNumber);
                                                       return true;
                            }else if((firstName!=null && firstName!="")//**Search by first&last name.**
                                && (lastName!=null || lastName!="" )
                                && (phoneNumber==null || phoneNumber =="")){
                           //logic to narrow down the result set based on condition met input values
                                 list =DAO.getService(firstName, lastName);

                                 return true;
                                 }else if (.......){// **Search by last name & phone number.**
                                       return true;
                                   }else if (.........){// **Search by phone number & first name.**
                                    return true;}



                     return false;

            }
  • 2
    First thing is to encapsulate the `(foo!=null && foo!="")` idiom; put that in a helper method. – Oliver Charlesworth Jul 06 '14 at 20:59
  • 2
    Also, use `!foo.isEmpty()` instead of `foo != ""`. Or even better, see [Should I use string.isEmpty() or “”.equals(string)?](https://stackoverflow.com/questions/3321526/should-i-use-string-isempty-or-equalsstring) – Greg Hewgill Jul 06 '14 at 21:00
  • So.. you only return `false` if all fields are empty? – SJuan76 Jul 06 '14 at 21:00
  • @OliCharlesworth - To add to this, look at this link for some solutions using some libraries like Apache or Guava, if you have those in your project. http://stackoverflow.com/questions/3598770/how-to-check-whether-a-string-is-not-null-empty – Jimmy Thompson Jul 06 '14 at 21:00
  • Read http://stackoverflow.com/questions/513832/how-do-i-compare-strings-in-java – JB Nizet Jul 06 '14 at 21:01
  • As each `if` ends with `return`, you can just avoid the `else` statements (if the logic continues beyond the `if`, it is in the `else`) – SJuan76 Jul 06 '14 at 21:02
  • SJuan76 if all fields empty ill return true and add entire table as result set. – user3810342 Jul 06 '14 at 21:28

2 Answers2

2

Right now, your logic is equivalent to this single line:

return (firstName != null && !firstName.isEmpty()) ||
    (lastName != null && !lastName.isEmpty()) ||
    (phoneNumber != null && !phoneNumber.isEmpty());
Bohemian
  • 412,405
  • 93
  • 575
  • 722
0

It looks like you're implementing a search method for each possible combination of your search criteria. You shouldn't do it like that. Instead, the method filtering the elements should test each criterion:

public List<Element> search(List<Element> allElements, UserInputs criteria) {
    List<Element> acceptedElements = new ArrayList<>();
    for (Element e : allElements) {
        if (isAccepted(e, criteria)) {
            acceptedElements.add(e);
        }
    }
    return acceptedElements;
}

private boolean isAccepted(Element e, UserInputs criteria) {
    String firstName = criteria.getFirstName();
    if (isCriterionFilled(firstName) && !hasFirstName(e, firstName)) {
        return false;
    }

    String lastName = criteria.getLastName();
    if (isCriterionFilled(lastName) && !hasLastName(e, lastName)) {
        return false;
    }

    String phone = criteria.getPhone();
    if (isCriterionFilled(phone) && !hasPhone(e, phone)) {
        return false;
    }

    return true;
}
JB Nizet
  • 678,734
  • 91
  • 1,224
  • 1,255
  • `String[] criteriaArr = { criteria.getFirstName(), criteria.getLastName(); criteria.getPhone(); }` – djechlin Jul 06 '14 at 21:27
  • Sorry guys for not putting the question in right way .The whole point of my question was to follow different approach than if-else.Please dont worry about what the method returns.Do I need to check for all permutations and combinations for number of input fields in this situation using if else statement.Is there any better approach. Object Oriented may be? – user3810342 Jul 06 '14 at 21:41
  • @user3810342: have you read my answer? Don't you understand that it precisely avoids checking all permutations? – JB Nizet Jul 06 '14 at 21:44
  • @JBNizet what if the input fields are null. I have to consider null values too when I query. Does your method satisfy these? If yes I think I got the answer. Example if all values are empty or null I have to return entire result set of the table. – user3810342 Jul 06 '14 at 21:49
  • That's what the `isCriterionFilled(String criterion)` method checks. It returns true if the criterion has been filled by the user, and false otherwise. – JB Nizet Jul 06 '14 at 21:54
  • @JBNizet I tested practically its working. Thank you very much. – user3810342 Jul 06 '14 at 22:00