Best way to handle redundant code that has repeated logic?

Question

In my form I have four RadioButtons, based on user selection, this code is executed:

private void button1_Click(object sender, EventArgs e)
        {
            listBox1.Items.Clear();
            if (radioButtonName.Checked)
            {
                var Qr = from n in mylist where n.Name == textBoxSearch.Text select new { n.Name, n.Age, n.Occu, n.Gender };
                foreach (var item in Qr)
                {
                    listBox1.Items.Add("Name: " + item.Name + "   " + "  Age: " + item.Age + "   " + "  Occupation: " + item.Occu + "   " + "  Gender: " + item.Gender);
                }
            }
            if (radioButtonAge.Checked)
            {
                var Qr = from n in mylist where n.Age == textBoxSearch.Text select new { n.Name, n.Age, n.Occu, n.Gender };
                foreach (var item in Qr)
                {
                    listBox1.Items.Add("Name: " + item.Name + "   " + "  Age: " + item.Age + "   " + "  Occupation: " + item.Occu + "   " + "  Gender: " + item.Gender);
                }

            }
            if (radioButtonGender.Checked)
            {
                var Qr = from n in mylist where n.Gender == textBoxSearch.Text select new { n.Name, n.Age, n.Occu, n.Gender };
                foreach (var item in Qr)
                {
                    listBox1.Items.Add("Name: " + item.Name + "   " + "  Age: " + item.Age + "   " + "  Occupation: " + item.Occu + "   " + "  Gender: " + item.Gender);
                }
            }
            if (radioButtonOccupation.Checked)
            {
                var Qr = from n in mylist where n.Occu == textBoxSearch.Text select new { n.Name, n.Age, n.Occu, n.Gender };
                foreach (var item in Qr)
                {
                    listBox1.Items.Add("Name: " + item.Name + "   " + "  Age: " + item.Age + "   " + "  Occupation: " + item.Occu + "   " + "  Gender: " + item.Gender);
                }

            }

        }

The code seems very redundant and repeated, but also I can't find a way to handle all 4 RadioButtons in a single line that has only one variable linked to user choice. myList is a List of a class I created that has 4 string properties (Name, Age, Gender, Occu)

raven · Answer 1 · 2016-09-05T21:38:30.183

wrap everything in a function like this one:

public void foo(RadioButton radioButton, Expression<Func<MyItem, bool>> expression)
    {
        if (radioButton.Checked)
        {
            var Qr = mylist.AsQueryable().Where(expression).Select(x => String.Format("Name: {0}, Age: {1}, Occ: {2}, Gender: {3}", x.Name, x.Age, x.Occu, x.Gender)).ToList();

            foreach (var item in Qr)
            {
                listBox1.Items.Add(item);
            }
        }
    }

    private void button1_Click(object sender, EventArgs e)
    {
        listBox1.Items.Clear();
        foo(radioButtonName, c => c.Gender == textBoxSearch.Text);
        foo(radioButtonAge, c => c.Age == textBoxSearch.Text);
        foo(radioButtonGender, c =>  c.Gender == textBoxSearch.Text);
        foo(radioButtonOccupation, c => c.Occu == textBoxSearch.Text);
    }



public class MyItem
    {
        public String Occu { get; set; }

        public String Age { get; set; }
        public String Name { get; set; }
        public String Gender { get; set; }

    }

score 8 · Accepted Answer · edited Sep 05 '16 at 20:09

8

The only difference is in filter (where) all the other can be combined:

 private void button1_Click(object sender, EventArgs e) {
   var lines = mylist
     .Where(item => radioButtonName.Checked && item.Name == textBoxSearch.Text ||
                    radioButtonAge.Checked && item.Age == textBoxSearch.Text ||
                    radioButtonGender.Checked && item.Gender == textBoxSearch.Text ||
                    radioButtonOccupation.Checked && item.Occu == textBoxSearch.Text)
     .Select(item => string.Format("Name: {0} Age: {1} Occupation: {2} Gender: {3}",
                                    item.Name, item.Age, item.Occu, item.Gender));  

   listBox1.Items.Clear();

   foreach (string line in lines)
     listBox1.Items.Add(line);   
 }

edited Sep 05 '16 at 20:09

Ehsan Sajjad

61,834
16
105
160

answered Sep 05 '16 at 20:01

Dmitry Bychenko

180,369
20
160
215

1

Quick question should the radiobutton check and text comparison be enclosed in brackets or does it not matter? eg `(radioButtonName.Checked && item.Name == textBoxSearch.Text) || (radioButtonAge.Checked && item.Age == textBoxSearch.Text)...` – Nkosi Sep 05 '16 at 20:07
1

@Nkosi: It doesn't matter in the context; however, if version with parentheis is more *readable* for you, you can add `(...)` – Dmitry Bychenko Sep 05 '16 at 20:10
@Nkosi see [this answer](http://stackoverflow.com/a/1196738/5555803). `&&`s get evaluated first – Orkhan Alikhanov Sep 05 '16 at 20:21
@OrkhanAlikhanov thanks. After walking it through in my head I figured it out. – Nkosi Sep 05 '16 at 20:23
You should also override ToString, or make a method called ToListboxString for your class. – Derek Sep 05 '16 at 22:42
@Derek: no, since I return `string` in the `Select`: `.Select(item => string.Format(...));` The code in the question uses *anonymous* classes and we can't declare/override methods there. – Dmitry Bychenko Sep 05 '16 at 22:47

Zein Makki · Answer 3 · 2016-09-07T05:00:39.573

You can use a dictionary to map the RadioButton to its Filter in one go. Assuming MyClass is the type of objects in your List:

private void button1_Click(object sender, EventArgs e)
{
   var mapping = new Dictionary<RadioButton, Func<MyClass, bool>>()
   {
      { radioButtonName , x => x.Name == textBoxSearch.Text },
      { radioButtonAge, x => x.Age == textBoxSearch.Text },
      { radioButtonGender, x => x.Gender == textBoxSearch.Text},
      { radioButtonOccupation, x => x.Occu == textBoxSearch.Text}
   };

   foreach(var map in mapping.Where(x=> x.Key.Checked))
   {
       var Qr = mylist.Where(map.Value).Select(n=> new {n.Name, n.Age, n.Occu, n.Gender});
       foreach (var item in Qr)
       {
          listBox1.Items.Add("Name: " + item.Name + "   " + "  Age: " + item.Age + "   " 
                           + "  Occupation: " + item.Occu + "   " + "  Gender: " 
                           + item.Gender);
       }
   }

}

This way you can easily plug new Radio Buttons using one simple line in the dictionary.

score 0 · Answer 4 · answered Sep 05 '16 at 20:20

You could first generate an anonymous list of your radio buttons and where predictions and then iterate through it (MyItem in this case is sample/placeholder for what your list contains, since I don't know the actual class name):

    private void button1_Click(object sender, EventArgs e)
    {
        // Generate anonymous list of objects that are different
        var radios = new[]
        {
            new { RadioButton = radioButtonName, CallBack = new Func<MyItem, bool>(x => x.Name == textBoxSearch.Text) },
            new { RadioButton = radioButtonAge, CallBack = new Func<MyItem, bool>(x => x.Age == textBoxSearch.Text) },
            new { RadioButton = radioButtonGender, CallBack = new Func<MyItem, bool>(x => x.Occu == textBoxSearch.Text) },
            new { RadioButton = radioButtonOccupation, CallBack = new Func<MyItem, bool>(x => x.Gender == textBoxSearch.Text) },
        };

        // Iterate through list and add items to ListBox1, if RadioButtton is checked
        listBox1.Items.Clear();
        foreach (var radio in radios)
        {
            if (!radio.RadioButton.Checked)
            {
                continue;
            }
            var Qr = mylist.Where(radio.CallBack).Select(n => new { n.Name, n.Age, n.Occu, n.Gender });
            foreach (var item in Qr)
            {
                listBox1.Items.Add($"Name: {item.Name}     Age: {item.Age}     Occupation: {item.Occu}     Gender: {item.Gender}");
            }
        }
    }

Best way to handle redundant code that has repeated logic?

4 Answers4