List of search results using Selenium

Question

I have written the following code for looping through WebElements in Selenium.

Can someone please provide me with feedback on how to improve my code?

   public static void checkAbsentSearchResult(String title, String Testgoal) throws IOException{       
         try{
                List<WebElement> allSearchResults = driver
                              .findElements(By
                                           .xpath("//*[contains(@class, 'teaser-list')]/li/article/div[2]/h3"));
                //Check if xpath contains any WebElements, if not check if no results message is displayed
                if(allSearchResults == null || allSearchResults.size() < 1 ){
                       String element = driver.findElement(By.xpath("//div[@class='view-empty']")).getText();
                       if(element.contains("no results found")){
                              WriteToXSL.WriteTo(Testgoal, "OK", "");
                              WriteToPDF.addRow(Testgoal, "OK", "");
                       }else{
                              WriteToXSL.WriteTo(Testgoal, "NOK", element.toString());
                              WriteToPDF.addRow(Testgoal, "NOK", "");
                       }
                }else{
                       boolean resultsFound = false;
                       for (WebElement Element : allSearchResults) {
                              if (Element.getText().contains(title)) {
                                    WriteToXSL.WriteTo(Testgoal, "NOK", "");
                                    WriteToPDF.addRow(Testgoal, "NOK", "");
                                    resultsFound = true;
                                    break;
                              }// close if
                       }// close for loop
                       if (resultsFound == false) {
                              WriteToXSL.WriteTo(Testgoal, "OK", "");
                              WriteToPDF.addRow(Testgoal, "OK", "");
                       }
                }
         }catch(Exception e){
                WriteToXSL.WriteTo(Testgoal, "NOK", e.toString());
                WriteToPDF.addRow(Testgoal, "NOK", "");
         }
   }

Answer 1

1. Method names in Java are usually camelCase as well as variable names. So,

   • WriteToXSL.WriteTo should be WriteToXSL.writeTo,
   • WebElement Element should be WebElement element,
   • Testgoal should be testgoal.

2. WriteToXSL and WriteToPDF aren't the best class names. The Java Language Specification, Java SE 7 Edition, 6.1 Declarations says the following:

   Class and Interface Type Names

   Names of class types should be descriptive nouns or noun phrases, not overly long, in mixed case with the first letter of each word capitalized.

   I would call them XslWriter and PdfWriter. (See also #3 here.)

   Also related: Effective Java, 2nd edition, Item 56: Adhere to generally accepted naming conventions

3. element is a String here:

   WriteToXSL.WriteTo(Testgoal, "NOK", element.toString());
   

   Calling toString() on a String is not too useful, you could remove that. Actually, I'd rename the variable to emptyViewText to describe its exact content.

4. As far as I see driver.findElements() never returns null, so you could remove the null check here:

   List<WebElement> allSearchResults = driver
                 .findElements(By
                              .xpath("//*[contains(@class, 'teaser-list')]/li/article/div[2]/h3"));
   //Check if xpath contains any WebElements, if not check if no results message is displayed
   if(allSearchResults == null || allSearchResults.size() < 1 ){
   

   The allSearchResults.size() < 1 condition could be changed to allSearchResults.isEmpty().

5. Comments on the closing curly braces are unnecessary and rather disturbing. Modern IDEs could show blocks.

       }// close if
   }// close for loop
   

   [“// …” comments at end of code block after } - good or bad?][4]

6. You could avoid the flag here with a return statement:

   boolean resultsFound = false;
   for (WebElement Element: allSearchResults) {
       if (Element.getText().contains(title)) {
           WriteToXSL.WriteTo(Testgoal, "NOK", "");
           WriteToPDF.addRow(Testgoal, "NOK", "");
           resultsFound = true;
           break;
       }
   }
   if (resultsFound == false) {
       WriteToXSL.WriteTo(Testgoal, "OK", "");
       WriteToPDF.addRow(Testgoal, "OK", "");
   }
   

   Consider this:

   for (WebElement Element: allSearchResults) {
       if (Element.getText().contains(title)) {
           WriteToXSL.WriteTo(Testgoal, "NOK", "");
           WriteToPDF.addRow(Testgoal, "NOK", "");
           return;
       }
   }
   WriteToXSL.WriteTo(Testgoal, "OK", "");
   WriteToPDF.addRow(Testgoal, "OK", "");
   

7. Actually, I would restructure the code to remove the duplicated XSL/PDF writing calls. I have changed them with two exceptions:

   public static void checkAbsentSearchResult(String title, String testGoal) throws IOException {
       try {
           doCheckAbsentSearchResult(title, testGoal);
           WriteToXSL.WriteTo(testGoal, "OK", "");
           WriteToPDF.addRow(testGoal, "OK", "");
       } catch (Exception e) {
           WriteToXSL.WriteTo(testGoal, "NOK", e.toString());
           WriteToPDF.addRow(testGoal, "NOK", "");
       }
   }
   
   public static void doCheckAbsentSearchResult(String title, String testGoal) throws IOException {
       final List<WebElement> allSearchResults = driver.findElements(By
               .xpath("//*[contains(@class, 'teaser-list')]/li/article/div[2]/h3"));
       // Check if xpath contains any WebElements, if not check if no
       // results message is displayed
       if (allSearchResults.isEmpty()) {
           final String emptyViewText = driver.findElement(By.xpath("//div[@class='view-empty']")).getText();
           if (!emptyViewText.contains("no results found")) {
               throw new RuntimeException(emptyViewText);
           }
       } else {
           for (final WebElement searchResultElement: allSearchResults) {
               if (searchResultElement.getText().contains(title)) {
                   throw new RuntimeException("");
               }
           }
       }
   }
   

   Note that it changes the output a little bit when there is an error. I guess it could be fine for a report.

Answer 2

Naming

As the method is writing state messages in each case, you should consider to rename the method to exportAbsentSearchResult.

Duplicate code

WriteToXSL.WriteTo(goal, state, message);
WriteToPDF.addRow(goal, state, message);

These two methods are called within different places in your method. So extract these calls into one method.

private static void writeToGoal(String goal,String state, String message){
    WriteToXSL.WriteTo(goal, state, message);
    WriteToPDF.addRow(goal, state, "");
}

As you aren't using the value of message for the call to WriteToPDF.addRow() you can leave the empty String there.

The state as String can be extracted into an enum to simplify adding further states.

enum SearchResultState{
    OK,NOK
}

So the writeToGoalmethod will look like

private static void writeToGoal(String goal,SearchResultState state, String message){
    WriteToXSL.WriteTo(goal, state.toString(), message);
    WriteToPDF.addRow(goal, state.toString(), "");
}

Integrating the changes

Assigning default values to state and message will reduce the code in the method.

public static void exportAbsentSearchResult(String title, String testgoal) throws IOException{
     String message = "";
     SearchResultState state = SearchResultState.OK;
     try{
            List<WebElement> allSearchResults = driver
                          .findElements(By
                                       .xpath("//*[contains(@class, 'teaser-list')]/li/article/div[2]/h3"));
            //Check if xpath contains any WebElements, if not check if no results message is displayed
            if(allSearchResults == null || allSearchResults.size() < 1 ){
                   String element = driver.findElement(By.xpath("//div[@class='view-empty']")).getText();
                   // as the initial state and message reflects the original condition              
                  //if(element.contains("no results found")) i inverted the condition
                   if(!element.contains("no results found")){ 
                          state = SearchResultState.NOK;
                          message = element;
                   }
            }else{
                   for (WebElement Element : allSearchResults) {
                          if (Element.getText().contains(title)) {
                                state = SearchResultState.NOK;
                                break;
                          }// close if
                   }// close for loop
            }
     }catch(Exception e){
            state = SearchResultState.NOK;
            message = e.toString();
     }
     writeToGoal(testgoal, state, message);
}