winforms C# .NET - How to Improve the Performance of my code while using loops(for ,foreach, etc...)

Question

I am developing an app for colleges, a part of the app is to set the weekly schedule of classes.

My parameters are Classroom number, Day of the week, and Time Slot.

So I have a range of classrooms, each classroom has a number.

in the Run-Time, the app generates some buttons based on how many classrooms I have on my database and sets classroom numbers on each Button.

What I want to do is to Label each Classroom button with "Red" BackColor if that certain classroom is full in the given Day of the Week and the given Time Slot.

I have accomplished what I wanted to do and my code works with no errors, but my only problem is now the performance.

Here is my Code:

private OleDbConnection Connection = new OleDbConnection();


private void SomeMethod(string Day, string Time)
    {
        int MaxIndex = 0;
        string str1 = "select Max([Row Index]) from Table";       
      OleDbCommand Command1 = new OleDbCommand(str1, Connection);
        Connection.Open();
        if (Command1.ExecuteScalar() == DBNull.Value)
            MaxIndex = 1;

        else
            MaxIndex = Convert.ToInt32(Command1.ExecuteScalar());
        Connection.Close();

        for (int i = 0; i < MaxIndex; i++)
        {
            string str = "select [classroom Number] from Table where [Day] = @ParamDay and [Time] = @ParamTime and [Row Index] = @ParamIndex";

            OleDbCommand Command = new OleDbCommand(str, Connection);
            Command.Parameters.Add("@ParamDay", Day);
            Command.Parameters.Add("@ParamTime", Time);
            Command.Parameters.Add("@ParamIndex", i + 1);

            Connection.Open();
            OleDbDataReader reader = Command.ExecuteReader();

            if (reader.Read())
            {
                foreach (Button btn in ButtonsPanel.Controls)
                {
                    if (btn.Text == reader["classroom Number"].ToString())
                    {
                        btn.BackColor = Color.Red;
                    }

                }
                Connection.Close();
            }
        }


    }

so this code takes about 13 seconds if I have 200 rows which I expect to have.

The question is... Is there anything I can do to my code so that these 13 seconds will reduce to at least 2-4 seconds?

For information: I have searched a lot on the internet, but could not find the solution to my issue here.

This is a bit open ended. You could move the I/O to a background thread to stop the UI from freezing (e.g. a worker thread) and optimize your database call. Maybe you have a missing index for example. — Kit, Jan 29 '21 at 01:16
Not in front of a computer to test, but you're opening and closing the database connection on each iteration, which is inefficient. Wrap the `Connection` initialisation in a `using` statement around the for loop and remove `Connection.Close` as that will be handled by the `using` statement — Hayden, Jan 29 '21 at 01:17
Also, remove the number of sql reads you need to do by potentially removing the for loop altogether, and use `[Row Index] <= MaxIndex` in your SQL query, and handle accordingly. This will only call the database once, not 200 times. — Hayden, Jan 29 '21 at 01:21
@Kit, I have thought of that. However, the user would still have to wait for a long time. The following actions of the user depend on the output of this code. — Ala Hirorî, Jan 29 '21 at 01:26
@Hayden, I don't think this would work in my case. I need to check for each row, the row Index is to select the row that needs to be checked for each iteration. If I remove the for loop then how will I check for each row? without for loop this code would execute once and would check for only one row that I will have to hard-code its index. — Ala Hirorî, Jan 29 '21 at 01:32
Retrieve the data once, and based on the row id in the data, act accordingly. You have the row id in the result set, which is now saved in memory. — Hayden, Jan 29 '21 at 01:34
@Hayden, I would be happy if you could add your solution with both modifications you suggested to me when you have access to your computer. I will understand your point better. — Ala Hirorî, Jan 29 '21 at 01:44
Well, if you're able to edit your question to provide the full method, as well as how you're initialising `Connection`, I'll see what I can do when I have time. — Hayden, Jan 29 '21 at 01:47
just do one select statement to query all the data in one go as hayden suggests, this means it will return multiple rows, then iterate each returned row and set the button ( I'd also add distinct for classroom ) — Keith Nicholas, Jan 29 '21 at 02:29
if I was using dapper ( which you should ) you can do it in one line like ```conn.Query("select [classroom Number] from Table where [Day] = @Day and [Time] = @Time and [Row Index] < @MaxIndex", new { Day, Time, MaxIndex}).ToList().ForEach(c => ButtonsPanel.Controls.Where(btn => btn.Text == c.ToString()).ToList().ForEach(btn => btn.BackColor = Color.Red)))``` — Keith Nicholas, Jan 29 '21 at 02:38
Use a profiler. Find out where exactly the time is being spent. Everything else is quite literally guesses. There's always better code with faster performance but **how much faster**. Without empirical data you're left chasing your tail. — Zer0, Jan 29 '21 at 03:39
I disagree with the closure of this question. Even though, I got the answer I was looking for, but closing the question because of not being focused enough is what I disagree with. The question is completely focused on the performance of my code and how can I improve my code to perform faster. I described the scenario I am working on and what I am actually trying to accomplish with codes. So whats that closure of the question for not being focused? There were already developers who got my point and posted suitable solutions. — Ala Hirorî, Jan 29 '21 at 13:16
In your question title you are asking about performance in relation to loops, while looping is a factor here it is misleading, the actual performance concern is in how you are connecting to the database. You also did not provide any diagnostics or research into which individual line in the code you were unhappy with the performance of. If you had done that this would have been a more focused question. — Chris Schaller, Jan 30 '21 at 13:27
@Chris, First, I asked this question because I thought the performance issue was caused by the for loop, I did not know that I could handle my case without the for loop, it was something new to me, I thought I still had to use for loop but do something with another part of my code which was why I asked the question and gave enough info for others to give me 3 CORRECT ANSWERS. If I knew where the real concern was, I wouldn’t have asked the question in the first place. At the end of the day, my question helped me overcome my problem, which is the goal of stack overflow, I guess. — Ala Hirorî, Jan 31 '21 at 11:57
@Chris, if you are thinking of a better Title, I assume you have ability to edit it. — Ala Hirorî, Jan 31 '21 at 12:01
This was in regard to the closure of the question. _your_ issue was solved, and that is great! But this particular post is not a good fit for the wider community as it is now full of speculation and as the solution is to _not use a loop_ it is not easy for new readers to identify that it was not the loop itself that is the underlying issue. We don't want people thinking that loops are inherently inefficient. I have been scolded recently for thinking the same thing, the company line is that SO is for the community, not OP... Its about building a knowledge base now, who knew? — Chris Schaller, Jan 31 '21 at 20:40

Gellio Gao · Answer 1 · 2021-01-29T03:34:10.213

There are two things you can change to improve:

Open and close the connection only once which can reduce the code running time.
Fetch all the data you want to process in one query.

Check out the code below:

    private OleDbConnection Connection = new OleDbConnection();

    private void SomeMethod(string Day, string Time)
    {
        int MaxIndex = 0;
        string str1 = "select Max([Row Index]) from Table";       
        OleDbCommand Command1 = new OleDbCommand(str1, Connection);
        Connection.Open();
        if (Command1.ExecuteScalar() == DBNull.Value)
            MaxIndex = 1;
        else
            MaxIndex = Convert.ToInt32(Command1.ExecuteScalar());

        string str = "select [classroom Number] from Table where [Day] = @ParamDay and [Time] = @ParamTime and [Row Index] between 1 and @ParamIndex";

        OleDbCommand Command = new OleDbCommand(str, Connection);
        Command.Parameters.Add("@ParamDay", Day);
        Command.Parameters.Add("@ParamTime", Time);
        Command.Parameters.Add("@ParamIndex", MaxIndex);

        OleDbDataReader reader = Command.ExecuteReader();
        while (reader.Read())
        {
            foreach (Button btn in ButtonsPanel.Controls)
            {
                if (btn.Text == reader["classroom Number"].ToString())
                {
                    btn.BackColor = Color.Red;
                }
            }
        }

        Connection.Close();
    }

This code will only execute the first row, needs to be modified to use `while (reader.Read())` — Hayden, Jan 29 '21 at 03:31
Thanks for your answer, I believe this code can also work in my case. — Ala Hirorî, Jan 29 '21 at 08:32

score 0 · Accepted Answer · answered Jan 29 '21 at 03:22

You do not seem to need the for cycle at all. And the MaxIndex either. Just download the records for the time and mark buttons.

private void SomeMethod(string Day, string Time)
{
    HashSet<string> classNumbers = new HashSet<string>();

    string str = "select [classroom Number] from Table where [Day] = @ParamDay and [Time] = @ParamTime";
    using (OleDbCommand Command = new OleDbCommand(str, Connection))
    {
        Command.Parameters.Add("@ParamDay", Day);
        Command.Parameters.Add("@ParamTime", Time);                
        Connection.Open();
        using (OleDbDataReader reader = Command.ExecuteReader(CommandBehavior.CloseConnection))
        {
            while (reader.Read())
            {
                classNumbers.Add(reader["classroom Number"].ToString());
            }
        }
    }

    foreach (Button btn in ButtonsPanel.Controls)
    {
        if (classNumbers.Contains(btn.Text))
        {
            btn.BackColor = Color.Red;
        }
    }
}

@Hayden CommandBehavior.CloseConnection should handle that imho — Antonín Lejsek, Jan 29 '21 at 03:37
@Hayden the `using` implementation will close the connection automatically when it exists the scope. — Chris Schaller, Jan 30 '21 at 13:29

Hayden · Answer 3 · 2021-02-01T00:51:28.387

As per my comments, you only need to execute the query once and loop through the result set. This will have performance gains since you're now only accessing the IO once, as IO is typically very slow.

Here's an example (haven't tested):

private void SomeMethod(string day, string time)
{
    // Using statement helps to dispose any resources once done with the connection
    // connectionString can be any string that opens your database
    using (OleDbConnection connection = new OleDbConnection(connectionString))
    {
        // The query has removed the notion of index, it will just get all the data for that day and time
        string query = "SELECT [classroom Number] FROM Table WHERE [Day] = @ParamDay AND [Time] = @ParamTime";

        // Since OleDbCommand inherits from IDisposable, use a using statement
        using (OleDbCommand command = new OleDbCommand(query, connection))
        {
            // Notice how we don't use index anymore
            command.Parameters.Add("@ParamDay", day);
            command.Parameters.Add("@ParamTime", time);

            // Open connection here, don't need to close connection
            connection.Open();

            // Since OleDbDataReader inherits from IDisposable, use a using statement
            using (OleDbDataReader reader = command.ExecuteReader())
            {
                // We're now looping through all the rows in the result set
                while (reader.Read())
                {
                    UpdateButtonColor(reader["classroom Number"].ToString());
                }
            }
        }
    }
}

private void UpdateButtonColor(string classroomNumber)
{
    foreach (Button btn in ButtonsPanel.Controls)
    {
        if (btn.Text == classroomNumber)
        {
            btn.BackColor = Color.Red;
        }
    }
}

Newer versions of C# allow the using statement to not require curly braces (which reduces nesting), which would look like:

private void SomeMethod(string day, string time)
{
    string query = "SELECT [classroom Number] FROM Table WHERE [Day] = @ParamDay AND [Time] = @ParamTime";
    using OleDbConnection connection = new OleDbConnection(connectionString);
    using OleDbCommand command = new OleDbCommand(query, connection);

    command.Parameters.Add("@ParamDay", day);
    command.Parameters.Add("@ParamTime", time);

    connection.Open();

    using OleDbDataReader reader = command.ExecuteReader();
    
    while (reader.Read())
    {
        UpdateButtonColor(reader["classroom Number"].ToString());
    }
}

Documentation for this can be found here

Thanks for your Answer. I have implemented the first Answer's Code, But I am sure that yours will work as well. — Ala Hirorî, Jan 29 '21 at 08:30
Can you provide a documentation link to this new `using` syntax? This is _not_ the correct syntax for the C# 8 _using declaration_. Please only post code examples in answers that will compile. — Chris Schaller, Jan 30 '21 at 13:39
@ChrisSchaller Thanks for pointing out, have removed round brackets around using statements. — Hayden, Feb 01 '21 at 00:52

winforms C# .NET - How to Improve the Performance of my code while using loops(for ,foreach, etc...)

3 Answers3