Optimize c# foreach complexity

Question

Alright so I`m doing a particular case here and I need to pass through a treeview with many foreaches.

After realising my code is not working properly in memory wise I decided to rewrite the code to LINQ. I have managed to get rid of the first foreaches but at the end, I cannot get rid of the signalUI foreach because it needs the Typeof(SignalUI) properties. These being said I don`t know how to do the LINQ for 2 lists at the same time. I have tried working with JOINS but had no results.

 private void signalUITextBox_textchanged(object sender, EventArgs e)
        {
            for (int j = 0; j < _numberOfSelectedSlaves; j++)
                if (_currentNode.Parent.Parent.Text == mas.Slaves[j].Name)
                {
                    int counter = 0;
                    foreach (FrameUI frame in mas.Slaves[j].FrameList)
                    {
                        int signalcounter = 0;
                        if(_currentNode.Text == mas.Slaves[j].FrameList[counter].FrameName)
                        foreach (SignalUI signalui in frame.SignalList)
                        {
                            foreach (Control i in this.Controls)
                            {
                                if (i is TextBox)
                                {
                                    var txtbox = (TextBox)sender;
                                    if (txtbox.Name == i.Name)
                                    {
                                        foreach (var k in typeof(SignalUI).GetProperties())
                                            if (signalui.SignalName + k.Name == txtbox.Name)
                                            {
                                                mas.Slaves[j].FrameList[counter].SignalList[signalcounter].SetProperty(k.Name, Convert.ToString(txtbox.Text));
                                            }
                                    }
                                }
                                    
                            }
                                signalcounter++;
                        }
                        counter++;
                    }
                }
        }

        private void signalUCheckBox_textchanged(object sender, EventArgs e)
    {
        var SlaveQuery = mas.Slaves.Where(slave => slave.Name == _currentNode.Parent.Parent.Text).First();
        var FrameQuery = SlaveQuery.FrameList.Where(frame => frame.FrameName == _currentNode.Text).First();
        var chkbox = (CheckBox)sender;
        var ControlQuery = this.Controls.OfType<CheckBox>().Where(control => control.Name == chkbox.Name).First();


    }

EDIT: Thanks for help. Following below answers i managed to remove all the nested loops

        private void signalUITextBox_textchanged(object sender, EventArgs e)
    {
        int j;
        int signalcounter = 0;
        int counter = 0;
        var txtbox = (TextBox)sender;
        var properties = typeof(SignalUI).GetProperties();
        var SlaveQuery = mas.Slaves.Where(slave => slave.Name == _currentNode.Parent.Parent.Text).First();
        var FrameQuery = SlaveQuery.FrameList.Where(frame => frame.FrameName == _currentNode.Text).First();
        for (j = 0; j < _numberOfSelectedSlaves; j++)
            if (_currentNode.Parent.Parent.Text == mas.Slaves[j].Name)
                break;
        foreach (FrameUI frame in mas.Slaves[j].FrameList)
        {
            if (_currentNode.Text == mas.Slaves[j].FrameList[counter].FrameName)
                break;
            counter++;
        }
        foreach (SignalUI signalui in FrameQuery.SignalList)
        {
            var prop = properties.FirstOrDefault(p => signalui.SignalName + p.Name == txtbox.Name);
            if (prop != null)
            {
                mas.Slaves[j].FrameList[counter].SignalList[signalcounter].SetProperty(prop.Name, Convert.ToString(txtbox.Text));
            }
            signalcounter++;
        }
        

    }

It... sounds like it would be easier to keep a Dictionary mapping slave-name/frame-name tuples to controls. — AKX
– AKX, Commented Aug 30, 2022 at 11:14
Don't use looping in the first place. Use data binding to bind UI components to data objects. The very fact you had to write so much code is a strong indication that something is wrong. People wouldn't be able to create complex UIs if they had to write so much code — Panagiotis Kanavos
– Panagiotis Kanavos, Commented Aug 30, 2022 at 11:14
I have an interface with a lot of dynamically generated Controls.. this seemed to be the most understandable way for me to design it as every control in the UI is named the same as the object properties. If it were for me to change now i would have to change the entire application, so a workaround with LINQ would be the simplest at this stage — AlexS
– AlexS, Commented Aug 30, 2022 at 11:18
LINQ is for making code more readable or quicker to write. It does not really improve performance or memory usage compared to well written regular loops. — JonasH
– JonasH, Commented Aug 30, 2022 at 12:52

Ondrej Tucny · Accepted Answer · 2022-08-30 11:42:21Z

1

As a general rule: Pull repeated calculations that always yield the same result within each loop iteration out of the body of the loop and to them once. The structure of your code will simplify, declutter, and you effectively reduce its inherent complexity.

The outline of your code will look like this:

private void signalUITextBox_textchanged(object sender, EventArgs e)
{
    // build a list of textboxes, but…
    // in fact, you don't need this at all if you care only of 'sender' as a TextBox
    var textBoxes = this.Controls.OfType<TextBox>().ToList();
    
    // hence:
    if (!(sender is TextBox)) return;
    var txtbox = (textBoxe) sender;

    // build a list of properties (hint: reduce the list further, if you can)
    var properties = typeof(SignalUI).GetProperties();
    // in addition, a dictionary may come handy: .ToDictionary(p => p.Name);

    // the rest of the code seems cluttered and unnecessarily complex, too,
    // but hard to refactor without detailed knowledge of your data structures
    for (int j = 0; j < _numberOfSelectedSlaves; j++)
    {
        if (_currentNode.Parent.Parent.Text == mas.Slaves[j].Name)
        {
            int counter = 0;
            foreach (FrameUI frame in mas.Slaves[j].FrameList)
            {
                int signalcounter = 0;
                if (_currentNode.Text == mas.Slaves[j].FrameList[counter].FrameName)
                    foreach (SignalUI signalui in frame.SignalList)
                    {
                        // find the matching property
                        var prop = properties.FirstOrDefault(p => signalui.SignalName + p.Name == txtbox.Name);
                        if (prop != null)
                        {
                            mas.Slaves[j].FrameList[counter].SignalList[signalcounter].SetProperty(prop.Name, Convert.ToString(txtbox.Text));
                        }
                        signalcounter++;
                    }
                counter++;
            }
        }
    }
}

Three nested loops less.

answered Aug 30, 2022 at 11:42

Ondrej Tucny

28.2k6 gold badges73 silver badges93 bronze badges

Sign up to request clarification or add additional context in comments.

2 Comments

AlexS Over a year ago

Following your code i have managed to refactor my code even better and removed all the nested foreaches. I will edit the main question.

Ondrej Tucny Over a year ago

@AlexS Great work! I'm glad it helped.

Collectives™ on Stack Overflow

Optimize c# foreach complexity

1 Answer 1

2 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

1 Answer 1

2 Comments

Your Answer

Sign up or log in

Post as a guest

Related