Solved

Removing Duplicate Code

Posted on 2015-02-14
6
161 Views
Last Modified: 2015-02-15
Greetings,

I am trying to clean up some code and learn c# as I am going. I am looking for a way to eliminate a duplicate code warning message I receive from devexpress coderush.

I have 5 buttons that the user can click on.  An example of only two of them are shown here for brevity.
The subtle differences are in the background color of each button changes when a button is pushed, monitorType variable changes, and the Image changes/

private void btnNEWdtm_Click ( object sender, EventArgs e )
        {
          Variables.monitorType = "DTM";

          //set the background color for the active and inactive buttons
          btnNEWdtm.BackColor = Color.PaleTurquoise;
          btnNEWrmm.BackColor = Color.Transparent;
          btnNEWscm.BackColor = Color.Transparent;
          btnNEWbhm.BackColor = Color.Transparent;
          btnNEWbhmpd.BackColor = Color.Transparent;

          // Set the PictureBox image property to this image.
          // ... Then, adjust its height and width properties.
          pbNEWmonitor.Image = Properties.Resources.DTM_12pctSize;
          pbNEWmonitor.Height = 228;
          pbNEWmonitor.Width = 242;
          pbNEWmonitor.Location = new Point ( 190, 46 );
        }
       
        private void btnNEWbhm_Click ( object sender, EventArgs e )
        {
          Variables.monitorType = "BHM";

          //set the background color for the active and inactive buttons
          btnNEWdtm.BackColor = Color.Transparent;
          btnNEWrmm.BackColor = Color.Transparent;
          btnNEWscm.BackColor = Color.Transparent;
          btnNEWbhm.BackColor = Color.PaleTurquoise;
          btnNEWbhmpd.BackColor = Color.Transparent;

          // Set the PictureBox image property to this image.
          // ... Then, adjust its height and width properties.
          pbNEWmonitor.Image = Properties.Resources.BHM_12pctSize;
          pbNEWmonitor.Height = 228;
          pbNEWmonitor.Width = 67;
          pbNEWmonitor.Location = new Point ( 280, 46 );
        }

What would be a more efficient way of handling this?

Thanks for the help,
Ron
0
Comment
Question by:RonWensley
  • 3
  • 2
6 Comments
 
LVL 74

Accepted Solution

by:
käµfm³d   👽 earned 500 total points
Comment Utility
One approach with event handlers is that when you have identical logic for the same type of controls (e.g. multiple Buttons, multiple Labels, multiple TextBoxes, etc.), then you can, in essence, overload the handler by having it handle more than one control's event. You can achieve this because the first argument to any control's event handler--that is, the sender parameter--is the control which raised the event. So say, for example, you want any button on your form to turn red when it is clicked. You could achieve that with a single event handler:

protected void button_click(object sender, EventArgs e)
{
    Button btn = (Button)sender;

    btn.BackColor = Color.Red;
}

Open in new window


Here I cast the first parameter to a Button. I can do this because I only associated this handler with buttons on my form. In theory, I could associate this handler with a label. But the code would error at runtime on the cast. You would only ever do this by mistake, but you can always code defensively if you like:

protected void button_click(object sender, EventArgs e)
{
    Button btn = sender as Button;

    if (btn != null)
    {
        btn.BackColor = Color.Red;
    }
}

Open in new window


----------------------------------

Now in your case, you can't do the above. The reason is because your handler logic is slightly different in each case. For instance, you assign "DTM" to monitorType in the first handler, and "BHM" to the same property in the second handler. There are a couple more lines that have the same problem. So what you can do is "refactor" the logic in these handlers out into a new method. Each of your handlers will call this new method, but they will each pass the values that they deal with. For example, you could declare a new method:

private void UpdateUI(string monitorType, Button buttonToHighlight, Image pbImage, int pbImageHeight, int pbImageWidth, Point pbImageLocation)
{
    Variables.monitorType = monitorType;

    //set the background color for the active and inactive buttons
    btnNEWdtm.BackColor = Color.Transparent;
    btnNEWrmm.BackColor = Color.Transparent;
    btnNEWscm.BackColor = Color.Transparent;
    btnNEWbhm.BackColor = Color.Transparent;
    btnNEWbhmpd.BackColor = Color.Transparent;
    
    buttonToHighlight.BackColor = Color.Turquoise;

    // Set the PictureBox image property to this image.
    // ... Then, adjust its height and width properties.
    pbNEWmonitor.Image = pbImage;
    pbNEWmonitor.Height = pbImageHeight;
    pbNEWmonitor.Width = pbImageWidth;
    pbNEWmonitor.Location = pbImageLocation;
}

Open in new window


Then you would adjust your handlers:

private void btnNEWdtm_Click ( object sender, EventArgs e )
{
    UpdateUI("DTM", btnNEWdtm, Properties.Resources.DTM_12pctSize, 228, 242, new Point ( 190, 46 ));
}

private void btnNEWbhm_Click ( object sender, EventArgs e )
{
    UpdateUI("BHM", btnNEWbhm, Properties.Resources.BHM_12pctSize, 228, 67, new Point ( 280, 46 ));
}

Open in new window

0
 
LVL 62

Expert Comment

by:Fernando Soto
Comment Utility
Hi Ron;

You only need one event handler for the five buttons to do what you need, please see code snippet below. You will need to change the event hanlder name in the Properties window to point to the same event. Also change the Tag property for each button to the proper type for the button. For example DTM, RMM and so on. Please read the comments I placed in the code.

private void btnNEWtype_Click(object sender, EventArgs e)
{
    // Cast sender to a type Button, which is the button that was clicked on
    Button btn = (Button)sender;
    // In the form designer set the Tag property of the button with the type DTM, RMM, SCM, ..., 
    // This way you can set Variables.monitorType to the correct value
    Variables.monitorType = btn.Tag.ToString();

    //set the background color for the active and inactive buttons
    // this will set them all the same. switch statement will set the correct one
    btnNEWdtm.BackColor = Color.Transparent;
    btnNEWrmm.BackColor = Color.Transparent;
    btnNEWscm.BackColor = Color.Transparent;
    btnNEWbhm.BackColor = Color.Transparent;
    btnNEWbhmpd.BackColor = Color.Transparent;

    switch (btn.Tag.ToString())
    {
        case "DTM" :
            btnNEWdtm.BackColor = Color.PaleTurquoise;
            // Set the PictureBox image property to this image.
            // ... Then, adjust its height and width properties.
            pbNEWmonitor.Image = Properties.Resources.DTM_12pctSize;
            pbNEWmonitor.Height = 228;
            pbNEWmonitor.Width = 242;
            pbNEWmonitor.Location = new Point(190, 46);
            break;
        case "RMM" :
            btnNEWrmm.BackColor = Color.PaleTurquoise;

            pbNEWmonitor.Image = Properties.Resources.RMM_12pctSize;
            pbNEWmonitor.Height = ???;
            pbNEWmonitor.Width = ???;
            pbNEWmonitor.Location = new Point(???, ??);                    
            break;
        case "SCM" :
            btnNEWscm.BackColor = Color.PaleTurquoise;

            pbNEWmonitor.Image = Properties.Resources.SCM_12pctSize;
            pbNEWmonitor.Height = ???;
            pbNEWmonitor.Width = ???;
            pbNEWmonitor.Location = new Point(???, ??);                    
            break;
        case "BHM" :
            btnNEWbhm.BackColor = Color.PaleTurquoise;

            pbNEWmonitor.Image = Properties.Resources.BHM_12pctSize;
            pbNEWmonitor.Height = 228;
            pbNEWmonitor.Width = 67;
            pbNEWmonitor.Location = new Point ( 280, 46 );
            break;
        case "BHMPD" :
            btnNEWbhmpd.BackColor = Color.PaleTurquoise;

            pbNEWmonitor.Image = Properties.Resources.BHMPD_12pctSize;
            pbNEWmonitor.Height = ???;
            pbNEWmonitor.Width = ???;
            pbNEWmonitor.Location = new Point(???, ??);                    
            break;
        default:
            break;
    }
}

Open in new window

0
 
LVL 74

Expert Comment

by:käµfm³d 👽
Comment Utility
@FernandoSoto

But your attempt still has 5 pieces of code to maintain in the event that any bit of that logic changes. That's not good in terms of maintainability.
0
How to run any project with ease

Manage projects of all sizes how you want. Great for personal to-do lists, project milestones, team priorities and launch plans.
- Combine task lists, docs, spreadsheets, and chat in one
- View and edit from mobile/offline
- Cut down on emails

 

Author Closing Comment

by:RonWensley
Comment Utility
Thank you for provide such a complete tutorial on this matter.  I implemented your code example and it works perfectly.  Now I have knowledge to apply else were in coding.  

Thank you,
Ron
0
 
LVL 62

Expert Comment

by:Fernando Soto
Comment Utility
@kaufmed;

Yes I do but in your implementation you still have 5 event handlers and a added function and each time one of the event handlers are fired you have the extra cost of a context switch happening. Many ways to handle the same problem. ;=)
0
 
LVL 74

Expert Comment

by:käµfm³d 👽
Comment Utility
Agreed. But my approach would result in compile-time errors and yours would result in run-time errors. If I have to add a new thing to change--let's say changing the context menu--then I would end up changing the function signature:

private void UpdateUI(string monitorType, Button buttonToHighlight, Image pbImage, int pbImageHeight, int pbImageWidth, Point pbImageLocation, ContextMenu menu)
{
...

Open in new window


...and then change each handler to send the additional parameter. But what if I missed one of the handlers? Boom! Compile-time error. In your code, if I happen to overlook one of the case statements, then I've now got a bug because my context menu never gets updated. Furthermore, switch/case used in this fashion tend to grow...and grow...and grow....  Personally, if I have to scroll past one screen to see the entire function, then I'm not happy.

Don't get me wrong:  Your approach works, and it's a common pattern to see, but I don't prefer it for the reasons I mention. At the very least, the approach is readable and understandable, so it at least has that going for it, and IMO readable code is one of the most important--if not the most important--things to strive for in code today  = )
0

Featured Post

Highfive + Dolby Voice = No More Audio Complaints!

Poor audio quality is one of the top reasons people don’t use video conferencing. Get the crispest, clearest audio powered by Dolby Voice in every meeting. Highfive and Dolby Voice deliver the best video conferencing and audio experience for every meeting and every room.

Join & Write a Comment

This article will show, step by step, how to integrate R code into a R Sweave document
It was really hard time for me to get the understanding of Delegates in C#. I went through many websites and articles but I found them very clumsy. After going through those sites, I noted down the points in a easy way so here I am sharing that unde…
An introduction to basic programming syntax in Java by creating a simple program. Viewers can follow the tutorial as they create their first class in Java. Definitions and explanations about each element are given to help prepare viewers for future …
In this fifth video of the Xpdf series, we discuss and demonstrate the PDFdetach utility, which is able to list and, more importantly, extract attachments that are embedded in PDF files. It does this via a command line interface, making it suitable …

763 members asked questions and received personalized solutions in the past 7 days.

Join the community of 500,000 technology professionals and ask your questions.

Join & Ask a Question

Need Help in Real-Time?

Connect with top rated Experts

12 Experts available now in Live!

Get 1:1 Help Now