CODEWITHSUNDEEP
c#sqlasp.net
Edd Davis
Edd Davis

Reputation: 59

Make this ForEach more efficient

I have approx 30-40 vehicles which have individaul callsigns (4 digit numbers) which will be Strings. Each of these vehicles has their own table. i need to check against each of these tables and see if the clean date is the same as the date passed in (using the "CleanDate" variable) currently I have an open connection with the following ForEach loop that will mark as checked if the date = CleanDate

  SqlConnection cnn = new SqlConnection(constr);
        cnn.Open();

        foreach (ListItem item in checkboxlistAM.Items)
        {
            string callsignstring = item.Text;
            string queryTable = "SELECT [date made ready AM] FROM [" + callsignstring + "]";
            SqlCommand cmd = new SqlCommand(queryTable, cnn);
            SqlDataReader rdr = cmd.ExecuteReader();

            while (rdr.Read())
            {
                if (rdr[0].ToString()!="")
                {
                    if ((DateTime)rdr[0] == CleanDate)
                    {
                        item.Selected = true;
                    }
                }
            }
            rdr.Close();


        }



        cnn.Close();

This is not efficient. How do i make this better?

The ideas I've had are -

SQL Stored Procedure - can anyone help with what that would look like? Could SP_MSFOREACHTABLE be used or would that make it less efficient?

adding the "callsigns" (strings) to an array or list and passing that in somehow?

TIA

Upvotes: 0

Views: 67

Answers (1)

Laoujin
Laoujin

Reputation: 10239

Usually you shouldn't worry about performance much but doing database calls in a loop is pretty much always going to slow down considerably once you get more than just a few checkboxlistAM.Items.

As Kevin mentioned in the comments, the real problem is that you are creating new tables with the same data.

If all Vehicles have the same structure, you can add a discriminator column, which value would be your current table names.

Alternively, you could eliminate the looped query with UNION:

SELECT Id, PlateNumber, '1' AS Descriminator FROM Vehicles1
UNION
SELECT Id, PlateNumber, '2' FROM Vehicles2
UNION
SELECT Id, PlateNumber, '3' FROM etc

Instead of building this query yourself, you could use a View and select from that view solely:

SELECT * FROM VehiclesView WHERE Descriminator IN ('1', '2', ...);

To avoid Sql injection you should probably don't build your SQL yourself but use an ORM or SqlCommand.Parameters.

If your data is all quite different for each type of Vehicle unions may still be used:

SELECT Id AS INTEGER1, PlateNumber AS STRING1, xxx AS STRING2 From Vehicles1
UNION
SELECT ID, yyy AS STRING1, zzz AS STRING2 FROM Vehicles2

This will be quite messy but you could implement some sort of AntiCorruption layer which turns the indecipherable STRING1, STRING2, ... back into something human readable.

If all the Vehicles are all so different, perhaps you could use a NoSql database (like MongoDb) instead?

If something is IDisposable, use a using construct. It's a good practice for cleanup:

using (SqlConnection cnn = new SqlConnection(constr))
{
  // do your stuff
  // .Close() will be called automatically
}

Upvotes: 4

Related Questions