-2

I have a little problem with my codes. I'm trying to make my program faster, because it have too big delay, when i'm getting data from mysql and need to make my code faster. Can you help me about this code, is correctly to select * in table and is it good idea? Thank you! It's my code!

       public bool GettingPlayers(ref clsConnection c)
    {
        try 
        {
            MySqlConnection connect = new MySqlConnection(connectionMysql);
            connect.Open();
            MySqlCommand query = new MySqlCommand("SELECT * FROM Users Where Username='" + Escape(c.Username) + "'", connect);
            query.Prepare();
            MySqlDataReader dr = query.ExecuteReader();
            if (dr.Read())
            {
                c.Username = dr[1].ToString();
                c.Cash = double.Parse(dr[2].ToString());
                c.TotalDistance = double.Parse(dr[3].ToString());
                c.TotalHealth = double.Parse(dr[4].ToString());
                c.Password = dr[5].ToString();
                c.Status = int.Parse(dr[6].ToString());
                c.IpAdress = dr[7].ToString();
                c.TotalJobsDone = int.Parse(dr[8].ToString());

            }
            else
            {
                dr.Close();
                connect.Close();
                return false;
            }
            dr.Close();
            connect.Close();
            return true;
        }
        catch (Exception ex)
        {
            Form1 frm1 = new Form1();
            frm1.LogTextToFile("sqlError", "GettingPlayers - " + ex);
            return false;
        }
    }

Also I have a public string Escape(string str) => str.Replace("'", "\'").Replace(""", "\"");

  • `SELECT *` is never a good idea, mention the columns – Vivek Nuna Jul 25 '20 at 13:03
  • `'" + Escape(c.Username) + "'",` Do **not** do this. https://stackoverflow.com/questions/14376473/what-are-good-ways-to-prevent-sql-injection . Also, I'd strongly suggest reading up on Dapper - it will reduce the size of the code block by around 70% I suspect. And be easier to read and understand. – mjwills Jul 25 '20 at 13:04
  • 1
    Entity is a faster interface MySqlConnection, Use in VS new DataSource and then select your database. Entity will create classes in c# to match the database tables. The low level interface is faster in Entity. – jdweng Jul 25 '20 at 13:05
  • Do you mean Entity Framework @jdweng? Or https://dev.mysql.com/doc/dev/connector-net/6.10/html/N_MySql_Data_Entity.htm? Or something else? – mjwills Jul 25 '20 at 13:06
  • I'd suggest using `using` blocks to make sure that everything (including `query`) is disposed correctly (and to allow you to remove your explicit `Close` calls). – mjwills Jul 25 '20 at 13:07
  • How many rows are being returned? How long is it taking? Please show us the script to create the table (and its indexes). – mjwills Jul 25 '20 at 13:08
  • `c.Cash = double.Parse(dr[2].ToString());` This is quite odd - taking what I presume is a number (from the database), converting it to a string then _back_ to a number again. – mjwills Jul 25 '20 at 13:11
  • @jdweng - I don't know Entity and don't know how to write and make my code on Entity... – Djani_Bulgaria Jul 25 '20 at 13:12
  • @mjwills - Can you give me example with using blocks how to make my code? Thank you! Aslo the table is created ones myself without code. Example if in program are working 10 users delay i 30sec-1min if only me delay is 1sec... So `c.Cash = double.Parse(dr[2].ToString());` how to make it? yes its number, but how to get it from table in other way? – Djani_Bulgaria Jul 25 '20 at 13:13
  • The answer below shows how to use `using`. If you show us the scripts for the table we can show you how to avoid the need for `double.Parse`. If performance is scaling like that, I'd say there is a good chance you are missing appropriate indexes. I'd also suggest commenting out `query.Prepare();`. – mjwills Jul 25 '20 at 13:19
  • @mjwills Thank you for your support! `public bool GetttingPlayers(ref clsConnection c, string Username) { if (Username != "") { return SQL.GettingPlayers(ref c); } else { return true; } }` – Djani_Bulgaria Jul 25 '20 at 13:24
  • See https://learn.microsoft.com/en-us/ef/ Same as your link just more info. – jdweng Jul 25 '20 at 14:15

1 Answers1

-2

You should avoid using the SELECT * in the query instead use the columns. I would suggest you use the stored procedure.

You are closing the connection twice it's unnecessary.

You should use the while loop in place of the if statement.

YOu can refer to the below code, I have not tested it but it will give you a good idea.

using (MySqlConnection connect = new MySqlConnection(connectionMysql))
{
    connect.Open();
    using (MySqlCommand query = new MySqlCommand("SELECT * FROM Users Where Username='" + Escape(c.Username) + "'", connect))
    {
        using (MySqlDataReader dr = query.ExecuteReader())
        {
            while (dr.Read())
            {
                c.Username = dr.GetString(1);
            }
        }
    }                                 
}

Few more suggestions.

Please use parameterized query to avoid the SQL injection, It will also improve the performance because the database can cache the execution plan.

you can use connection pooling instead of opening and closing the connections if you are making many calls.

you can use async/await to make the DB calls, this can improve the responsiveness of the application.

Vivek Nuna
  • 25,472
  • 25
  • 109
  • 197