1
\$\begingroup\$

I am trying to design some classes which I intent to use as a framework. I would like them to be compliant to SOLID design principles.

I had a basic structure and then I implemented Strategy design pattern and tried adding dependency injection as well.

Would like this to be reviewed so that I know if my understanding is correct or can still be made better.

public interface IBrowser
{
    IWebDriver GetWebDriver(string webDriverFolderPath);
}

public class EdgeBrowser : IBrowser
{
    public IWebDriver GetWebDriver(string webDriverFolderPath)
    {
        var options = new EdgeOptions()
        {
            PageLoadStrategy = PageLoadStrategy.Eager,
            UseInPrivateBrowsing = true
        };

        return new EdgeDriver(webDriverFolderPath, options, TimeSpan.FromSeconds(60));
    }
}

public class IEBrowser : IBrowser
{
    public IWebDriver GetWebDriver(string webDriverFolderPath)
    {
        var options = new InternetExplorerOptions()
        {
            IntroduceInstabilityByIgnoringProtectedModeSettings = true
        };

        return new InternetExplorerDriver(webDriverFolderPath, options, TimeSpan.FromSeconds(60));
    }
}

public interface IConfiguration
{
    string GetConfiguration(string key);
}

public class AppSettingsConfiguration : IConfiguration
{
    public string GetConfiguration(string key)
    {
        return ConfigurationManager.AppSettings[key].ToString();
    }
}

public class WebDriver
{
    private static Dictionary<Enums.Browsers, IBrowser> Browsers = new Dictionary<Enums.Browsers, IBrowser>();
    private static Dictionary<string, Enums.Browsers> BrowserMapper = new Dictionary<string, Enums.Browsers>();
    private static AppSettingsConfiguration _AppSettingsConfiguration;

    static WebDriver()
    {
        Browsers.Add(Enums.Browsers.Edge, new EdgeBrowser());
        Browsers.Add(Enums.Browsers.IE, new IEBrowser());
        Browsers.Add(Enums.Browsers.Chrome, new ChromeBrowser());
        Browsers.Add(Enums.Browsers.Firefox, new FirefoxBrowser());

        BrowserMapper.Add("Edge", Enums.Browsers.Edge);
        BrowserMapper.Add("IE", Enums.Browsers.IE);
        BrowserMapper.Add("Internet Explorer", Enums.Browsers.IE);
        BrowserMapper.Add("Chrome", Enums.Browsers.Chrome);
        BrowserMapper.Add("Firefox", Enums.Browsers.Firefox);
        BrowserMapper.Add("edge", Enums.Browsers.Edge);
        BrowserMapper.Add("ie", Enums.Browsers.IE);
        BrowserMapper.Add("internet explorer", Enums.Browsers.IE);
        BrowserMapper.Add("chrome", Enums.Browsers.Chrome);
        BrowserMapper.Add("firefox", Enums.Browsers.Firefox);
        BrowserMapper.Add("internetexplorer", Enums.Browsers.IE);
        BrowserMapper.Add("InternetExplorer", Enums.Browsers.IE);
    }

    public WebDriver(AppSettingsConfiguration appSettingsConfiguration)
    {
        _AppSettingsConfiguration = appSettingsConfiguration;
    }

    public IWebDriver GetWebDriver(string browser)
    {
        var browserName = BrowserMapper[browser];

        return Browsers[browserName].GetWebDriver(_AppSettingsConfiguration.GetConfiguration("WebDriverFolderPath"));
    }
}

public void Initialize()
{
    if (WebDriver == null)
    {
        WebDriver = new WebDriver(new AppSettingsConfiguration()).GetWebDriver(BaseData.Browser);
    }
}
\$\endgroup\$
2
  • \$\begingroup\$ Perhaps GetWebDriver() could just use the Browsers enumeration to avoid having to create duplicate mappings like "internetexplorer", "InternetExplorer", and "internet explorer". \$\endgroup\$
    – Shelby115
    Jan 17, 2019 at 14:22
  • \$\begingroup\$ @Shelby115 thanks for the reply. Initialize method receives data read from an excel. So browser name can vary and I was not able to think about any other way to map user input to enums... \$\endgroup\$
    – niklodeon
    Jan 17, 2019 at 14:50

1 Answer 1

2
\$\begingroup\$

I'm not really sure that there is any good argument having these static fields, especially when it comes to unit testing the WebDriver class.

public class WebDriver
{
    private static Dictionary... // this
    private static Dictionary... // this
    private static AppSetting... // this

    static WebDriver() // this
    {
        ...
    }

    ...
}

I would suggest you change those to private readonly fields instead.

and then we have this Initialize() method hanging out on it's own -- which means your code could not compile as is.

public class Foo // where is the class?
{
    public void Initialize()
    {
        if (WebDriver == null)
        {
            WebDriver = new WebDriver(new AppSettingsConfiguration())
                .GetWebDriver(BaseData.Browser);
        }
    }
}

I highly would not recommend passing around the AppSettingsConfiguration class, when it seems like the only time you use it is to pull a string value by calling _AppSettingsConfiguration.GetConfiguration("WebDriverFolderPath").

It would make more sense for you to handle reading your configuration somewhere else and not make the WebDriver responsible for handling that... (This actually breaks your "S" in the SOLID principle).

I would probably advise to consider refactoring it along these lines...

public enum Browsers { Unknown, Ie, Chrome, Edge, FireFox }

public class WebDriverFactory
{
    private readonly string _path;

    public WebDriverFactory(string path)
    {
        if (String.IsNullOrEmpty(path)) throw new ArgumentNullException(nameof(path));
        // also, consider to a check that the path exists

        _path = path;
    }

    public IWebDriver Construct(Browsers browser)
    {
        switch (browser)
        {
            case Browser.Edge:
            {
                var options = new EdgeOptions()
                {
                    PageLoadStrategy = PageLoadStrategy.Eager,
                    UseInPrivateBrowsing = true
                };
                return new EdgeDriver(_path, options, TimeSpan.FromSeconds(60));
            }
            case.Unknown:
                throw new Exception("not a valid browser");
            default:
                throw new NotImplementedException();
        }
    }
}
\$\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.