Modify the SlowLoadableComponent class to be open for extension

[elgatov]

🚀 Feature Proposal

Modify the SlowLoadableComponent class to be open for extension, following the Open-closed principle:

In object-oriented programming, the open–closed principle states "software entities (classes, modules, functions, etc.) should be open for extension, but closed for modification";[1] that is, such an entity can allow its behaviour to be extended without modifying its source code.

Specifically:

At the time Meyer was writing, adding fields or functions to a library inevitably required changes to any programs depending on that library.[citation needed] Meyer's proposed solution to this dilemma relied on the notion of object-oriented inheritance (specifically implementation inheritance):
A class is closed, since it may be compiled, stored in a library, baselined, and used by client classes. But it is also open, since any new class may use it as parent, adding new features. When a descendant class is defined, there is no need to change the original or to disturb its clients.[4]

Motivation

Right now SlowLoadableComponent allows a derived class to inherit from it and override how the Load method works:

selenium/dotnet/src/support/UI/SlowLoadableComponent{T}.cs

Lines 72 to 110 in bf2fc56

	/// <summary>
	/// Ensures that the component is currently loaded.
	/// </summary>
	/// <returns>The loaded component.</returns>
	/// <remarks>This is equivalent to the Get() method in Java version.</remarks>
	public override T Load()
	{
	if (this.IsLoaded)
	{
	return (T)this;
	}
	else
	{
	this.TryLoad();
	}
	DateTime end = this.clock.LaterBy(this.timeout);
	while (this.clock.IsNowBefore(end))
	{
	if (this.IsLoaded)
	{
	return (T)this;
	}
	this.HandleErrors();
	this.Wait();
	}
	if (this.IsLoaded)
	{
	return (T)this;
	}
	else
	{
	string timeoutMessage = string.Format(CultureInfo.InvariantCulture, "Timed out after {0} seconds.", this.timeout.TotalSeconds);
	throw new WebDriverTimeoutException(timeoutMessage);
	}
	}

It also lets derived classes access to SleepInterval:

selenium/dotnet/src/support/UI/SlowLoadableComponent{T}.cs

Lines 63 to 70 in bf2fc56

	/// <summary>
	/// Gets or sets the time to sleep between each check of the load status of the component.
	/// </summary>
	public TimeSpan SleepInterval
	{
	get { return this.sleepInterval; }
	set { this.sleepInterval = value; }
	}

And IsLoaded:

selenium/dotnet/src/support/UI/LoadableComponent{T}.cs

Lines 51 to 74 in bf2fc56

	/// <summary>
	/// Gets a value indicating whether the component is fully loaded.
	/// </summary>
	/// <remarks>
	/// When the component is loaded, this property will return true or false depending on
	/// the execution of <see cref="EvaluateLoadedStatus"/> to indicate the not loaded state.
	/// </remarks>
	protected bool IsLoaded
	{
	get
	{
	bool isLoaded = false;
	try
	{
	isLoaded = this.EvaluateLoadedStatus();
	}
	catch (WebDriverException)
	{
	return false;
	}
	return isLoaded;
	}
	}

It does not, however, let us access clock and timeout fields, and the Wait method:

selenium/dotnet/src/support/UI/SlowLoadableComponent{T}.cs

Lines 39 to 40 in bf2fc56

	private readonly IClock clock;
	private readonly TimeSpan timeout;

selenium/dotnet/src/support/UI/SlowLoadableComponent{T}.cs

Lines 124 to 127 in bf2fc56

	private void Wait()
	{
	Thread.Sleep(this.sleepInterval);
	}

Which stops us from changing how the Load method works and still be able to wait for the IsLoaded condition to be true.

I propose clock, timeout and Wait become protected.

Example

Let's imagine a component that we now for a fact will only load when the user executes an action (for example, clicking on a button). and we know the component will load slowly. We could skip the first evaluation of IsLoaded:

public override T Load()
{
     /*if (this.IsLoaded) 
     //{ 
     //    return (T)this; 
     //} 
     //else 
     //{ 
     //    this.TryLoad(); 
     //} 
     // we skip this party and go straight to trying to loading the component
     */

    this.TryLoad();

    DateTime end = this.clock.LaterBy(this.timeout);

    while (this.clock.IsNowBefore(end))
    {
        if (this.IsLoaded)
        {
            return (T)this;
        }

        this.HandleErrors();
        this.Wait();
    }

    if (this.IsLoaded)
    {
        return (T)this;
    }
    else
    {
        string timeoutMessage = string.Format(CultureInfo.InvariantCulture, "Timed out after {0} seconds.", this.timeout.TotalSeconds);
        throw new WebDriverTimeoutException(timeoutMessage);
        }
    }

This implementation will not work because we do not have access to clock, timeout and Wait

The same can be achieved by saving a local copy of the fields on a derived class and recreating the Wait method:

        private readonly TimeSpan timeout;
        private readonly IClock clock;

        protected CustomSlowLoadableComponent(TimeSpan timeout) : this(timeout, new SystemClock())
        {
        }

        protected CustomSlowLoadableComponent(TimeSpan timeout, IClock clock) : base(timeout, clock)
        {
            this.timeout = timeout;
            this.clock = clock;
        }

        ...

       private void Wait()
        {
            Thread.Sleep(SleepInterval);
        }

But then again, we would be adding "unnecesary" code.

Thank you.

[jimevans]

Fixed in 5632da3

[github-actions]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.