Sending a message based on selected row

Question

I have a gridview with CheckBox inside it. When the user selects a row and clicks on a button, a message is sent for the specific user.

How can this be optimized?

protected void btnSendSMSForSpeceficUser_Click(object sender, EventArgs e)
    {
        using (NoavaranModel.NoavaranEntities1 dbContext=new NoavaranModel.NoavaranEntities1())
        {
            int userId = Int32.Parse(Session["UserId"].ToString());
            var query = (from p in dbContext.Users
                         where p.UserId == userId
                         select p.CountOfSMS).FirstOrDefault();
            var query2 = (from p in dbContext.Users
                          where p.UserId == userId
                          select p).FirstOrDefault();
            int j = 0,sendCount=0;
            for (int i = 0; i <= grdStudents.Rows.Count - 1; i++)
            {
                string MobileNumbers = grdStudents.Rows[i].Cells[2].Text;
                int stID = Int32.Parse(grdStudents.Rows[i].Cells[0].Text);
                var query3 = (from p in dbContext.Students
                              where p.Id == stID
                              where p.IsRecivedSMS==false
                              select p).FirstOrDefault();
                GridViewRow row = grdStudents.Rows[i];
                CheckBox Ckbox = (CheckBox)row.FindControl("chkSelectStudents");
                if (Ckbox.Checked)
                {
                    j++;
                    if (j <= query) {
                        sendCount++;
                        Utility.SendMessageForStudents(MobileNumbers, txtMessage.Text);
                        query3.IsRecivedSMS = true;
                    }
                }
            }
            query2.CountOfSMS = query.Value - sendCount;
            dbContext.SaveChanges();
            ListBox1.Items.Add(sendCount.ToString());
        }
    }

Answer 1

I'm not sure of everything (worked with notepad) but:

protected void btnSendSMSForSpeceficUser_Click(object sender, EventArgs e)
{
    using (NoavaranModel.NoavaranEntities1 dbContext = new NoavaranModel.NoavaranEntities1())
    {
        var userId = int.Parse(Session["UserId"].ToString());

        var user = dbContext.Users.FirstOrDefault(p => p.UserId == userId);
        var countOfSms = user.CountOfSMS.Value;
        var j = 0;
        foreach (var row in grdStudents.Rows)
        {
            var cells = row.Cells;
            var MobileNumbers = cells[2].Text;
            var stID = int.Parse(cells[0].Text);

            var student = dbContext.Students.FirstOrDefault(p => p.Id == stID && !p.IsRecivedSMS);

            var Ckbox = (CheckBox)row.FindControl("chkSelectStudents");
            if (Ckbox.Checked)
            {
                j++;
                if (j <= countOfSms) {
                    Utility.SendMessageForStudents(MobileNumbers, txtMessage.Text);
                    student.IsRecivedSMS = true;

                    user.CountOfSMS = countOfSms - 1;
                }
            }
        }

        dbContext.SaveChanges();
        ListBox1.Items.Add(sendCount.ToString());
    }
}

This is a cleaner version of your code with less query (the first was unneccesary) and inside the loop you are making a lot of small query which can be really bad but first you have to modify your code to not work directly with the context in you page code but through an interface (first iterate through the rows and get all data then pass them to the worker (which can work with the context) and return the result & update the UI).

Answer 2

To be perfectly honest it seems quite optimized to me. And to prove that point, about the only thing you could change is making it so you only make one round trip to the server for all of the ID's, but that would also force you to iterate that list twice, consider the following code:

// list of ID's that need their IsReceivedSMS set to true
var ids = grdStudents.Rows;

for (int i = 0; i <= grdStudents.Rows.Count - 1; i++)
{
    string MobileNumbers = grdStudents.Rows[i].Cells[2].Text;
    int stID = Int32.Parse(grdStudents.Rows[i].Cells[0].Text);

    GridViewRow row = grdStudents.Rows[i];
    CheckBox Ckbox = (CheckBox)row.FindControl("chkSelectStudents");
    if (Ckbox.Checked)
    {
        j++;
        if (j <= query) {
            sendCount++;
            Utility.SendMessageForStudents(MobileNumbers, txtMessage.Text);

            // we've sent the SMS so let's queue it for update
            ids.Add(stID);
        }
    }
}

// this **should** generate an IN clause on the server
// allowing you to perform just **one** round trip
var query3 = (from p in dbContext.Students
              where ids.Contains(p.Id)
              where p.IsRecivedSMS==false
              select p).ToList();

// here is the second iteration
query3.ForEach(o => o.IsRecivedSMS = true)

However, if that query to Students is heavy (which it seems unlikely but it could be), the iterating a very select list a second time would be less expensive.

Again, there are some things that need to be considered here that I have no knowledge of.

So, to recap, the code you originally posted makes one round trip for every ID regardless of its need to update IsReceivedSMS whereas this example makes one round trip for the very select list that needs updated. However, there are times that performing this type of optimization literally holds no value, and thus my original statement.