CODEWITHSUNDEEP
c#staticrefactoring
Sean Anderson
Sean Anderson

Reputation: 29271

Refactoring big ball of mud; not sure static is being used properly here. Advice?

I'll admit sometimes the deeper nuances of the keyword static escape me.

Here's what I'm seeing:

public partial class Default : CSSDEIStatusBase
{
    private static Default _csWitWeb;

    protected void Page_Load(object sender, EventArgs e)
    {
        //DoStuff
        _csWitWeb = this;
        //OtherStuff
    }

    public static void ForceLoadSyncOperation(int? index)
    {
        Default._csWitWeb.LoadSelectedSyncOperation(index);
    }
}

The only references to ForceLoadSyncOperation are:

Default.ForceLoadSyncOperation(index);

or

Default.ForceLoadSyncOperation(null);

Both of these calls originate from:

    public partial class DataOriginUserControl : System.Web.UI.UserControl

and are not located inside of static methods.

E.G:

protected void btnCancelSyncOperation_Click(object sender, EventArgs e)
{
    this.lblErrorMessage.Text = string.Empty;
    this.lblErrorMessage.Visible = false;

    int index = _syncOperation.Sequence - 1;
    Default.ForceLoadSyncOperation(index);
}

This all seems really quirky to me. Does this smell to anyone else? Not really sure how to untangle it, though, as I can't exactly create an instance of the Default page inside of a user control.

Thoughts? Thanks for reading.

protected void LoadSelectedSyncOperation(int? index)
{
    SyncOperationConfiguration[] syncOperations = CSServiceClient.GetInterfaceConfiguration().SyncOperationConfigurations.ToArray();

    PopulateSyncOperationsListView(syncOperations);
    SyncOperationConfiguration syncOperation = null;

    try
    {
        syncOperation = syncOperations[index.HasValue ? index.Value : 0];
    }
    catch
    {
        syncOperation = syncOperations[0];
    }

    ucDataOrigin.LoadSyncOperationData(syncOperation);
    Session["ConfigMenuActiveIndex"] = 1;
    menuConfiguration.Items[(int)Session["ConfigMenuActiveIndex"]].Selected = true;
    mvwConfiguration.ActiveViewIndex = (int)Session["ConfigMenuActiveIndex"];
}

Upvotes: 1

Views: 133

Answers (3)

gilly3
gilly3

Reputation: 91497

Presumably, the user control is contained within the Default page and the static member is being used as a shortcut to get the current instance of Default. I would've done it this way:

Default defaultPage = this.Page as Default;
if (defaultPage != null)
{
    defaultPage.LoadSelectedSyncOperation(index);
}

Using a static member in this way is not safe. It opens up the door for race conditions. There is the potential risk that the user control is loaded in another page and calls LoadSelectedSyncOperation() on a separate request's instance of Default, thus wreaking all kinds of potential havoc.

Upvotes: 2

RoccoC5
RoccoC5

Reputation: 4213

I would definitely say your concerns are valid. I can't think of any reason that this design would make sense, ever. This would throw a flag for me, too.

Based on your reply to my comment, if the Default.LoadSelectedSyncOperation is not dependent upon the Default page somehow, then I suggest it be refactored into a separate class (not an ASP.NET Page).

Whether it makes sense for the method or new class to be static or not is a separate concern and would be based on the logic contained within the method.

Upvotes: 1

Piotr Perak
Piotr Perak

Reputation: 11088

I don't know what LoadSelectedSyncOperation does but this code looks weird. Whenever you click btnCancelSyncOperation you end up calling this method on some page, but you never know on which of them. It doesn't make much sense to me.

Upvotes: 1

Related Questions