2
\$\begingroup\$

I'm trying to optimize my code and in the process of doing that I made a class Query_builder that handles SQL queries. That class has couple of methods: ExecuteScalar (execute scalar queries that mainly return int or long as a type), FillDataTable(used to fill Datatable with data from SQL tables) and the main one Execute (used for "shaping" the query). My plan backfired on me and I ended up slowing down my reports in app (time it takes to fill a full monthly report).

Class Query_builder:

 class Query_builder
{
    public T Execute<T>(string sql, CommandType commandType, Func<NpgsqlCommand, T> function, List<NpgsqlParameter> parameters)
    {

        using (NpgsqlConnection conn = KonekcijaNaServer.Spajanje(true))  //created a connection with postgresql database
        {
            using (var cmd = new NpgsqlCommand(sql, conn) { CommandType = commandType }) //using a NpgsqlCommand with query as "sql", "conn" as connection and commandType as a type of query.
            {
                if (parameters.Count > 0 ) //if there are any parameters in the list
                {
                    foreach (var parameter in parameters)
                    {
                        cmd.Parameters.AddWithValue(parameter.ParameterName,parameter.Value);  //add parameter to the referenced place 
                    }
                }                    
                return function(cmd); 
            }
        }            
    }

    public T ExecuteScalar<T>(string sql, CommandType commandType, List<NpgsqlParameter> parameters)
    {
       return Execute<T>(sql, commandType, c => { //return to Execute<T> with parameters: "sql","commandType",lamba expression as function and parameters.
            var returnValue =c.ExecuteScalar();  //execute ExecuteScalar and get the return value from Execute<T>
            return (returnValue != null && returnValue != DBNull.Value && returnValue is T) //return if not null or DBNull
            ? (T)returnValue : default(T); //if condition is true, return "returnValue", if not return default of declared type
        }, parameters);          
    }

    public DataTable FillDataTable(string sql, CommandType commandType, List<NpgsqlParameter> parameters)
    {
        return Execute<DataTable>(sql, commandType, c => FillDataTable(c), parameters);                                                                                          
    }

    private DataTable FillDataTable(NpgsqlCommand command)
    {
        using (NpgsqlConnection conn = KonekcijaNaServer.Spajanje(true)) //using the connection to postgresql database
        {
            DataTable dataTable = new DataTable(); //create a new datatable
            using (NpgsqlDataAdapter adapter = new NpgsqlDataAdapter(command)) //create an adapted with sql command as "command"
            {
                command.Connection = conn; //connection for executing is "conn"
                adapter.Fill(dataTable); //fill the adapted with result DataTable from sql query
            }
            return dataTable;                 
        }          
    }  
}

My connection class KonekcijaNaServer (Edited by idea of RobH):

public static class KonekcijaNaServer
{
    public static string Connectionstring = "Server=192.168.130.240;Port=5433;User Id=postgres;" +
                                           "Password=*********;Database=postgres;Pooling=false;";

    public static NpgsqlConnection Spajanje(bool openConnection)
    {
        NpgsqlConnection conn = new NpgsqlConnection(Connectionstring);
        if (openConnection)
        {
            conn.Open();
        }
        return conn;
    }
 }

The way I call the methods (for example, if I need a DataTable as return):

komanda = "begin;select zaposlenik_id,ime,prezime,odjel,bolovanje,godisnji from zaposlenici " +
                              "order by zaposlenik_id;commit;"; //sql command
                    listaParametara.Clear(); //clearing the list if any parameters are left from last query
                    dt = instanca.FillDataTable(komanda, CommandType.Text, listaParametara); //"dt" is Datatable, "instanca" is an instance of Query_builder class and it calls a method FillDataTable

For generating a report I'm calling class Query_builder 14 times, for 14 different queries and in the background are calculations for example: generating work hours from some of those queries and filling one column of datagridview with that data.

Before making a class Query_builder my queries used to look different and time for generating report was much shorted:

NpgsqlCommand bolovanje_godisnji = new NpgsqlCommand("begin;select zaposlenik_id,ime,prezime,odjel,bolovanje,godisnji from zaposlenici " +
                          "order by zaposlenik_id;commit;", conn);
                    NpgsqlDataAdapter da = new NpgsqlDataAdapter(bolovanje_godisnji);
                    DataTable dz = new DataTable();
                    da.Fill(dz);

I assume the problem is in generating SQL queries because my logic behind and data manipulation stayed the same with a little bit of tweaks (nothing that would slow down process so much).

I was trying to get an example of how much it takes me to fill the full report. My results are following: In this build with class Query_builder where that class acts as core of SQL query generation, It takes about 00:01:16 sec to execute the whole report and fill datagridview with it. On my last build without general class for SQL queries, the whole report would calculate and fill the datagridview within 00:00:10 seconds.

My whole report starts by checking the checkbox checkBoxMjesecni (represents monthly report) and selecting an item from combobox comboBoxMjesec (list that fills comboBoxMjesec represents months). After its all filled I look at the role the current user has (admin, one department or another) and assign the proper limitations depending on a role (in this case, admin will have full report of all employees and if someone else from some other department logs in he will have have monthly report based on his department):

if (checkBoxMjesecni.Checked == true)
            {
            #region Odredjivanje mjeseca //determining whuch month is selected
            string mjesec = comboBoxMjesec.GetItemText(comboBoxMjesec.SelectedItem);
                string mjesec_broj = "";
                if (mjesec == "Siječanj") mjesec_broj = "1";
                else if (mjesec == "Veljača") mjesec_broj = "2";
                else if (mjesec == "Ožujak") mjesec_broj = "3";
                else if (mjesec == "Travanj") mjesec_broj = "4";
                else if (mjesec == "Svibanj") mjesec_broj = "5";
                else if (mjesec == "Lipanj") mjesec_broj = "6";
                else if (mjesec == "Srpanj") mjesec_broj = "7";
                else if (mjesec == "Kolovoz") mjesec_broj = "8";
                else if (mjesec == "Rujan") mjesec_broj = "9";
                else if (mjesec == "Listopad") mjesec_broj = "10";
                else if (mjesec == "Studeni") mjesec_broj = "11";
                else if (mjesec == "Prosinac") mjesec_broj = "12";
                #endregion

                string godina = comboBoxMjesecGodina.GetItemText(comboBoxMjesecGodina.SelectedItem); //gets what year is selected
                //if admin is logged in
                if (odjel_id_prop == 0) //odjel_id_prop represents a propery that is assigned after logging in, says: "this user is admin, he has deparment_id=0, Its taken from class constructor
                {
                    komanda = "begin;select zaposlenik_id,ime,prezime,odjel,bolovanje,godisnji from zaposlenici " +
                              "order by zaposlenik_id;commit;";
                    listaParametara.Clear();  
                    dt = instanca.FillDataTable(komanda, CommandType.Text, listaParametara); 
                }
                //if another department is logged in
                else if (odjel_id_prop == 1)
                {
                    komanda = "begin;select zaposlenik_id,ime,prezime,odjel,bolovanje,godisnji from zaposlenici " +
                              " where (odjel_id=1 or odjel_id=2 or odjel_id=5 or odjel_id=6) order by zaposlenik_id;commit;";
                    //odjel_id represents deparment_id
                    listaParametara.Clear(); 
                    dt = instanca.FillDataTable(komanda, CommandType.Text, listaParametara); 

                    //if another department is logged in
                }
                else if (odjel_id_prop == 7)
                {
                    komanda = "begin;select zaposlenik_id,ime,prezime,odjel,bolovanje,godisnji from zaposlenici " +
                              " where odjel_id = 7 order by zaposlenik_id;commit;";
                    listaParametara.Clear(); 
                    dt = instanca.FillDataTable(komanda, CommandType.Text, listaParametara); //popuni dt tablicu s podacima koje vrati metoda FillDataTable()

                }
                //if anyone else is logged in
                else
                {
                    komanda = "begin;select zaposlenik_id,ime,prezime,odjel,bolovanje,godisnji from zaposlenici " +
                              " where odjel_id = @odjel_id order by zaposlenik_id;commit;";
                    listaParametara.Add(new NpgsqlParameter { ParameterName = "@odjel_id", Value = odjel_id_prop });
                    dt = instanca.FillDataTable(komanda, CommandType.Text, listaParametara);
                    listaParametara.Clear();
                }                   

After I found out who is logged in and gave proper query for it and fill the datatable dt with return value of query. (Edited by idea of Derek Smyth) :

for (int i = 0; i < dt.Rows.Count; i++) //dt is the datatable that was filled with data depending on the role of user that is logged in
                {
                    //query that returns how many times an employee logged in in that month of a year
                    komanda = "begin;select count(*) from radni_sati where zaposlenik_id=@zaposlenik_id and " +
                              " extract(month from datum)=@mjesec_broj and extract(year from datum)=@godina ;commit;";
                    listaParametara.Add(new NpgsqlParameter { ParameterName = "@mjesec_broj", Value = mjesec_broj }); //month we are making report on 
                    listaParametara.Add(new NpgsqlParameter { ParameterName = "@godina", Value = godina }); //year we are making report on
                    listaParametara.Add(new NpgsqlParameter { ParameterName = "@zaposlenik_id", Value = dt.Rows[i][0] }); //employee id
                    long ZapisUMjesecu = instanca.ExecuteScalar<long>(komanda, CommandType.Text, listaParametara);
                    listaParametara.Clear();

                    //returns the department_id from current employee 
                    komanda = "begin;select odjel_id from zaposlenici where zaposlenik_id=@zaposlenik_id ;commit;";
                    listaParametara.Add(new NpgsqlParameter { ParameterName = "@zaposlenik_id", Value = dt.Rows[i][0] });
                    int OdjelID = instanca.ExecuteScalar<int>(komanda, CommandType.Text, listaParametara);
                    listaParametara.Clear();

                    //if that employee has atlest 1 day of work
                    bool radio = ZapisUMjesecu > 0; //ako postoje zapisi, tj. ako se zaposlenik bar jednom prijavio u mjesecu
                    if (radio)
                    {
                        listaParametara.Add(new NpgsqlParameter { ParameterName = "@mjesec_broj", Value = mjesec_broj });
                        listaParametara.Add(new NpgsqlParameter { ParameterName = "@godina", Value = godina });
                        listaParametara.Add(new NpgsqlParameter { ParameterName = "@zaposlenik_id", Value = dt.Rows[i][0] });

                        Task<long> iskoristeno_bolovanje_tsk = Task.Run(() => instanca.ExecuteScalar<long>(
                        "begin;select count(*) from bolovanje where extract(month from bolovanje.datum)" +
                        "=@mjesec_broj and extract(year from bolovanje.datum)=@godina and zaposlenik_id=@zaposlenik_id ;commit;",
                        CommandType.Text, listaParametara));

                        //returns all the dates that employee was sick 
                        Task<DataTable> DatumiBolovanja_tsk = Task.Run(() => instanca.FillDataTable(
                            "begin;select datum from bolovanje where extract(month from bolovanje.datum)" +
                            "=@mjesec_broj and extract(year from bolovanje.datum)=@godina and zaposlenik_id=@zaposlenik_id ;commit;",
                            CommandType.Text, listaParametara));


                        //return number of holidays employee had
                        Task<long> iskoristeni_godisnji_tsk = Task.Run(() => instanca.ExecuteScalar<long>(
                            "begin;select count(*) from godisnji where extract(month from godisnji.datum)" +
                            "=@mjesec_broj and extract(year from godisnji.datum)=@godina and zaposlenik_id=@zaposlenik_id;commit;",
                            CommandType.Text, listaParametara));

                        //returns all the dates that employee was on holiday
                        Task<DataTable> DatumiGodisnjeg_tsk = Task.Run(() => instanca.FillDataTable(
                            "begin;select datum from godisnji where extract(month from godisnji.datum)" +
                            "=@mjesec_broj and extract(year from godisnji.datum)=@godina and zaposlenik_id=@zaposlenik_id ;commit;",
                            CommandType.Text, listaParametara));


                        //returns a number of days that employee was working
                        Task<long> redovi_tsk = Task.Run(() => instanca.ExecuteScalar<long>(
                            "begin;select count(*) from radni_sati where zaposlenik_id=@zaposlenik_id" +
                            " and (dolazak is not null and odlazak is not null and sati_rada is not null) and extract(month from radni_sati.datum)" +
                            "=@mjesec_broj and extract(year from radni_sati.datum)=@godina group by zaposlenik_id order by zaposlenik_id;commit;",
                            CommandType.Text, listaParametara));

                        //calculate number of hours that employee was working in that month  
                        Task<DataTable> UkupnoSati_tsk = Task.Run(() => instanca.FillDataTable(
                            "begin;select cast(radni_sati.sati_rada as time) from radni_sati where zaposlenik_id=@zaposlenik_id " +
                            " and extract(month from radni_sati.datum)=@mjesec_broj and extract(year from radni_sati.datum)=@godina;commit;",
                            CommandType.Text, listaParametara));

                        // wait for all the tasks above to complete
                        Task.WaitAll(
                            iskoristeno_bolovanje_tsk, DatumiBolovanja_tsk,
                            iskoristeni_godisnji_tsk, DatumiGodisnjeg_tsk,
                            redovi_tsk, UkupnoSati_tsk);

                        // get the values after each of these return
                        long iskoristeno_bolovanje = iskoristeno_bolovanje_tsk.Result;
                        DataTable DatumiBolovanja = DatumiBolovanja_tsk.Result;

                        long iskoristeni_godisnji = iskoristeni_godisnji_tsk.Result;
                        DataTable DatumiGodisnjeg = DatumiGodisnjeg_tsk.Result;

                        long redovi = redovi_tsk.Result;
                        DataTable UkupnoSati = UkupnoSati_tsk.Result;


                        listaParametara.Clear();

When I'm done with that I calculate how many workhours an employee had in a month:

#region Izracun Sati rada //calculation of workhours per employee
                        TimeSpan Ukupno_sati = TimeSpan.Zero;
                        TimeSpan Ukupno_sati_prosjek = TimeSpan.Zero;
                        for (int x = 0; x < UkupnoSati.Rows.Count; x++) 
                        {
                            string ukupno_prijelazni = Convert.ToString(UkupnoSati.Rows[x][0]); //pretvara zapis u string
                            string Ukupno_sati_string = Convert.ToString(UkupnoSati.Rows[x][0]).Substring(ukupno_prijelazni.IndexOf(' ') + 1); //takes only time from timestamp format
                            if (Ukupno_sati_string != "") //if its valid
                            {
                                Ukupno_sati += TimeSpan.Parse(Ukupno_sati_string); //add time to sum of hours (Ukupno_Sati)
                                Ukupno_sati_prosjek += TimeSpan.Parse(Ukupno_sati_string); //later used for calculating average worktime
                            }

                            if (UkupnoSati.Rows.Count > 0) //if that person worked that month
                            {
                                //format a string in "hh:mm:ss" format
                                suma = string.Format("{0}:{1}:{2}", ((int)Ukupno_sati.TotalHours), Ukupno_sati.Minutes, Ukupno_sati.Seconds);
                            }

                        }

                    #endregion 

Now I'm calculating time of sick days and holidays per employee:

                            string nazivDana = ""; //name of the day in a week
                        string bolovanje_vrijeme = ""; //sick days
                        string godisnji_vrijeme = ""; //holidays
                        TimeSpan ts_bolovanje_vrijeme = TimeSpan.Zero;
                        TimeSpan ts_godisnji_vrijeme = TimeSpan.Zero;
                    if (DatumiBolovanja.Rows.Count > 0) //if an employee has atlest 1 sick day in that month
                    {
                        //calculating sick days
                        for (int x = 0; x < DatumiBolovanja.Rows.Count; x++)
                        {
                            DateTime datum = Convert.ToDateTime(DatumiBolovanja.Rows[x][0]); //takes date
                            nazivDana = datum.ToString("dddd"); //takes the date and turns it into name of the day
                            //everything in if, else if and else loops is calculation depending on department_id and day of the week

                            if (nazivDana != "Subota" && (OdjelID == 1 || OdjelID == 2 || OdjelID == 3 || OdjelID == 5 || OdjelID == 6))
                            {
                                ts_bolovanje_vrijeme = TimeSpan.FromHours(8);
                                ts_bolovanje_vrijeme += TimeSpan.FromTicks(ts_bolovanje_vrijeme.Ticks);                                                                    
                                bolovanje_vrijeme = string.Format("{0}:{1}:{2}", ((int)ts_bolovanje_vrijeme.TotalHours), ts_bolovanje_vrijeme.Minutes.ToString("00"), ts_bolovanje_vrijeme.Seconds.ToString("00"));
                            }
                            else if (nazivDana != "Subota" && (OdjelID == 4 || OdjelID == 7 || OdjelID == 8 || OdjelID == 9 || OdjelID == 10))
                            {
                                ts_bolovanje_vrijeme = TimeSpan.FromHours(7);
                                ts_bolovanje_vrijeme += TimeSpan.FromTicks(ts_bolovanje_vrijeme.Ticks);                                                                     
                                bolovanje_vrijeme = string.Format("{0}:{1}:{2}", ((int)ts_bolovanje_vrijeme.TotalHours), ts_bolovanje_vrijeme.Minutes.ToString("00"), ts_bolovanje_vrijeme.Seconds.ToString("00"));
                            }
                            else if (nazivDana == "Subota")
                            {
                                ts_bolovanje_vrijeme = TimeSpan.FromHours(5);
                                ts_bolovanje_vrijeme += TimeSpan.FromTicks(ts_bolovanje_vrijeme.Ticks);                                                                    
                                bolovanje_vrijeme = string.Format("{0}:{1}:{2}", ((int)ts_bolovanje_vrijeme.TotalHours), ts_bolovanje_vrijeme.Minutes.ToString("00"), ts_bolovanje_vrijeme.Seconds.ToString("00"));
                            }
                        }

                    }
                    else bolovanje_vrijeme = string.Format("00:00:00"); //if an employee had no sick days 

                    //same goes for holidays, same logic
                    if (DatumiGodisnjeg.Rows.Count > 0)
                    {
                        for (int x = 0; x < DatumiGodisnjeg.Rows.Count; x++)
                        {
                            DateTime datum = Convert.ToDateTime(DatumiGodisnjeg.Rows[x][0]);
                            nazivDana = datum.ToString("dddd");
                            if (nazivDana != "Subota" && (OdjelID == 1 || OdjelID == 2 || OdjelID == 3 || OdjelID == 5 || OdjelID == 6))
                            {
                                ts_godisnji_vrijeme = TimeSpan.FromHours(8); 
                                ts_godisnji_vrijeme += TimeSpan.FromTicks(ts_godisnji_vrijeme.Ticks);                                                                                                        
                                godisnji_vrijeme = string.Format("{0}:{1}:{2}", ((int)ts_godisnji_vrijeme.TotalHours), ts_godisnji_vrijeme.Minutes.ToString("00"), ts_godisnji_vrijeme.Seconds.ToString("00"));
                            }
                            else if ((nazivDana != "Subota" && (OdjelID == 4 || OdjelID == 7 || OdjelID == 8 || OdjelID == 9 || OdjelID == 10)))
                            {
                                ts_godisnji_vrijeme = TimeSpan.FromHours(7); 
                                ts_godisnji_vrijeme += TimeSpan.FromTicks(ts_godisnji_vrijeme.Ticks);                                                                    
                                godisnji_vrijeme = string.Format("{0}:{1}:{2}", ((int)ts_godisnji_vrijeme.TotalHours), ts_godisnji_vrijeme.Minutes.ToString("00"), ts_godisnji_vrijeme.Seconds.ToString("00"));
                            }
                            else if (nazivDana == "Subota")
                            {
                                ts_godisnji_vrijeme = TimeSpan.FromHours(5); 
                                ts_godisnji_vrijeme += TimeSpan.FromTicks(ts_godisnji_vrijeme.Ticks);                                                                   
                                godisnji_vrijeme = string.Format("{0}:{1}:{2}", ((int)ts_godisnji_vrijeme.TotalHours), ts_godisnji_vrijeme.Minutes.ToString("00"), ts_godisnji_vrijeme.Seconds.ToString("00"));
                            }
                        }
                    }
                    else godisnji_vrijeme = string.Format("00:00:00");

Next calculations are official feast days which are stored in a list called Blagdani_lista :

 #region Izracun blagdana
                        //calculation of feast days
                        DateTime danasnji_datum = DateTime.Now.Date; //current date
                        string Ukupno_vrijeme = ""; 
                        TimeSpan Ukupno = TimeSpan.Zero; //sum
                        double Ukupuno_sekundi_double = 0;
                        //sum of workhours+sick days+holidays
                        Ukupuno_sekundi_double = Ukupno_sati.TotalMilliseconds + ts_bolovanje_vrijeme.TotalMilliseconds + ts_godisnji_vrijeme.TotalMilliseconds;
                        double blagdani_ukupno = 0; //feast days
                        TimeSpan Blagdani_vrijeme = TimeSpan.Zero;
                        string blagdani_ispis = ""; //string for formating
                        int brojac = 0; //counter for checking does a person have a "right" to have calculation of feast days in summ of hours

                        foreach (var item in Blagdani.Blagdani_lista) //foreach feast day in list
                        {

                            if (item.datum.Month == Convert.ToInt32(mjesec_broj)) //if that feast day is in the month we are making report for
                            {

                                //calculate the department_id
                                komanda = "begin;select odjel_id from zaposlenici where zaposlenik_id=@zaposlenik_id;commit;";
                                listaParametara.Add(new NpgsqlParameter { ParameterName = "@zaposlenik_id", Value = dt.Rows[i][0] });
                                int odjelID = instanca.ExecuteScalar<int>(komanda, CommandType.Text, listaParametara);
                                listaParametara.Clear();

                                //check is the employee on sick day when the feast day is                            
                                komanda = "begin;select count(*) from bolovanje where datum=@datum and " +
                                          " zaposlenik_id=@zaposlenik_id;commit;";
                                listaParametara.Add(new NpgsqlParameter { ParameterName = "@datum", Value = item.datum.ToString("dd/MM/yyyy", CultureInfo.InvariantCulture) });
                                listaParametara.Add(new NpgsqlParameter { ParameterName = "@zaposlenik_id", Value = dt.Rows[i][0] });
                                long BlagdanBolovanje = instanca.ExecuteScalar<long>(komanda, CommandType.Text, listaParametara);
                                listaParametara.Clear();



                                int rezultat = danasnji_datum.CompareTo(item.datum); //compare did the feast day pass the current date or is it today or is it comming
                                //if the feast day passed or if its today and its not sunday and he is not on his sick day and he belongs to some of the deparments listed
                                // same type of calculation is made as for sick days or holidays
                                if ((rezultat == 0 || rezultat == 1) && item.dan_u_tjednu != "Subota" && BlagdanBolovanje != 1 && (odjelID == 4 || odjelID == 7 || odjelID == 8 || odjelID == 9 || odjelID == 10))
                                {
                                    brojac++; //increase the counter 
                                    TimeSpan Blagdan = TimeSpan.FromHours(7);
                                    Ukupuno_sekundi_double += Blagdan.TotalMilliseconds;
                                    Ukupno = TimeSpan.FromMilliseconds(Ukupuno_sekundi_double);
                                    Ukupno_vrijeme = string.Format("{0}:{1}:{2}", ((int)Ukupno.TotalHours), Ukupno.Minutes, Ukupno.Seconds);

                                    blagdani_ukupno += Blagdan.TotalMilliseconds;
                                    Blagdani_vrijeme = TimeSpan.FromMilliseconds(blagdani_ukupno);
                                    blagdani_ispis = string.Format("{0}:{1}:{2}", ((int)Blagdani_vrijeme.TotalHours), Blagdani_vrijeme.Minutes.ToString("00"), Blagdani_vrijeme.Seconds.ToString("00"));
                                }
                                //another set of departments
                                else if ((rezultat == 0 || rezultat == 1) && item.dan_u_tjednu != "Subota" && BlagdanBolovanje != 1 && (odjelID == 1 || odjelID == 2 || odjelID == 3 || odjelID == 5 || odjelID == 6))
                                {
                                    brojac++;
                                    TimeSpan Blagdan = TimeSpan.FromHours(8);
                                    Ukupuno_sekundi_double += Blagdan.TotalMilliseconds;
                                    Ukupno = TimeSpan.FromMilliseconds(Ukupuno_sekundi_double);
                                    Ukupno_vrijeme = string.Format("{0}:{1}:{2}", ((int)Ukupno.TotalHours), Ukupno.Minutes, Ukupno.Seconds);

                                    blagdani_ukupno += Blagdan.TotalMilliseconds;
                                    Blagdani_vrijeme = TimeSpan.FromMilliseconds(blagdani_ukupno);
                                    blagdani_ispis = string.Format("{0}:{1}:{2}", ((int)Blagdani_vrijeme.TotalHours), Blagdani_vrijeme.Minutes.ToString("00"), Blagdani_vrijeme.Seconds.ToString("00"));
                                }

                                else if ((rezultat == 0 || rezultat == 1) && item.dan_u_tjednu == "Subota" && BlagdanBolovanje != 1 && (odjelID == 4 || odjelID == 7 || odjelID == 8 || odjelID == 9 || odjelID == 10))
                                {
                                    brojac++;
                                    TimeSpan Blagdan = TimeSpan.FromHours(5);
                                    Ukupuno_sekundi_double += Blagdan.TotalMilliseconds;
                                    Ukupno = TimeSpan.FromMilliseconds(Ukupuno_sekundi_double);
                                    Ukupno_vrijeme = string.Format("{0}:{1}:{2}", ((int)Ukupno.TotalHours), Ukupno.Minutes, Ukupno.Seconds);

                                    blagdani_ukupno += Blagdan.TotalMilliseconds;
                                    Blagdani_vrijeme = TimeSpan.FromMilliseconds(blagdani_ukupno);
                                    blagdani_ispis = string.Format("{0}:{1}:{2}", ((int)Blagdani_vrijeme.TotalHours), Blagdani_vrijeme.Minutes.ToString("00"), Blagdani_vrijeme.Seconds.ToString("00"));

                                }
                            }
                        }
                        if (brojac == 0) //if person doesnt have a "right" to have that feast day on his summ
                        {
                            blagdani_ispis = string.Format("00:00:00"); 
                        }
                        brojac = 0; //reset the counter

                        Ukupno = TimeSpan.FromMilliseconds(Ukupuno_sekundi_double); //summ becomes what it was before feast day calculation

The last calculation I make is average worktime:

#region Izracun prosjeka
                        //gets number of working hours for that month
                        komanda = "begin;select sati_rada from radni_sati where zaposlenik_id=@zaposlenik_id " +
                                  " and (napomena='' or napomena is null) and extract(month from radni_sati.datum)=@mjesec_broj and extract(year from radni_sati.datum)=@godina" +
                                  " and sati_rada is not null;commit;";
                        listaParametara.Add(new NpgsqlParameter { ParameterName = "@zaposlenik_id", Value = dt.Rows[i][0] });
                        listaParametara.Add(new NpgsqlParameter { ParameterName = "@mjesec_broj", Value = mjesec_broj });
                        listaParametara.Add(new NpgsqlParameter { ParameterName = "@godina", Value = godina });
                        DataTable SatiRadaBezNapomene = instanca.FillDataTable(komanda, CommandType.Text, listaParametara);
                        listaParametara.Clear();

                        double prosjek = 0;
                        for (int x = 0; x < UkupnoSatiQuery.Rows.Count; x++)
                        {
                            //calculate department_id
                            komanda = "begin;select odjel_id from zaposlenici where zaposlenik_id=@zaposlenik_id;commit;";
                            listaParametara.Add(new NpgsqlParameter { ParameterName = "@zaposlenik_id", Value = dt.Rows[i][0] });
                            int odjel_id = instanca.ExecuteScalar<int>(komanda, CommandType.Text, listaParametara);
                            listaParametara.Clear();

                            if (SatiRadaBezNapomene.Rows.Count > 0) //if that person even has worktime in this month
                            {
                                //depending on the deparment_id
                                if (odjel_id == 1 || odjel_id == 2 || odjel_id == 3 || odjel_id == 5 || odjel_id == 6)
                                {
                                    //redovi=number of rows(days) person has been working + number of holidays +number of sickdays
                                    prosjek = Ukupno.TotalMilliseconds / (redovi + iskoristeni_godisnji + iskoristeno_bolovanje);
                                    upisProsjeka = TimeSpan.FromMilliseconds(prosjek).ToString(@"hh\:mm\:ss");

                                }
                                else if (odjel_id == 4 || odjel_id == 7 || odjel_id == 8 || odjel_id == 9 || odjel_id == 10)
                                {
                                    prosjek = Ukupno.TotalMilliseconds / (redovi + iskoristeni_godisnji + iskoristeno_bolovanje);
                                    upisProsjeka = TimeSpan.FromMilliseconds(prosjek).ToString(@"hh\:mm\:ss");
                                }


                            }
                        }

After all the calculations I fill the datagridview:

                            Ukupno_vrijeme = string.Format("{0}:{1}:{2}", ((int)Ukupno.TotalHours), Ukupno.Minutes, Ukupno.Seconds);
                        DataGridViewRow row = new DataGridViewRow();
                        row.CreateCells(izvjestaj_tablica);
                        row.Cells[0].Value = dt.Rows[i][0]; //ID
                        row.Cells[1].Value = dt.Rows[i][1]; //name
                        row.Cells[2].Value = dt.Rows[i][2]; //surname
                        row.Cells[3].Value = dt.Rows[i][3]; //deparment_id
                        row.Cells[4].Value = suma; //working hours
                        row.Cells[5].Value = upisProsjeka; //average worktime
                        row.Cells[6].Value = redovi; //days of work
                        row.Cells[7].Value = bolovanje_vrijeme;    //sick days
                        row.Cells[8].Value = godisnji_vrijeme; //holidays
                        row.Cells[9].Value = blagdani_ispis; //feast days
                        row.Cells[10].Value = Ukupno_vrijeme; //total

That's how my monthly report looks like. I didn't include some basic stuff like creating a datagridview, filling it with columns and their names, formating them and some other properties for the datagridview.

\$\endgroup\$
  • \$\begingroup\$ The code you've added, is this a single giant if clause? :-O \$\endgroup\$ – t3chb0t Oct 11 '17 at 14:52
  • \$\begingroup\$ @t3chb0t you could say that yea...its everything that happends when I press "start" for monthly report. The problem is why did that giant if slowed down so much after Query_builder \$\endgroup\$ – Marko Petričević Oct 11 '17 at 17:42
  • \$\begingroup\$ @MarkoPetričević hi there why not use something like EntityFrameworks for this problem? \$\endgroup\$ – BKSpurgeon Oct 12 '17 at 1:52
  • \$\begingroup\$ @BKSpurgeon The only thing why Im not using it is coz Im used to this implementation, but Im opened for propositions \$\endgroup\$ – Marko Petričević Oct 12 '17 at 7:04
  • \$\begingroup\$ Are you sure the difference in time isn't caused by you now opening and closing the connection multiple times? I can't see your old code but it looks like it was using a shared connection. I have no idea whether NpgsqlConnection supports connection pooling. \$\endgroup\$ – RobH Oct 12 '17 at 7:57
1
\$\begingroup\$

I can't really help with the performance issue because following your code is really hard (I don't speak the language it's in). That said, I just can't look at this and say nothing:

class Konekcija_na_server
{
    public static string Connectionstring = "Server=127.0.0.1;Port=5433;User Id=postgres;" +
    "Password=*********;Database=postgres;Pooling=false;";

    public static NpgsqlConnection Spajanje(string status)
    {
        bool spajanje = false;

        NpgsqlConnection conn = new NpgsqlConnection(Connectionstring);
        if (status == "spoji")
        {             
            conn.Open();
            spajanje = true;
        }
        else if (status == "prekini")
        {
            conn.Close();              
        }

        if (spajanje == true)
        {
            return conn;                
        }
        else return null;
    }
}
  1. Include an access modifier (public, internal etc.) in your class definitions.
  2. C# never uses snake_case. Classes should be PascalCase
  3. Why is the method called the same thing as a bool declared in the body? That implies one (or both) are named badly
  4. Why is status a string? Use an enum or bool (openConnection)
  5. Why is it named in English? Be consistent with your language
  6. What is the point in calling it with "prekini"? You create a new closed connection and then do nothing with it.
  7. if (foo == true) is better written as if (foo)
  8. The final else is redundant
  9. The class should be static

You could do this:

public static class KonekcijaNaServer
{
    public static string Connectionstring = "Server=127.0.0.1;Port=5433;User Id=postgres;" +
    "Password=*********;Database=postgres;Pooling=false;";

    public static NpgsqlConnection Spajanje(bool openConnection)
    {
        NpgsqlConnection conn = new NpgsqlConnection(Connectionstring);
        if (openConnection)
        {             
            conn.Open();
        }
        return conn;
    }
}

You need to aggressively refactor the rest of the code so it's readable before you can hope to track down the problems with it.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the answer and tips, that's one part less to edit =) P.S. One question: Do I have to close connection after using ur edit, coz im confused now, does it automatically closes or its a manual procedure? \$\endgroup\$ – Marko Petričević Oct 12 '17 at 11:02
  • \$\begingroup\$ @MarkoPetričević - it's closed by the using statement calling Dispose. You would never have closed any connections using the method as it was. \$\endgroup\$ – RobH Oct 12 '17 at 11:09
  • \$\begingroup\$ I hope Im not asking much, but what if Im not using using statement? \$\endgroup\$ – Marko Petričević Oct 12 '17 at 11:15
  • \$\begingroup\$ @MarkoPetričević - if you're not using a using then you'll need to call Close or Dispose on the NpgsqlConnection instance. Your method created a new one and then closed that rather than closing any of the previously opened connections. \$\endgroup\$ – RobH Oct 12 '17 at 11:16
  • \$\begingroup\$ got it, thanks for answering, didn't realize that. \$\endgroup\$ – Marko Petričević Oct 12 '17 at 11:37
1
\$\begingroup\$

The code that queries the database and returns sick days, holidays, working days, has a pattern to it. The code sets parameters, runs scalar/datatable query, sets parameters... and so forth.

I'm talking about this code here...

                //if that employee has atlest 1 day of work
                if (ZapisUMjesecu > 0)
                {
                    //return number of sick days he had
                    komanda = "begin;select count(*) from bolovanje where extract(month from bolovanje.datum)" +
                              "=@mjesec_broj and extract(year from bolovanje.datum)=@godina and zaposlenik_id=@zaposlenik_id ;commit;";
                    listaParametara.Add(new NpgsqlParameter { ParameterName = "@mjesec_broj", Value = mjesec_broj });
                    listaParametara.Add(new NpgsqlParameter { ParameterName = "@godina", Value = godina });
                    listaParametara.Add(new NpgsqlParameter { ParameterName = "@zaposlenik_id", Value = dt.Rows[i][0] });
                    long iskoristeno_bolovanje = instanca.ExecuteScalar<long>(komanda, CommandType.Text, listaParametara);
                    listaParametara.Clear();

                    //returns all the dates that employee was sick 
                    komanda = "begin;select datum from bolovanje where extract(month from bolovanje.datum)" +
                              "=@mjesec_broj and extract(year from bolovanje.datum)=@godina and zaposlenik_id=@zaposlenik_id ;commit;";
                    listaParametara.Add(new NpgsqlParameter { ParameterName = "@zaposlenik_id", Value = dt.Rows[i][0] });
                    listaParametara.Add(new NpgsqlParameter { ParameterName = "@mjesec_broj", Value = mjesec_broj });
                    listaParametara.Add(new NpgsqlParameter { ParameterName = "@godina", Value = godina });
                    DataTable DatumiBolovanja = instanca.FillDataTable(komanda, CommandType.Text, listaParametara);
                    listaParametara.Clear();

The parameters are set each time and then cleared. That is unnecessary and the parameters can be shared across each scalar/datatable query. It's small performance improvement but it all adds up.

Additionally, these queries could be run asynchronously! Maybe... I'm a little hesitant because it depends on the implementation of instanca and the sharing of the database connection. So this might not work!

Here's some new code.

    bool HasWorked = ZapisUMjesecu > 0;

    if (HasWorked)
    {
        // all parameters are the same across all calls to instanca (order of parameters shouldn't matter?)
        listaParametara.Add(new NpgsqlParameter { ParameterName = "@mjesec_broj", Value = mjesec_broj });
        listaParametara.Add(new NpgsqlParameter { ParameterName = "@godina", Value = godina });
        listaParametara.Add(new NpgsqlParameter { ParameterName = "@zaposlenik_id", Value = dt.Rows[i][0] });

        // if all goes well these queries should run asynchronously
        //   depends on how instanca has been implemented... 

        //return number of sick days employee had
        Task<long> iskoristeno_bolovanje_tsk = Task.Run(() => instanca.ExecuteScalar<long>(
            "begin;select count(*) from bolovanje where extract(month from bolovanje.datum)" +
            "=@mjesec_broj and extract(year from bolovanje.datum)=@godina and zaposlenik_id=@zaposlenik_id ;commit;", 
            CommandType.Text, listaParametara));

        //returns all the dates that employee was sick 
        Task<DataTable> DatumiBolovanja_tsk = Task.Run(() => instanca.FillDataTable(
            "begin;select datum from bolovanje where extract(month from bolovanje.datum)" +
            "=@mjesec_broj and extract(year from bolovanje.datum)=@godina and zaposlenik_id=@zaposlenik_id ;commit;", 
            CommandType.Text, listaParametara));


        //return number of holidays employee had
        Task<long> iskoristeni_godisnji_tsk = Task.Run(() => instanca.ExecuteScalar<long>(
            "begin;select count(*) from godisnji where extract(month from godisnji.datum)" +
            "=@mjesec_broj and extract(year from godisnji.datum)=@godina and zaposlenik_id=@zaposlenik_id;commit;",
            CommandType.Text, listaParametara));

        //returns all the dates that employee was on holiday
        Task<DataTable> DatumiGodisnjeg_tsk = Task.Run(() => instanca.FillDataTable(
            "begin;select datum from godisnji where extract(month from godisnji.datum)" +
            "=@mjesec_broj and extract(year from godisnji.datum)=@godina and zaposlenik_id=@zaposlenik_id ;commit;",
            CommandType.Text, listaParametara));


        //returns a number of days that employee was working
        Task<long> redovi_tsk = Task.Run(() => instanca.ExecuteScalar<long>(
            "begin;select count(*) from radni_sati where zaposlenik_id=@zaposlenik_id" +
            " and (dolazak is not null and odlazak is not null and sati_rada is not null) and extract(month from radni_sati.datum)" +
            "=@mjesec_broj and extract(year from radni_sati.datum)=@godina group by zaposlenik_id order by zaposlenik_id;commit;",
            CommandType.Text, listaParametara));

        //calculate number of hours that employee was working in that month  
        Task<DataTable> UkupnoSati_tsk = Task.Run(() => instanca.FillDataTable(
            "begin;select cast(radni_sati.sati_rada as time) from radni_sati where zaposlenik_id=@zaposlenik_id " +
            " and extract(month from radni_sati.datum)=@mjesec_broj and extract(year from radni_sati.datum)=@godina;commit;",
            CommandType.Text, listaParametara));


        // wait for all the tasks above to complete
        Task.WaitAll(
            iskoristeno_bolovanje_tsk, DatumiBolovanja_tsk, 
            iskoristeni_godisnji_tsk, DatumiGodisnjeg_tsk,
            redovi_tsk, UkupnoSati_tsk);



        // get the values after each of these return
        long iskoristeno_bolovanje = iskoristeno_bolovanje_tsk.Result;
        DataTable DatumiBolovanja = DatumiBolovanja_tsk.Result;

        long iskoristeni_godisnji = iskoristeni_godisnji_tsk.Result;
        DataTable DatumiGodisnjeg = DatumiGodisnjeg_tsk.Result;

        long redovi = redovi_tsk.Result;
        DataTable UkupnoSati = UkupnoSati_tsk.Result;


        listaParametara.Clear();
    }

It's not clean code as I'd prefer to refactor this into an Employee object with DaysSick() and SickDates() methods. However, lets see if it improves performance on this section.

For training purposes :)

The parameters are created once and shared across each call to instanca. From the original code this should fine.

If we take a query

      //return number of sick days employee had
        Task<long> iskoristeno_bolovanje_tsk = Task.Run(() => instanca.ExecuteScalar<long>(
            "begin;select count(*) from bolovanje where extract(month from bolovanje.datum)" +
            "=@mjesec_broj and extract(year from bolovanje.datum)=@godina and zaposlenik_id=@zaposlenik_id ;commit;", 
            CommandType.Text, listaParametara));

The call to TaskRun(() => instanca... will run asynchronously creating a task, all six queries will hit the database at the same time (just the way it's been coded)! I'd need to be with you running the code to know if that was suitable.

This code waits until each of the task finishes...

 // wait for all the tasks above to complete
    Task.WaitAll(
        iskoristeno_bolovanje_tsk, DatumiBolovanja_tsk, 
        iskoristeni_godisnji_tsk, DatumiGodisnjeg_tsk,
        redovi_tsk, UkupnoSati_tsk);

And the following code gets the results.

Be careful with this one. You could try it a little a time and see what happens.

\$\endgroup\$
  • \$\begingroup\$ sry for being away all this time, to much stuff to do...Now im back on coding this, so I'll reply when I implement this =) \$\endgroup\$ – Marko Petričević Oct 25 '17 at 8:15
  • \$\begingroup\$ Did it, works perfectly =) Thanks again for ur effort for helping me =) \$\endgroup\$ – Marko Petričević Oct 30 '17 at 13:00
  • \$\begingroup\$ @MarkoPetričević, hey man. Did the performance improve? \$\endgroup\$ – Derek Smyth Nov 4 '17 at 0:33
  • \$\begingroup\$ With all edits all I got was clean code (thanks for that), but in performance department I got a slight change, noting noticeable. \$\endgroup\$ – Marko Petričević Nov 6 '17 at 7:20
0
\$\begingroup\$

Wow... as far as code reviews go... this needs refactoring!

That initial if statement could be simplified with a look up table:

        string[] months = new[] { "Sijecanj", "Veljaca", "Ožujak", "Travanj", "etc"};

        var mjesec = "Sijecanj";
        var mjesec_broj = (Array.IndexOf(months, mjesec) + 1).ToString();

That would save a few lines of code. You could put it in an object to add context to the IndexOf() call.

I think the Strategy pattern might come in useful here also.

\$\endgroup\$
  • \$\begingroup\$ I would be grateful, If u find anything that can be done on a better way, Im here to learn =) P.S. This was written like 5 months ago and I know alot of stuff is just streched without taking care of clean code or faster execution. If u have any questions, feel free to ask \$\endgroup\$ – Marko Petričević Oct 12 '17 at 7:01
  • \$\begingroup\$ @MarkoPetričević some thoughts have crossed my mind. With the code being as complex as it is and since it is split across the post it's difficult to get an idea of it all. I'm seeing patterns. I'll post another answer in a while. \$\endgroup\$ – Derek Smyth Oct 12 '17 at 16:42
  • \$\begingroup\$ @MarkoPetričević the code is about gathering information about an employee. The code could be made much easier to read if employee object was created. Also I believe this code could be made to run asynchronously very easy also. Most of the queries could be ran asynchronously. \$\endgroup\$ – Derek Smyth Oct 12 '17 at 17:25
0
\$\begingroup\$

What we should do is simply the code and then look for performance gains. You are using version control? Any changes you make can be rolled back; right!?

The if-else statement in this code is redundant. Both clauses do the same thing. They may have been different before but they are the same now.

if (SatiRadaBezNapomene.Rows.Count > 0) //if that person even has worktime in this month
{
    //depending on the deparment_id
    if (odjel_id == 1 || odjel_id == 2 || odjel_id == 3 || odjel_id == 5 || odjel_id == 6)
    {
        //redovi = number of rows(days) person has been working + number of holidays + number of sickdays
        prosjek = Ukupno.TotalMilliseconds / (redovi + iskoristeni_godisnji + iskoristeno_bolovanje);
        upisProsjeka = TimeSpan.FromMilliseconds(prosjek).ToString(@"hh\:mm\:ss");
    }
    else if (odjel_id == 4 || odjel_id == 7 || odjel_id == 8 || odjel_id == 9 || odjel_id == 10)
    {
        prosjek = Ukupno.TotalMilliseconds / (redovi + iskoristeni_godisnji + iskoristeno_bolovanje);
        upisProsjeka = TimeSpan.FromMilliseconds(prosjek).ToString(@"hh\:mm\:ss");
    }
}

Becomes...

// if that person even has work time in this month
//   a person object may be required
bool HasWorkedOvertime = SatiRadaBezNapomene.Rows.Count > 0

if (HasWorkedOvertime) 
{
    //redovi = number of rows(days) person has been working + number of holidays + number of sickdays
    prosjek = Ukupno.TotalMilliseconds / (redovi + iskoristeni_godisnji + iskoristeno_bolovanje);
    upisProsjeka = TimeSpan.FromMilliseconds(prosjek).ToString(@"hh\:mm\:ss");
}
\$\endgroup\$
  • \$\begingroup\$ I love the idea, my only question is: I need to separate workers in their departments coz depending on department, they have different parameters, how to solve that problem? And yes, Im using ankhSVN \$\endgroup\$ – Marko Petričević Oct 12 '17 at 18:55
  • \$\begingroup\$ you can still separate workers as part of their department. The code that was changed didn't operate differently because of that department; the values of redovi + iskoristeni_godisnji + iskoristeno_bolovanje might have different values but they are always used the same in the code that changed. Be sure and use version control if you make changes. \$\endgroup\$ – Derek Smyth Oct 12 '17 at 19:12
  • \$\begingroup\$ Just tested it, it works. One less part of the code, thanks for ur effort =) \$\endgroup\$ – Marko Petričević Oct 12 '17 at 20:10

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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