c# Refactoring two nearly identical Methods

Question

Refactoring is good, but sometimes it is not so easy to work out how to refactor and indeed whether something can actually be refactored!

I have a number of methods which are almost identical - I can refactor them but one part of the refactoring is beyond my logic.

Here are two un-refactored methods:

 private void projectToolStripMenuItem_Click(object sender, EventArgs e)
    {
        if (projectToolStripMenuItem.Checked)
        {
            projectToolStripMenuItem.Checked = false;
            if (!projectForm.IsDisposed) projectForm.Hide();
        }
        else
        {
            if (projectForm.IsDisposed)
                projectForm = new frmProject();
            projectForm.Show(dockPanel, DockState.DockRight);
            projectToolStripMenuItem.Checked = true;
        }

    }

    private void logginToolStripMenuItem_Click(object sender, EventArgs e)
    {
        if (logginToolStripMenuItem.Checked)
        {
            logginToolStripMenuItem.Checked = false;
            if (!outputForm.IsDisposed) outputForm.Hide();
        }
        else
        {
            if (outputForm.IsDisposed)
                outputForm = new frmOutput();
            outputForm.Show(dockPanel, DockState.DockBottom);
            logginToolStripMenuItem.Checked = true;
        }
    }

With Refactoring I would get a method like this that the previously un-refactored methods would call

private void refactoredMethod(TooStripMenuItem menuItem, DockContent frmName)
{

        if (menuItem.Checked)
        {
            menuItem.Checked = false;
            if (!frmName.IsDisposed) frmName.Hide();
        }
        else
        {
            if (frmName.IsDisposed)
                frmName= new frmProject(); // Still Problematic
            frmName.Show(dockPanel, DockState.DockRight);
            menuItem.Checked = true;
        }
    }

So what we have an almost completely refactored method - with one problem, How can I tell which form I wish to instantiate from the frmName variable?

Answer 1

You could make the method generic and take advantage of new() generic constraint.

private TForm refactoredMethod<TForm>(TooStripMenuItem menuItem, TForm frmName) where TForm : Form, new()
{
    if (menuItem.Checked)
    {
        menuItem.Checked = false;
        if (!frmName.IsDisposed) frmName.Hide();
    }
    else
    {
        if (frmName.IsDisposed)
            frmName= new TForm();
        frmName.Show(dockPanel, DockState.DockRight);
        menuItem.Checked = true;
    }
    return frmName;
}

So you could call it as

projectForm = refactoredMethod<frmProject>(projectToolStripMenuItem, projectForm);

One limitation is that your form should have a public parameterless constructor. If you have a Form with parameterized constructor, you could pass Func<TForm> to your method which acts as a factory method.

Answer 2

Pass a factory to the method, like this:

private void RefactoredMethod(..., Func<TypeOfItemToCreate> creator)
{
    ...
    if (frmName.IsDisposed)
            frmName = creator(); 
}

The only requirement is that both of the classes you create need to have some common interface or base class.

Answer 3

I see there is already answer yet want to write something more. I believe that refactoring is much more than you describe here. One thing is changing names for function, one is putting code into separate function. Also remember there is no proper refactoring without proper unit tests.

In your example you are mixing high level functions with low level functions (changing false to true, creating objects etc,), also function itself is not self described.

So there is plenty to do in terms of refactoring:

void hideForm(TForm form){
    if(!form.IsDisposed){
        form.Hide();
    }
}

void showFormInDockPanel<TForm>(TForm form, DockPanel dockPanel){
    if(form.IsDisposed){
        form = new TForm();
    }
    form.Show(dockPanel, DockState.DockRight);
}

void toggleFormAndMenuItem<TForm>(TForm form, TooStripMenuItem menuItem){
    if(menuItem.checked){
        hideForm(form);
    }
    else {
        showFormInDockPanel<TForm>(form, dockPanel);
    }

    menuItem.checked = !menuItem.checked;
}

private void projectToolStripMenuItem_Click(object sender, EventArgs e){
    toggleFormAndMenuItem<frmProject>(projectToolStripMenuItem, projectForm);
}