2
\$\begingroup\$

I'd created a C# class to connect to a MySQL database (Connection.cs) and a separate class for the user validation (User.cs).

Connection.cs

public static MySqlConnection GetConnection()
{
    try
    {
        var connectionString = ConfigurationManager.ConnectionStrings["ConnectionString"].ConnectionString;
        _connection = new MySqlConnection(connectionString);
        _connection.Open();
    }
    catch (MySqlException ex) {

        switch (ex.Number)
        {
            //0: Cannot connect to server.
            case 0:
                MessageBox.Show("Cannot connect to server.  Contact administrator");
                break;
            //1045: Invalid user name and/or password.
            case 1045:
                MessageBox.Show("Invalid username/password, please try again");
                break;
        }

    }

    return _connection;
}

User.cs

public bool AuthenticateUser()
{
    string query = "SELECT * FROM users WHERE user_name = @userName LIMIT 1";
    bool password = false;
    try
    {
        Console.Write("connection");
        using (MySqlConnection connection = Connection.GetConnection())
        {
            Console.Write("Cmd");
            using (var cmd = new MySqlCommand(query, connection))
            {
                cmd.Parameters.AddWithValue("@userName", this.UserName);
                using (var reader = cmd.ExecuteReader())
                {
                    while (reader.Read())
                    {
                        password = BCrypt.Net.BCrypt.Verify(this._password + reader["salt"].ToString(), reader["password"].ToString());
                        this._user_id = int.Parse(reader["id"].ToString());
                    }
                }
            }
        }
    }
    catch (MySqlException ex)
    {
        throw new ArgumentException(ex.Message);
    }
    catch (Exception ex) {
        throw new ArgumentException(ex.Message);
    }

    return password;

}

I didn't created a method to dispose/close the connection anymore, since as far as I know, the using statement automatically disposes the type of IDisposable. I was looking for a suggestion before I continue coding for more functionalities.

\$\endgroup\$
  • 2
    \$\begingroup\$ Can you add a little more context as to what you're using this for? It feels a little light for a review. \$\endgroup\$ – Dannnno Feb 28 '18 at 13:32
  • \$\begingroup\$ Have you considered using something like Dapper \$\endgroup\$ – Nkosi Mar 1 '18 at 21:25
2
\$\begingroup\$

A couple of issues I found:

GetConnection method:

var connectionString = ConfigurationManager.ConnectionStrings["ConnectionString"].ConnectionString;
_connection = new MySqlConnection(connectionString);
_connection.Open();

Here you're creating a new connection object and storing it in _connection. This is not defined anywhere in the code, but seems like a private field. This shouldn't be necessary, as you're simply returning it and leaving it to the caller to handle it. Just use a normal variable and get rid of the field:

MySqlConnection connection = new MySqlConnection(connectionString);
connection.Open();
return connection;
    catch (MySqlException ex) {

        switch (ex.Number)
        {
            //0: Cannot connect to server.
            case 0:
                MessageBox.Show("Cannot connect to server.  Contact administrator");
                break;
            //1045: Invalid user name and/or password.
            case 1045:
                MessageBox.Show("Invalid username/password, please try again");
                break;
        }

    }

This seems like a bad idea to me. For one, you're using MessageBox.Show to display errors, which ties it to the presentation layer. This breaks Separation of concerns, as the class should only do one thing. Leave it to the caller to display errors the way it likes.

Another issue is that any further detail about the error is lost, like the stack trace, message or any inner exceptions. A second, major issue is that any error with a number other than 0 or 1045 is simply shallowed and the caller has no idea it failed, leaving it with a closed, failed connection. My suggestion is to simply remove the try-catch althogether and make the caller handle errors as it finds appropriate.

AuthenticateUser method:

string query = "SELECT * FROM users WHERE user_name = @userName LIMIT 1";

A small issue with the LIMIT 1 is that it implies that there could be more than one user in the DB with the same username. That can be an indication of an issue on the DB design, lacking an UNIQUE key in the user_name column, it shouldn't be needed. Another problem is the use of SELECT * which brings more data than needed. Specify those columns you actually use and leave the rest out.

Console.Write("connection");

This is odd to put here. For one, other pieces of code refer to MessageBox to display errors and messages, while this uses the console. Why this inconsistency? Either use one display technology or the other, but clearly not both. As before, this creates a poor user experience in seeing random debug messages out of nothing. Most likely, this could be replaced by some kind of log to a file or similar, but remember to add more context to it, so you can extract some useful information out of the file.

cmd.Parameters.AddWithValue("@userName", this.UserName);

It's always nice to see properly parametrized queries. But remember that AddWithValue is a bad practice, declare the exact data type for the parameter and make sure it matches the server side declaration exactly.

while (reader.Read())

There is no need to loop here. As before, the query will return at most only 1 row, or nothing is the user name is wrong, so just replace it by a single call. if (!reader.Read()) return false; will do, and will exit early is no user exist.

password = BCrypt.Net.BCrypt.Verify(this._password + reader["salt"].ToString(), reader["password"].ToString());

Great to see a properly stored password and a nice validation! There is a small thing though. Keep the scope of the password variable as small as possible, possibly with a return at this point to avoid having to read until the end of the method. Also, "password" don't seems like a proper name for this one, something like "validPassword" would better describe what's going on.

this._user_id = int.Parse(reader["id"].ToString());

This is possibly the gravest problem. The method uses class fields all other the place (to input the username and password and to return the user id), which seems odd to me, because that ties the result of this method to the state of the class, which can be used elsewhere (although we don't have that much context here). Why not use method parameters and return values instead? So you can get rid of all private class-level variables. I find a method declaration like this to be much better:

public int? AuthenticateUser(string userName, string password)

The username and password are passed as parameters instead of class fields, and the return value is the id of the logged user or null if nothing is found. This clearly expresses what's going on and what the method expects and return. Also, this allows to simplify the body by doing something like this:

//Check if the password matches, and exit if not
if(!BCrypt.Net.BCrypt.Verify(this._password + reader["salt"].ToString(), reader["password"].ToString())) return null;
//We've got a valid user and password, return its id
return int.Parse(reader["id"].ToString());
   catch (MySqlException ex)
    {
        throw new ArgumentException(ex.Message);
    }

Once again the same problem. The exception is caught and a new one is thrown. But here the problem is that it's misleading. For one, the type ArgumentException is wrong, you didn't make a mistake in the arguments, something blew up while contacting the DB. Moreover, you keep only the message, but you discard the stacktrace, any inner exception and DB-specific exception information which can help debugging. This is will likely give you problems later on. My suggestion is to simply remove the try-catch and leave it to the caller to handle the exception and log, display something or retry. At most, throw a specific exception adding some context, but keep all of the original exception as an inner exception.

\$\endgroup\$
1
\$\begingroup\$

The Method returns an IDisposable Object (MySqlConnection), so you are right that you do not need to worry about disposing of the connection when you are using the Using statement.


As far as you are using connectionString you don't actually need it here:

try
{
    var connectionString = ConfigurationManager.ConnectionStrings["ConnectionString"].ConnectionString;
    _connection = new MySqlConnection(connectionString);
    _connection.Open();
}

Just put the string in as a parameter

try
{
    _connection = new MySqlConnection(ConfigurationManager.ConnectionStrings["ConnectionString"].ConnectionString);
    _connection.Open();
}

it makes that line a little longer, but doesn't waste the time of assigning to a variable and then calling the value of the variable. If you were using it more than just right there, I would agree that you should assign it to a variable.

For the sake of readability, you could do this as well.

try
{
    _connection = new MySqlConnection(
        ConfigurationManager
            .ConnectionStrings
            ["ConnectionString"]
            .ConnectionString
    );
    _connection.Open();
    return _connection;
}

I also moved the Return so that it was inside the try statement, there isn't any reason to leave it out there all by itself. you could get an exception on the return, granted it would not be a SQL exception but it is still good to get into the habit of keeping code together, the Catch shouldn't separate this could, there does not seem to be a reason for it.

\$\endgroup\$
  • 2
    \$\begingroup\$ I disagree with the second suggestion. Time won't be a factor here, as it's a single operation and the time opening the DB connection is likely to be orders of magnitude longer than a memory operation. Moreover, the compiler will possibly optimize it and get rid of the variable anyway. Readability is far more important. \$\endgroup\$ – Alejandro Mar 3 '18 at 13:55
  • \$\begingroup\$ There is no change to readability here. the time wasted is the coder's time, not compilers time. the variable is not necessary, and anything that is not necessary is usually clutter. \$\endgroup\$ – Malachi Mar 4 '18 at 14:44
  • 1
    \$\begingroup\$ Longer lines like that are often creating readability problems, not the number of lines. The variable is not strictly necessary, it's the compiler job to get rid of it and most likely it knows how to optimize. I still disagree with the suggestion, you spend code writer's time in order to save code readers' time. It's giving things a name, not clutter. \$\endgroup\$ – Alejandro Mar 4 '18 at 23:42
  • 2
    \$\begingroup\$ that isn't a long line, if it were in an IDE you wouldn't even have to scroll right to see the entire thing, there is a difference of 12 characters between the original declaration of the variable and the line that I created without it. I think we are falling into a personal preference thing rather than a code review. Removing extraneous variables in your code in good practice. and that is my point here, there is no loss to readability here. \$\endgroup\$ – Malachi Mar 5 '18 at 13:54
0
\$\begingroup\$

You should null the user_id

bool password = false;
this._user_id = null;

match would be a better name than password

You should select column names rather than * and refer to them by ordinal position for better efficiency.

\$\endgroup\$
  • 2
    \$\begingroup\$ why null the user_id? \$\endgroup\$ – Malachi Mar 1 '18 at 15:44
  • \$\begingroup\$ @Malachi What if the user is not found? \$\endgroup\$ – paparazzo Mar 1 '18 at 15:50
  • \$\begingroup\$ that should be set in the property declaration and not in one of the methods. \$\endgroup\$ – Malachi Mar 1 '18 at 17:16
  • \$\begingroup\$ @Malachi But the fact is that it IS set in the method. If that is a problem with the current method then why did you not point that out in your answer? \$\endgroup\$ – paparazzo Mar 1 '18 at 17:18

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.