4
\$\begingroup\$

I have click event handlers for two buttons. The code is working well, but I want to apply the DRY principle. How should I rewrite the following code in the button_click event handler so that I don't have to repeat in all my 10 buttons?

private void CreateWindows(UserControl ux, Form rpt, object sender, ItemClickEventArgs e)
{
    ux.Show();

    rpt.SuspendLayout();
    rpt.ControlBox = false;
    rpt.TopLevel = false;
    rpt.WindowState = FormWindowState.Maximized;
    rpt.MdiParent = this;
    rpt.Show();
    rpt.ResumeLayout();

    dock1.SuspendLayout();
    dock1.Controls.Clear();
    dock1.Controls.Add(ux);
    dock1.ResumeLayout();
}

private BaseDocument FindDocument<T>() where T : Form
{
    foreach (BaseDocument document in dxDocumentManager.View.Documents) {
        if (document.Form is T) {
            return document;
        }
    }
    return null;
}

private void button1_ItemClick(object sender, ItemClickEventArgs e)
{
    dxRpt12MonthsItemPurchaseAnalysisByAmount filter = new formFilter1();

    BaseDocument document = FindDocument<formReport1>();

    if(document != null)
    {
        tabbedView1.Controller.Activate(document);
        if (this.ActiveMdiChild is formReport1)
        {
            filter.Focus();
        }
    }
    else
    {
        CreateWindows(new formFilter1(), new formReport1(), sender, e);
    }
}

private void button2_ItemClick(object sender, ItemClickEventArgs e)
{
    fmSalesInvoiceSearch filter = new formFilter2();

    BaseDocument document = FindDocument<formReport2>();

    if (document != null)
    {
        tabbedView1.Controller.Activate(document);
        if (this.ActiveMdiChild is formReport2)
        {
            filter.Focus();
        }
    }
    else
    {
        CreateWindows(new formFilter2(), new formReport2(), sender, e);
    }
}
\$\endgroup\$
  • 2
    \$\begingroup\$ You mention ten buttons, but you've only posted code for two. There may be too much guesswork involved in answering. \$\endgroup\$ – 200_success Nov 17 '14 at 10:59
  • \$\begingroup\$ You are mixing and matching the style of your braces: re. Your FindDocument method in comparison to your button handlers. \$\endgroup\$ – d347hm4n Nov 17 '14 at 11:53
  • \$\begingroup\$ Hi @200_success, sorry I couldn't find my post (this one) as that was my first time using CR as I have always been using SO. Anyway, I have posted two because the rest of the 8 are the doing the same thing except calling different WinForm or WinReport \$\endgroup\$ – furor Nov 25 '14 at 16:19
  • \$\begingroup\$ @d347hm4n Which particular line of code do you mean by different style? \$\endgroup\$ – furor Nov 25 '14 at 16:20
  • \$\begingroup\$ Compare the if statement in teh FindDocument method with the if statements in the buttton handler. You are doing eith braces on their own lines or doing a trailing brace. \$\endgroup\$ – d347hm4n Nov 25 '14 at 16:22
3
\$\begingroup\$

Naming:

Like the previous answer stated, your classnames are far from following conventions. The should be PascalCase and not camelCase. Also a name like dxRpt12MonthsItemPurchaseAnalysisByAmount is definitly not meaningful, not for you, not for others who read your code. Try names like FirstReportForm, FirstFilterForm or FilterForm1, ReportForm1 and so on.

Tip: Names of Classes, Structs, and Interfaces

Braces:

Microsoft doesn't have any conventions on the formatting of your braces. This means you can chose between following:

if (isValid) {
    DoSomething();
}

and

if (isValid)
{
    DoSomething();
}

But it's best practice to stick with one and not to mix the two up in your code. (I prefer the second use of braces.)

Logical use of variables:

In both click event handlers you instantiate a new filter. Whether or not document is null, you call CreateWindows with again a new instance of that filter-class. Just re-use the instance you created before. You didn't manipulate it in any way.

Generic method:

First of all, the following code is written out of my head and not tested as I have little information what all your classes are/mean. But here's an attempt to make it generic:

private void HandleButtonClick<T>(Form filterForm) where T : Form, new()
{
    var document = FindDocument<T>();

    if(document == null)
    {
        CreateWindows(filterForm, new T(), sender, e);
    }
    else
    {
        tabbedView1.Controller.Activate(document);
        if (this.ActiveMdiChild is T)
        {
            filterForm.Focus();
        }
    }
}

And the usage of the method:

private void button1_ItemClick(object sender, ItemClickEventArgs e)
{
    HandleButtonClick<ReportForm1>(new FilterForm1());
}

private void button2_ItemClick(object sender, ItemClickEventArgs e)
{
    HandleButtonClick<ReportForm2>(new FilterForm2());
}

Notes:

  • T is the type of the report form
  • The new() constraint is set on the method, check tip for more info
  • I reversed the null check for readability, this might be personal choice

Tip: new Constraint (C# Reference)

I cannot reproduce your code and therefore cannot test the method. Please let me know if you got it to work using this method. Hope this helps!

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Yes I am aware of the naming convention which will be refactor. The ex programmer really mixed her VB6 convention into C# making a mess. However the main thing is to know if your method work or not. I will try it when I get back to office this Thur \$\endgroup\$ – furor Nov 25 '14 at 16:22
2
\$\begingroup\$

I'm not a big fan of this "negative check":

if(document != null)
{}
else
{}

I much prefer this style:

if(document == null)
{}
else
{}

Names like formReport1, formFilter1, dxRpt12MonthsItemPurchaseAnalysisByAmount,... are bad names for classes; if possible change them to more descriptive names (and use PascalCase).

My guess is that the entire body of your button click event handlers can be abstracted into a single method, considering that CreateWindows(new formFilter2(), new formReport2(), sender, e); also seems to be pretty generic. Look at that method and check how it handles the various types of forms and filters.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ As a rule of thumb I use the more likely path in the if clause and the more unlikely in the else clause. The common code path stands on top, is read at first. But this may be an old habit from times when branch prediction was just: always true \$\endgroup\$ – aggsol Nov 17 '14 at 14:52
  • \$\begingroup\$ BCdotNET and Code Clown - yeah I am aware of the nesting if statement (I also use Resharper for their GoToImplementation navigation and it also comes with code analysis). Well, I am still in the mist of refactoring the ex programmer codes \$\endgroup\$ – furor Nov 25 '14 at 16:25
0
\$\begingroup\$

The advice I have to give assumes that formFilter1 and formFilter2 both inherit from a common base class FormFilter. If they don't, you will need to change them in order for this to work. We're going to use some polymorphism and Activator.CreateInstance() to help us extract a method to handle clicks no matter where they come from.

private void button1_ItemClick(object sender, ItemClickEventArgs e)
{
    dxRpt12MonthsItemPurchaseAnalysisByAmount filter = new formFilter1();
    BaseDocument document = FindDocument<formReport1>();

    HandleButtonClick(sender,e,filter,document);
}

private void HandleButtonClick(object sender, ItemClickEventArgs e, FormFilter filter, BaseDocument document)
{
    if (document == null)
    {
        CreateWindows(Activator.CreateInstance(typeof(document)), Activator.CreateInstance(typeof(filter)), sender, e);
    }
    else
    {
        tabbedView1.Controller.Activate(document);
        if (this.ActiveMdiChild is formReport1)
        {
            filter.Focus();
        }
    }
}

See this SO answer for more on Activator.CreateInstance() and how to create a new instance of an object without knowing it's type. Please note that this code is obviously untested.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Activator.CreateInstance() is new to me. I will check this out too. \$\endgroup\$ – furor Nov 25 '14 at 16:27

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.