3

When running the below method in my application the app freezes and when I pause VS it seems to be stuck on the line that goes:

SqlDataReader reader = select.ExecuteReader();

I've got other SQL methods running fine so I know the connection string is correct, I've double checked the SQL and that's fine. Am I wrong in think the reader variable can not contain the returning value of the scalar function when the ExecuteReader() is called?

public static bool AccountValidation(string username, string password)
{
    string statement = "select dbo.AccountValidation('" + username + "','" + password + "')";
    SqlCommand select = new SqlCommand(statement, connect);
    connect.Open();
    SqlDataReader reader = select.ExecuteReader();
    string result = reader.ToString();
    connect.Close();

    if (result != "true")
    {
        return false;
    }
    else
    {
        return true;
    }
}
Improve this question
4
  • 2
    Slightly unrelated but you really should consider parameterizing your query as your current approach could potentially leave you vulnerable to nastiness like SQL Injection. Commented Jun 14, 2017 at 18:58
  • What do you expect reader.ToString() to do? Did you mean to do select.ExecuteScalar() instead? Commented Jun 14, 2017 at 19:00
  • Im not the first to say it but to reiterate, Parameterize your queries. Your code is potentially vulnerable to SQL Injection attacks. You should also consider wrapping objects that implement IDisposable in using statements. Commented Jun 14, 2017 at 19:01
  • what is dbo.AccountValidation? a user-defined function, returning a scalar, or a table? you seem to expect it to return the string "true", in this case use it with the ExecuteScalar function. If you use a DataReader, you need to call its Read() method at least once to fetch. Commented Jun 14, 2017 at 19:03

3 Answers 3

Reset to default
5

The main problem is that you are not actually reading anything back from the data reader, you have to iterate over the result set and then read based on ordinal/positional index.

There are also other big problems like

  • Not using parameters which leaves the code vulnerable to sql injection attacks.
  • Not wrapping your disposables in using blocks which could leave database connections open if there are exceptions
  • sharing db connections in your types. Create connections on an as needed basis and then dispose them when you are done.

Here is your updated code with fixes. I guessed at the column types (varchar), fix that and the lengths as they are implemented in your schema.

public static bool AccountValidation(string username, string password)
{
    const string statement = "select dbo.AccountValidation(@username, @password)";
    string result = null;

    // reference assembly System.Configuration
    string connStr = System.Configuration.ConfigurationManager.ConnectionStrings["YourDb"].ConnectionString;

    using(var connection = new SqlConnection(connStr))
    using(SqlCommand cmd = new SqlCommand(statement, connect))
    {
        cmd.Parameters.Add(new SqlParameter("@username", SqlDbType.VarChar, 200){Value = username});
        cmd.Parameters.Add(new SqlParameter("@password", SqlDbType.VarChar, 200){Value = password});
        connect.Open();
        using(SqlDataReader reader = cmd.ExecuteReader())
        {
            if(reader.Read())
                result = reader.GetString(0); // read back the first column of the first row
        }
    }
    if (result != "true")
    {
        return false;
    }
    else
    {
        return true;
    }
}

On a side note it would be cleaner to return a bit from your database function AccountValidation and then read that back with reader.GetBoolean(0) and assign that to the result and return that directly instead of doing string comparisons.

Also, as mentioned above in the comments, if you are only returning 1 value it is easier (and less code) to call ExecuteScalar instead of ExecuteReader.

Improve this answer
Sign up to request clarification or add additional context in comments.

1 Comment

Besides the redundant if statement at the bottom (thats on OP not you), this is the perfect answer.
2

Add the line 

reader.Read();

before the line

string result = reader.ToString();

Also, please parameterize your queries.

Improve this answer

1 Comment

reader.ToString() will not return the first ordinal result. Most likely it will just return the default full type name. reader.GetString(0) will.
0

I don't have enough reputation to leave a comment, but to answer part of your question, yes, you can use a SqlDataReader to read single scalar results with or without column aliases.

Improve this answer

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.