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).

• Sirwan Afifi: thanks great, i have a table called 'SMSLog' with these fields(id,userName,datetime,stMobileNo) and i want to log all messages that admin send,where can i place code is better?

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.

• Sirwan Afifi: thanks,i want admin can select specific users in Gridview and send message for them.
• Mike Perrenoud: @SirwanAfifi, certainly, and that's what your current code is doing. All I did was pull the query that determines the rows to update out into one query instead of many small ones. Again, often time optimizations like this cause more problems than they help, you have to determine that based on whether or not you already have a performance issue.