Nsryjdtyk

I have this code that returns a client object, but to do it I use two methods.

How can I improve this to be more efficient and ensure it meets common standards?

    //Method that returns a client from a string

    private static Cliente ObtenerCliente(string query)

    {

        try

        {

            abrirConexion(); //Static method that opens a connection



            cmd = new MySqlCommand(query, mySqlConexion);

            MySqlDataReader dataReader = cmd.ExecuteReader();



            if (dataReader.Read())

            {

                return new Cliente(

                    Convert.ToInt32(dataReader["id_cliente"]),

                    dataReader["nombre_cliente"].ToString());

            }

            else

            {

                dataReader.Close();

                return null;

            }

        }

        catch (MySqlException e)

        {

            MessageBox.Show(e.ToString());

            return null;

        }

        finally

        {

            cerrarConexion(); //Static method that closes the connection

        }

    }



    //Method that returns a customer from a phone number given

    public static Cliente ObtenerCliente_Telefono(string telefono)

    {

        string query = "SELECT * FROM clientes WHERE (telefono1_cliente = '" + telefono + "' OR telefono2_cliente = '" + telefono + "');";

        return ObtenerCliente(query); //Metodo que hace la consulta

    }

    public static Cliente ObtenerCliente_Id(int id)

    {

        string query = "SELECT * FROM clientes  WHERE id_cliente = " + id + ';';

        return ObtenerCliente(query);

    }

Nothing wrong with Cliente ObtenerCliente accepting the query text.

Suffers from non-deterministic. You are only going to get the first but without an order by then SQL will just pick one for you.

using is a better approach in my opinion

For sure I don't like

Static method that opens a connection

Create the connection and let it be properly disposed

private static Cliente ObtenerCliente(string query)

{

    using (Connection mySqlConexion = new Connection(conString))

    {

        try

        {

            //abrirConexion(); //Static method that opens a connection

            mySqlConexion.Open();

            using (cmd = new MySqlCommand(query, mySqlConexion))

            {

                using (MySqlDataReader dataReader = cmd.ExecuteReader())

                {                    

                    if (dataReader.Read())

                    {

                        return new Cliente(

                        Convert.ToInt32(dataReader["id_cliente"]),

                        dataReader["nombre_cliente"].ToString());

                    }

                    else

                    {

                        //dataReader.Close();  let the using dispose

                        return null;

                    }

                }

            }

        }

        catch (MySqlException e)

        {

            MessageBox.Show(e.ToString());

            return null;

        }

        finally

        {

             //cerrarConexion(); //Static method that closes the connection

        }

    }

}

Extending paparazzo's answer, I would use a tertiary to return the result. I also like the descriptiveness of default rather than null:

return dataReader.Read()

? new Cliente(Convert.ToInt32(dataReader["id_cliente"]),

                    dataReader["nombre_cliente"].ToString())

    : default(Cliente);

Also consider using Top(1) in the queries, together with the order by suggestion, as the code only processes one row