2
\$\begingroup\$

I'm writing Selenium code in Java. And below is one of the methods.

private static void getPaceNumber(WebDriver chromeDriver, String dBName, XSSFSheet paceSheet, String pubName, int i,
        XSSFCell cell, XSSFWorkbook workbook) throws Exception {
    System.out.println("DBID is " + dBName + " and fpn is " + pubName);
    CellStyle style = workbook.createCellStyle();
    int defaultHeight = paceSheet.getRow(0).getHeight();
    cell = paceSheet.getRow(i).createCell(1);
    paceSheet.getRow(i).setHeight((short) (defaultHeight * 2));
    if (dBName == "" || dBName.equals("null")) {
        System.out.println("Null Block");
        cell.setCellValue("N/A");
    } else {
        chromeDriver.findElement(By.xpath("html/body/form[2]/b/b/table/tbody/tr[2]/td[2]/textarea"))
                .sendKeys("\"" + dBName + "\"");
        chromeDriver.findElement(By.xpath("html/body/form[2]/b/b/table/tbody/tr[4]/td[2]/input[1]")).click();
        // Thread.sleep(500L);
        System.out.println("entered second block");
        List<WebElement> pace = chromeDriver
                .findElements(By.xpath("html/body/form[2]/table[1]/tbody/tr[2]/td[2]/input[1]"));
        int paceSize = pace.size();
        System.out.println("pace size is " + paceSize);
        int pubPaceNumber = 0;
        int dbPaceNumber;
        if (paceSize >= 1) {
            dbPaceNumber = Integer.parseInt(
                    chromeDriver.findElement(By.xpath("html/body/form[2]/table[1]/tbody/tr[2]/td[2]/input[1]"))
                            .getAttribute("value"));
            chromeDriver.findElement(By.xpath(".//*[@id='searchPublication']")).click();
            chromeDriver.findElement(By.xpath("html/body/form[2]/b/b/table/tbody/tr[2]/td[2]/textarea"))
                    .sendKeys("\"" + pubName + "\"");
            chromeDriver.findElement(By.xpath("html/body/form[2]/b/b/table/tbody/tr[4]/td[2]/input[1]")).click();

            int paceSizse = chromeDriver
                    .findElements(By.xpath("html/body/form[2]/table[1]/tbody/tr[2]/td[2]/input[1]")).size();
            if (paceSizse >= 1) {
                pubPaceNumber = Integer.parseInt(
                        chromeDriver.findElement(By.xpath("html/body/form[2]/table[1]/tbody/tr[2]/td[2]/input[1]"))
                                .getAttribute("value"));
            } else {
                List<WebElement> table = chromeDriver
                        .findElements(By.xpath("html/body/form[2]/table[1]/tbody/tr[4]/td/b"));
                int tabSize = table.size();
                System.out.println("Tab size is " + tabSize);
                if (tabSize == 1) {
                    chromeDriver.findElement(By.xpath(".//*[@id='searchPublication']")).click();
                    chromeDriver.findElement(By.xpath("html/body/form[2]/b/b/table/tbody/tr[2]/td[2]/textarea"))
                            .sendKeys("\"" + pubName + "\"");
                    chromeDriver.findElement(By.xpath("html/body/form[2]/b/b/table/tbody/tr[4]/td[2]/input[1]"))
                            .click();
                    List<WebElement> paceWithFPN = chromeDriver
                            .findElements(By.xpath("html/body/form[2]/table[1]/tbody/tr[2]/td[2]/input[1]"));
                    int paceWithFPNSize = paceWithFPN.size();
                    if (paceWithFPNSize >= 1) {

                        cell.setCellValue("N/A");
                    } else {
                        cell.setCellValue("N/A");
                    }

                } else {
                    cell.setCellValue("N/A");
                }

            }

            if (dbPaceNumber == pubPaceNumber) {
                cell.setCellValue(dbPaceNumber);
            } else {

                cell.setCellValue(dbPaceNumber + "\n" + pubPaceNumber);
                style.setWrapText(true);
                style.setAlignment(CellStyle.ALIGN_RIGHT);
                cell.setCellStyle(style);
            }

        } else {
            List<WebElement> table = chromeDriver
                    .findElements(By.xpath("html/body/form[2]/table[1]/tbody/tr[4]/td/b"));
            int tabSize = table.size();
            System.out.println("Tab size is " + tabSize);
            if (tabSize == 1) {
                chromeDriver.findElement(By.xpath(".//*[@id='searchPublication']")).click();
                chromeDriver.findElement(By.xpath("html/body/form[2]/b/b/table/tbody/tr[2]/td[2]/textarea"))
                        .sendKeys("\"" + pubName + "\"");
                chromeDriver.findElement(By.xpath("html/body/form[2]/b/b/table/tbody/tr[4]/td[2]/input[1]"))
                        .click();
                List<WebElement> paceWithFPN = chromeDriver
                        .findElements(By.xpath("html/body/form[2]/table[1]/tbody/tr[2]/td[2]/input[1]"));
                int paceWithFPNSize = paceWithFPN.size();
                if (paceWithFPNSize >= 1) {
                    int paceSubNumber = Integer.parseInt(chromeDriver
                            .findElement(By.xpath("html/body/form[2]/table[1]/tbody/tr[2]/td[2]/input[1]"))
                            .getAttribute("value"));

                    cell.setCellType(Cell.CELL_TYPE_NUMERIC);
                    cell.setCellValue(paceSubNumber);
                } else {
                    cell.setCellValue("N/A");
                }

            } else {
                cell.setCellValue("N/A");
            }

        }

    }
    chromeDriver.findElement(By.xpath(".//*[@id='searchPublication']")).click();
}

Basically there are 2 values used in this program: dBName and pubName. Here what the program does.

  1. enter dBName in a text area and get the result and store it in variable dbPaceNumber.
  2. hit on search and again do the same but this time enter pubName and store the value in variable pubPaceNumber
  3. Compare these two variables and see if they are same, if so, enter the result in Excel Cell else concatenate these two values and store it in Excel Cell.

What needs work

The below block of code is repeated once for pubName and once for dBName. I want to know if I can make a common block for both cases and reuse it. Is there a way that I can make a single block to check once for pubName and once for dBName?

List<WebElement> table = chromeDriver
        .findElements(By.xpath("html/body/form[2]/table[1]/tbody/tr[4]/td/b"));
int tabSize = table.size();
System.out.println("Tab size is " + tabSize);
if (tabSize == 1) {
    chromeDriver.findElement(By.xpath(".//*[@id='searchPublication']")).click();
    chromeDriver.findElement(By.xpath("html/body/form[2]/b/b/table/tbody/tr[2]/td[2]/textarea"))
            .sendKeys("\"" + pubName + "\"");
    chromeDriver.findElement(By.xpath("html/body/form[2]/b/b/table/tbody/tr[4]/td[2]/input[1]"))
            .click();
    List<WebElement> paceWithFPN = chromeDriver
            .findElements(By.xpath("html/body/form[2]/table[1]/tbody/tr[2]/td[2]/input[1]"));
    int paceWithFPNSize = paceWithFPN.size();
    if (paceWithFPNSize >= 1) {
        int paceSubNumber = Integer.parseInt(chromeDriver
                .findElement(By.xpath("html/body/form[2]/table[1]/tbody/tr[2]/td[2]/input[1]"))
                .getAttribute("value"));

        cell.setCellType(Cell.CELL_TYPE_NUMERIC);
        cell.setCellValue(paceSubNumber);
    } else {
        cell.setCellValue("N/A");
    }

} else {
    cell.setCellValue("N/A");
}
\$\endgroup\$
1
  • \$\begingroup\$ I assume that the two blocks are actually mostly identical? The first one has `if (paceWithFPNSize >= 1) { cell.setCellValue("N/A"); } else { cell.setCellValue("N/A"); }. Presumably that's a copy error? Or should the if check go away? \$\endgroup\$
    – Eric Stein
    Nov 10, 2015 at 14:56

1 Answer 1

3
\$\begingroup\$

Bug

You talk about duplication of a block of code, but that code is not a duplicate in the source you posted. It's unclear if that's a bug or a posting error..

Design

Use methods! Even short, 1-line methods will make it much easier to read and understand what's going on. In particular, duplicated code should usually be extracted for clarity and maintenance sanity. Good method names can help document the intent of method calls. If the block of code you're talking about is supposed to be identical, you can extract it into a method whose name documents what that block of code does.

General

Don't abbreviate. Is "dBName" supposed to be "databaseName"? Nobody unfamiliar with your code will be able to tell what it is supposed to stand for.

For non-trivial applications, use a logger.

You pass in a non-final Cell parameter that is totally ignored. Don't ask for a cell if you're just going to overwrite it locally. You do know that the caller of this method isn't getting back the Cell you're working with inside the method, right?

i is a terrible name for a method argument. How about rowNumber?

Don't throw Exception. Throw the most specific exception(s) you can. If you've got to throw more than 2, create an abstraction-level exception (PaceException, maybe?), catch the other exceptions, and rethrow a PaceException.

It would be nice if there was an instance doing this work, instead of it happening in a static method. Then you wouldn't have to pass chromeDriver around all over the place.

Sample Refactoring

I didn't mess with the order of any of the calls because I wasn't sure where it was important. Some code can be further consolidated if there aren't any order dependencies.

import java.util.List;

public final class Snippet {

    private static final String FORM_PATH = "html/body/form[2]/";
    private static final String PACE_PATH = "table[1]/tbody/tr[2]/td[2]/input[1]";
    private static final String TAB_PATH = "table[1]/tbody/tr[4]/td/b";

    private static void getPaceNumber(
            final WebDriver chromeDriver,
            final String dbName,
            final XSSFSheet paceSheet,
            final String pubName,
            final int rowNumber,
            final XSSFWorkbook workbook) {

        System.out.println("DBID is " + dbName + " and fpn is " + pubName);
        final XSSFCell cell = paceSheet.getRow(rowNumber).createCell(1);
        final int defaultHeight = paceSheet.getRow(0).getHeight();
        paceSheet.getRow(rowNumber).setHeight((short) (defaultHeight * 2));

        if (dbName == "" || dbName.equals("null")) {
            System.out.println("Null Block");
            cell.setCellValue("N/A");
            searchPublication(chromeDriver);
            return;
        }

        sendAndClick(chromeDriver, dbName);

        int pubPaceNumber = 0;
        int dbPaceNumber;

        if (hasPaceElements(chromeDriver)) {
            dbPaceNumber = findPaceValue(chromeDriver);
            searchPublication(chromeDriver);
            sendAndClick(chromeDriver, pubName);

            if (hasPaceElements(chromeDriver)) {
                pubPaceNumber = findPaceValue(chromeDriver);
            } else {
                if (hasTabElement(chromeDriver)) {
                    searchPublication(chromeDriver);
                    sendAndClick(chromeDriver, pubName);
                }
                cell.setCellValue("N/A");
            }

            if (dbPaceNumber == pubPaceNumber) {
                cell.setCellValue(dbPaceNumber);
            } else {
                final CellStyle style = workbook.createCellStyle();
                style.setWrapText(true);
                style.setAlignment(CellStyle.ALIGN_RIGHT);
                cell.setCellStyle(style);
                cell.setCellValue(dbPaceNumber + "\n" + pubPaceNumber);
            }

        } else {
            if (hasTabElement(chromeDriver)) {
                searchPublication(chromeDriver);
                sendAndClick(chromeDriver, pubName);
                if (hasPaceElements(chromeDriver)) {
                    cell.setCellType(Cell.CELL_TYPE_NUMERIC);
                    cell.setCellValue(findPaceValue(chromeDriver));
                } else {
                    cell.setCellValue("N/A");
                }
            } else {
                cell.setCellValue("N/A");
            }
        }
        searchPublication(chromeDriver);
    }

    private static void searchPublication(final WebDriver chromeDriver) {
        chromeDriver.findElement(By.xpath(".//*[@id='searchPublication']")).click();
    }

    private static WebElement findFormElement(final WebDriver chromeDriver, final String path) {
        return chromeDriver.findElement(By.xpath(FORM_PATH + path));
    }

    private static List<WebElement> findFormElements(final WebDriver chromeDriver, final String path) {
        return chromeDriver.findElements(By.xpath(FORM_PATH + path));
    }

    private static boolean hasPaceElements(final WebDriver chromeDriver) {
        return !findFormElements(chromeDriver, PACE_PATH).isEmpty();
    }

    private static int findPaceValue(final WebDriver chromeDriver) {
        final WebElement element = findFormElement(chromeDriver, PACE_PATH);
        return Integer.parseInt(element.getAttribute("value"));
    }

    private static boolean hasTabElement(final WebDriver chromeDriver) {
        return findFormElements(chromeDriver, TAB_PATH).size() == 1;
    }

    private static void sendAndClick(final WebDriver chromeDriver, final String key) {
        findFormElement(chromeDriver, "b/b/table/tbody/tr[2]/td[2]/textarea").sendKeys("\"" + key + "\"");
        findFormElement(chromeDriver, "b/b/table/tbody/tr[4]/td[2]/input[1]").click();
    }
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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