3
\$\begingroup\$

I have this code, this code querying employee schedules in a month, in a work unit. If the List<Employee> count are few, the code works great, but if the List<Employee> count are many, the code runs slowly. How I can optimize this code?

The DataRow is datagrid row containing employee id, name, etc..

await Task.Run(() =>
{
    List<Employee> employees = DatabaseHelper.GetEmployees(unitId, false, false);
    for (int i = 0; i < employees.Count; i++)
    {
        int no = i + 1;
        DataRow row = DataTableJadwal.Rows.Add(no + ".", employees[i].Nama, employees[i].ID);
        DatabaseHelper.GetUserSchedules(ref row, month, year);
    }

    Thread.Sleep(300);
});

public static void GetUserSchedules(ref DataRow row, int month, int year)
{
    using (SqlConnection sqlConn = new SqlConnection(string.Format(constring, UserID, Password, Server, Database)))
    {
        using (SqlCommand sqlComm = new SqlCommand(
            "SELECT Date, Shift FROM ScheduleList WHERE (ID = @ID) AND (MONTH(Date) = @Month) AND (YEAR(Date) = @Year)"
            , sqlConn))
        {
            try
            {
                sqlComm.Parameters.AddWithValue("@ID", row["ID"]);
                sqlComm.Parameters.AddWithValue("@Month", month);
                sqlComm.Parameters.AddWithValue("@Year", year);

                sqlConn.Open();
                using (SqlDataReader sqlReader = sqlComm.ExecuteReader())
                {

                    int totalHour = 0;
                    while (sqlReader.Read())
                    {
                        DateTime dt = (DateTime)sqlReader["Date"];
                        string shift = (string)sqlReader["Shift"];

                        // a user may had 1,2,3 schedule a day
                        string cellText = row[dt.Day.ToString("00")].ToString();
                        if (cellText.Length < 1)
                            cellText = shift;
                        else
                            cellText += "," + shift;

                        // count schedule hour total
                        if (Utilities.FreeScheduleList.IndexOf(shift.ToUpper()) == -1)
                        {
                            if (shift == "M" || shift == "Q")
                                totalHour += 10;
                            else
                                totalHour += 7;
                        }

                        row[dt.Day.ToString("00")] = cellText;
                    }

                    row["Total"] = totalHour;
                }
            }
            catch (Exception ex)
            {
                MessageBoxEx.Show(ex.Message, "GetUserSchedules Error", MessageBoxButton.OK, MessageBoxImage.Stop);
            }
        }
    }
}
\$\endgroup\$

closed as off-topic by 200_success Aug 24 '17 at 4:49

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Questions containing broken code or asking for advice about code not yet written are off-topic, as the code is not ready for review. After the question has been edited to contain working code, we will consider reopening it." – 200_success
If this question can be reworded to fit the rules in the help center, please edit the question.

  • 1
    \$\begingroup\$ Did you do some benchmarks to figure out what instructions make your code slow? \$\endgroup\$ – Maxim Aug 22 '17 at 7:02
  • \$\begingroup\$ I do not know how to do that, I never do that before, \$\endgroup\$ – SIRS Aug 22 '17 at 7:15
  • \$\begingroup\$ Without benchmarks it's just guessing. Thread.Sleep(300); what are you waiting for here? \$\endgroup\$ – t3chb0t Aug 22 '17 at 7:18
  • 1
    \$\begingroup\$ I would try optimizing two things first - share connection and command object between calling the same request only with different parameters values. Haven't been using this for a long time, but I think it shall be doable... The another way - I'd just join first select (GetEmployees) with second select (GetUserSchedules) into one command and iterate through returned rows. You then have some redundancy, but this is common when optimizing queries. \$\endgroup\$ – SebGruch Aug 22 '17 at 10:38
  • 1
    \$\begingroup\$ I don't think this is working code. row in DatabaseHelper.GetUserSchedules(ref row, month, year); is not defined \$\endgroup\$ – paparazzo Aug 22 '17 at 13:49
1
\$\begingroup\$

First, create index on your table as @RobH suggestion

CREATE INDEX IX_ScheduleList_Id_Date on your_schema.ScheduleList
(
    Id,
    Date,
    Shift
)

Then as @Paparazzi suggestion change your query so you only need to call it once,

SELECT e.ID, nama, Date, Shift 
  FROM Employee e 
  JOIN ScheduleList s
    ON s.ID = e.ID
   AND e.unitId = @UnitId
   AND s.Date >= @StartDate 
   AND s.Date < @AndDate

Then filter the result in your code, it should be something like this.

while (sqlReader.Read())
{
    string ID = sqlReader.GetString(0);
    var rows = dataTable.AsEnumerable().Where(r => r.Field<string>("ID") == ID);
    if (rows.Count() < 1)
    {
        // add row if row is not exist
        num += 1;
        string Nama = sqlReader.GetString(1);
        row = dataTable.Rows.Add(num + ".", Nama, ID);

        row["Total"] = 0;
    }
    else { row = rows.First(); }

    // YOUR CODE
    // ....
}

Don't forget to pass the DataTable, not the DataRow

await Task.Run(() =>
{
    DatabaseHelper.GetEmployeeAndSchedules(ref DataTable, unitId, month, year);
    Thread.Sleep(300);
});
\$\endgroup\$
  • \$\begingroup\$ Thanks @nyongrand that code boost the execution time from ~7 sec to ~2 \$\endgroup\$ – SIRS Aug 24 '17 at 14:37
3
\$\begingroup\$

I'm going to try to avoid raging about the data table based data access layer because I doubt a random person on the internet is going to change the direction of your project. So, let's look at your SQL query:

SELECT Date, 
       Shift 
FROM   ScheduleList 
WHERE  ID = @ID 
  AND  MONTH(Date) = @Month
  AND  YEAR(Date) = @Year

I've removed the extra brackets because they were just adding noise. This query could easily be changed so you could add a covering index to your schedule list table:

SELECT Date, 
       Shift 
FROM   ScheduleList 
WHERE  ID = @ID 
  AND  Date > @Date
  AND  Date < DATEADD(MONTH, 1, @Date)

Combined with an index on the Date and Id columns which also includes Shift and your query will be much faster.

CREATE INDEX IX_ScheduleList_Id_Date on your_schema.ScheduleList
(
    Id,
    Date
) INCLUDE (Shift)

I'd be more inclined to do this though:

SELECT Date, 
       Shift 
FROM   ScheduleList 
WHERE  ID = @ID 
  AND  Date > @StartDate
  AND  Date < @EndDate

This way you can search for schedule list in any window of time, not just one month at a time.

Having said that, what I'd actually do is return the employee list with their schedules at the same time - SQL is a relational database: it's good at joining related data together.

(SQL typed into browser and I'm rusty so it might not be 100% right).

\$\endgroup\$
  • \$\begingroup\$ I use sql 2000, so instead I use CREATE INDEX IX_ScheduleList_Id_Date on your_schema.ScheduleList(Id, Date, Shift) is that right? after using your suggestion, the time needed decrease from ~100 ms to ~60ms \$\endgroup\$ – SIRS Aug 23 '17 at 1:11
  • \$\begingroup\$ @SIRS - it's 2017 why are you still using SQL 2000? As far as I know, 2000 hasn't been supported since 2013. You should upgrade to a later (supported) version of SQL Server. \$\endgroup\$ – RobH Aug 23 '17 at 7:30
  • \$\begingroup\$ I did not do programming for my self, everything was setup and I can't just change the thing, sometimes it's easier to live with a decision you don't like \$\endgroup\$ – SIRS Aug 23 '17 at 8:17
  • \$\begingroup\$ @SIRS - easier isn't the same as better. I would hope you would at least tell them that it's not good that they're relying on such old software. \$\endgroup\$ – RobH Aug 23 '17 at 8:32
1
\$\begingroup\$
DateTime dt = (DateTime)sqlReader["Date"]; 

This is faster

DateTime dt = sqlReader.GetDate(0);

string shift = sqlReader.GetString(1);

Create and open the connection once and pass it to GetUserSchedules

Indexes cannot be used with MONTH(Date) = @Month

SELECT Date, 
       Shift 
FROM   ScheduleList 
WHERE  ID = @ID 
  AND  Date > @start
  AND  Date < @end

sqlComm.Parameters.AddWithValue("@start", new DateTime(year, month, 0);
sqlComm.Parameters.AddWithValue("@end", new DateTime(year, month + 1, 0));

I bet you can do this with one query

SELECT e.ID, nama, Date, Shift 
  FROM Employee e 
  JOIN ScheduleList s
    ON s.ID = e.ID
   AND e.unitId = x 
   AND s.Date > DateTime(year, month, 0) 
   AND s.Date < DateTime(year, month + 1, 0)

Use sqlCmd for command. sqlComm looks too much like sqlConn. Use sqlCon.

\$\endgroup\$

Not the answer you're looking for? Browse other questions tagged or ask your own question.