11
\$\begingroup\$

When I realized that the code below has a major problem in what it'll do (it'll just update the database with information currently in the database), I was told that there are a lot of mistakes.

Please point out the mistakes to me, suggest improvements and how to avoid the mistakes in the future.

public User Get(int UserID)
{
    string sql = "select * from users where userid = :id";

    List<MySqlParameter> args = new List<MySqlParameter>();
    args.Add(new MySqlParameter("id", UserID));

    MySqlCommand cmd = new MySqlCommand(sql, db.conn);
    cmd.Parameters.AddRange(args.ToArray());

    db.createConnection();
    MySqlDataReader dr = cmd.ExecuteReader();
    db.closeConnection();

    User user = new User();

    if (dr.HasRows)
    {
        dr.Read();

        user.EmailAddress = (string)dr["email"];
        user.Password = (string)dr["passcode"];
    }

    return user;
}

public void  Update(int UserID)
{
    User user = Get(UserID);

    string sql = "update users set email = :email, passcode = :pass where userid = :id";

    List<MySqlParameter> args = new List<MySqlParameter>();
    args.Add(new MySqlParameter(":email", user.EmailAddress));
    args.Add(new MySqlParameter(":pass", user.Password));
    args.Add(new MySqlParameter("id", UserID));

    MySqlCommand cmd = new MySqlCommand(sql, db.conn);
    cmd.Parameters.AddRange(args.ToArray());

    db.createConnection();
    cmd.ExecuteNonQuery();
    db.closeConnection();
}
\$\endgroup\$
2
  • 4
    \$\begingroup\$ This code uses .NET 1.0 technology. You can still learn and use that but step quickly into Entity Framework. \$\endgroup\$ Jul 30, 2012 at 15:16
  • \$\begingroup\$ A couple of quick comments. 1) It's generally a good idea to wrap any object, which can be cast to IDisposable, into using(..){ } statement. 2) Your select statement "select * from users where userid = :id" is too implicit, I'd recommend to use "select field1, field2..." type of statement as a more explicit way. In that case changes to database schema will cause your command to fail after schema changes and it will be easier to catch bugs, related to schema changes. \$\endgroup\$
    – Dmitriy
    Jul 31, 2012 at 9:02

2 Answers 2

12
\$\begingroup\$

I'm not sure why you're using your own built classes to access the database when .net has a very complete set of classes that can be used for data access.

Just looking at your syntax, I see the same functionality, so to me that is a waste of development time.

Also, it seems while I don't have access to your classes locally, I've changed them to use the included .net classes.

EDIT: Changed dr.GetString("") to dr.GetString(dr.GetOrdinal(""))

using System.Data.SqlClient;

public User Get(int userID)
{
    const string sql = "select * from users where userid = @id";
    const string connnectionString = "<your connection string>";

    // Using will dispose of the connection when the inner statements 
    // have completed executing.  The dispose actually closes the connection.
    // The class has to implement the IDisposable interface.
    // var is a strongly typed variable, mostly used to keep the code easy to read.
    using(var connection = new SqlConnection(connnectionString))
    {
        connection.Open();

        // Make sure the SqlCommand object is disposed
        using (var cmd = new SqlCommand(sql, connection))
        {
            // No need to use a separate list to add parameters, just
            // add them directly to the command.
            cmd.Parameters.Add(new SqlParameter("@id", userID));

            var dr = cmd.ExecuteReader();
            var user = new User();

            // Read returns a boolean if it reads a record.
            if (dr.Read())
            {
                user.EmailAddress = dr.GetString(dr.GetOrdinal("email"));
                user.Password = dr.GetString(dr.GetOrdinal("passcode"));
            }
            return user;
        }
    }
}

public void Update(int userID)
{
    const string sql = "update users set email = @email, passcode = @pass where userid = @id";
    const string connnectionString = "<your connection string>";

    var user = Get(userID);

    using (var connection = new SqlConnection(connnectionString))
    {
        connection.Open();

        using (var cmd = new SqlCommand(sql, connection))
        {
            cmd.Parameters.Add(new SqlParameter("@email", user.EmailAddress));
            cmd.Parameters.Add(new SqlParameter("@pass", user.Password));
            cmd.Parameters.Add(new SqlParameter("@id", userID));

            cmd.ExecuteNonQuery();
        }
    }
}
\$\endgroup\$
8
1
\$\begingroup\$

Besides the suggestions from @JeffVanzela , I would suggest to use object initializer syntax when building lists:

var foo = new List<string>() {
    "item 1",
    "item 2",
    "item 3", // Comma at the end of the last item allows swapping items around easily.
};

This also works for dictionaries:

var bar = new Dictionary<int, string>() {
    {1, "item 1"},
    {3, "item 2"},
    {5, "item 3"},
};
\$\endgroup\$

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