0
\$\begingroup\$

I, also, need to know whether this is a valid implementation of the factory pattern.

The amount of effort which goes into configuring DriverFactory to simply get a WebDriver instance seems disproportionate to the point where I ask: "is there not a better way?"

The main class code:

package dur.bounceme.net.SeleniumBase;

import java.io.IOException;
import java.util.Properties;
import java.util.logging.Logger;
import org.openqa.selenium.WebDriver;

public class App {

    private static final Logger LOG = Logger.getLogger(App.class.getName());
    private Properties properties = new Properties();

    public static void main(String[] args) throws IOException {
        new App().initSelenium();
    }

    private void initSelenium() throws IOException {
        properties.loadFromXML(App.class.getResourceAsStream("/selenium.xml"));
        WebDriver webDriver = DriverFactory.getWebDriver(properties);  //is this factory and usage correct?
        HomePage homePage = new HomePage(webDriver);
        homePage.populateCatalogue();
    }
}

And my factory:

package dur.bounceme.net.SeleniumBase;

import java.net.MalformedURLException;
import java.net.URL;
import java.util.ArrayList;
import java.util.List;
import java.util.Properties;
import java.util.logging.Logger;
import org.openqa.selenium.WebDriver;
import org.openqa.selenium.firefox.FirefoxBinary;
import org.openqa.selenium.firefox.FirefoxDriver;
import org.openqa.selenium.firefox.FirefoxOptions;

public class DriverFactory {

    private static final Logger LOG = Logger.getLogger(DriverFactory.class.getName());

    private Properties properties = new Properties();
    private FirefoxBinary firefoxBinary = null;
    private FirefoxOptions firefoxOptions = null;
    private WebDriver webDriver = null;
    private URL url = null;
    private String driver = null;
    private String gecko = null;
    private final List<String> commandLineOptions = new ArrayList<>();

    private DriverFactory() {
    }

    private DriverFactory(Properties properties) throws MalformedURLException {
        this.properties = properties;
        loadProps();
        init();
    }

     static WebDriver getWebDriver(Properties properties) throws MalformedURLException {
        DriverFactory driverFactory = new DriverFactory(properties);
        return driverFactory.webDriver;
    }

    private void loadProps() throws MalformedURLException {
        LOG.fine(properties.toString());
        url = new URL(properties.getProperty("url"));
        driver = properties.getProperty("driver");
        gecko = properties.getProperty("gecko");
        String commandLineOption = properties.getProperty("option01");
        commandLineOptions.add(commandLineOption);
    }

    private void init() throws MalformedURLException {
        firefoxBinary = new FirefoxBinary();
        commandLineOptions.forEach((commandLineOption) -> {
            LOG.fine(commandLineOption.toString());
            firefoxBinary.addCommandLineOptions(commandLineOption);
        });
        System.setProperty(driver, gecko);
        firefoxOptions = new FirefoxOptions();
        firefoxOptions.setBinary(firefoxBinary);
        webDriver = new FirefoxDriver(firefoxOptions);
        webDriver.get(url.toString());
        LOG.fine(webDriver.getTitle());
        LOG.fine(webDriver.getCurrentUrl().toLowerCase());
    }

    void close() {
        webDriver.close();
    }

}

There seems no nice way to get around loading configuration from a file, which is then loaded. But, it strikes me that I'm re-inventing the wheel of loading a props file for the purpose of configuring a Selenium WebDriver when I expect there's a tried-and-true approach.

Or, at least, more re-usable.

Although, what possible use there would be for multiple WebDriver instances is beyond me. It would seem Selenium Grid would handle that question quite differently.

\$\endgroup\$
  • 1
    \$\begingroup\$ Looks like this factory will only create drives for FireFox. What about other browsers such as Chrome or IE? The factory should create more than one item of the same kind. \$\endgroup\$ – Ankit Soni Dec 23 '18 at 18:35
1
\$\begingroup\$

Quick Simplifications

  • The 0-args constructor is not used. Remove it.
  • close() can not be called in a meaningful way. Nobody can obtain an instance of DriverFactory, so making that an instance method is not useful in any way.
  • You're misusing fields as variables. Everything you're doing can be put into the factory method itself. There's no need for any instance data, you're not accessing it anywhere.

Let's consider instead:

static WebDriver getWebDriver(Properties properties) throws MalformedURLException {
    LOG.fine(properties.toString);
    final URL url = new URL(properties.getProperty("url"));
    final String driver = properties.getProperty("driver");
    final String gecko = properties.getProperty("gecko");
    final List<String> commandLineOptions = new ArrayList<>();
    commandLineOptions.add(properties.getProperty("option01"));

    final FirefoxBinary ffBinary = new FirefoxBinary();
    commandLineOptions.forEach((commandLineOption) -> {
        LOG.fine(commandLineOption);
        ffBinary.addCommandLineOptions(commandLineOption);
    });
    System.setProperty(driver, gecko);

    final FirefoxOptions ffOptions = new FirefoxOptions();
    ffOptions.setBinary(ffBinary);

    WebDriver result = new FirefoxDriver(ffOptions);
    result.get(url.toString()); 
    LOG.fine(webDriver.getTitle());
    LOG.fine(webDriver.getCurrentUrl().toLowerCase());
    return result;
}

This does mostly the same as the code you presented. We can see here that opening yourself up to MalformedURLException is not necessary. You're reading the String from properties into a URL and then call .toString() on it.

What we can see pretty easily now is that you're reading the contents of the passed Properties into variables to just use them once. It might be easier to reimagine the code as something like the following:

public static WebDriver getWebDriver(Properties properties) {
    final FirefoxBinary ffBinary = discoverBinary(properties);
    setSystemProperties(properties);
    final FirefoxOptions ffOptions = buildOptions(ffBinary);
    return createWebDriver(ffOptions, properties);
}

This makes the code comparatively abstract and easy to follow. You can grasp what happens without needing a deeper understanding of Selenium itself (not that the original code required a lot of it).

This abstraction just makes it easier to understand what's going on. The methods themselves are pretty trivial :)

\$\endgroup\$
  • \$\begingroup\$ My thinking was that this method could end up many lines long so to put as much as possible into other classes. No? \$\endgroup\$ – Thufir Dec 23 '18 at 19:08
  • 1
    \$\begingroup\$ But that needs to provide abstraction. Don't just hide away lines, that's only moving the problem. Hide complexity \$\endgroup\$ – Vogel612 Dec 23 '18 at 19:09

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.