C # Refactoring two almost identical methods

Question

Refactoring is good, but sometimes it’s not so easy to figure out how to refactor and whether something really can be reorganized!

I have several methods that are almost identical - I can reorganize them, but one part of the refactoring goes beyond my logic.

Here are two non-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 such that previously non-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, we have an almost completely refactored method - with one problem, How can I determine which form I want to create from the frmName variable?

Answer 1

You can make a general method and use new() general 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 him

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

One of the limitations is that your form must have an open constructor without parameters. If you have a Form with a parameterized constructor, you can pass Func<TForm> to your method, which acts like a factory method.

Answer 2

Pass the factory method, for example:

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

The only requirement is that both classes you create must have some common interface or base class.

Answer 3

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

In your example, you mix high-level functions with low-level functions (changing false to true, creating objects, etc.), the function itself is not described independently.

Thus, there are many possibilities for 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); }