HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Click handlers for two to ten buttons

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
clickhandlerstenbuttonstwofor

Problem

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() 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();

    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();

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

Solution

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(Form filterForm) where T : Form, new()
{
    var document = FindDocument();

    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(new FilterForm1());
}

private void button2_ItemClick(object sender, ItemClickEventArgs e)
{
    HandleButtonClick(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!

Code Snippets

if (isValid) {
    DoSomething();
}
if (isValid)
{
    DoSomething();
}
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();
        }
    }
}
private void button1_ItemClick(object sender, ItemClickEventArgs e)
{
    HandleButtonClick<ReportForm1>(new FilterForm1());
}

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

Context

StackExchange Code Review Q#70077, answer score: 3

Revisions (0)

No revisions yet.