C# method with parameters

Question

I've written some code to check whether a given subject name exists in my SQL database. Please see below:

string exists = String.Empty;
using (SqlConnection connection = new SqlConnection(connectionString))
{
   connection.Open();
   using (SqlCommand command = new SqlCommand("SELECT Subject_Name FROM Subject WHERE Subject_Name ='" + input + "'", connection))
   {
     using (SqlDataReader reader = command.ExecuteReader())
      {
            while (reader.Read())
            {
                for (int j = 0; j < reader.FieldCount; j++)
                 {
                       exists = reader.GetValue(j) + "";
                 }
            }
      }
   }
}

note: connectionString has already been declared above in my program

I am using this code in a few methods and don't want to have duplicated code, therefore I am thinking of making this into a method.

Problem lies in that the next piece of code in my methods in which i use the above code uses the exists string and checks whether or not it is empty.

Pseudo code below:

if(String.ISNullOrEmpty(exists))
{
//do some code
}
else
{
//do some code
}

I have made the method as follows:

private static string SubjectExistsChecker(string input, string exists)
        {
            using (SqlConnection connection = new SqlConnection(connectionString))
            {
                connection.Open();
                using (SqlCommand command = new SqlCommand("SELECT Subject_Name FROM Subject WHERE Subject_Name ='" + input + "'", connection))
                {
                    using (SqlDataReader reader = command.ExecuteReader())
                    {
                        while (reader.Read())
                        {
                            for (int j = 0; j < reader.FieldCount; j++)
                            {
                                exists = reader.GetValue(j) + "";
                            }
                        }
                    }
                }
                return exists;
            }
        }

I then call the method as follows:

MethodName()
{
//some code asking for subjectName which we call `input`

 String exists = string.Empty;
 SubjectExistsChecker(input, exists);


 if (string.IsNullOrEmpty(exists))
{
//do some code
}
else
{
//do some code
}

When writing the code out fully (not using method) as shown at the top, this works.

Now that I've made the method, it doesn't work and exists stays null.

Can anyone help? Where am I going wrong? I'm guessing its something to do with the exists string.

Because you never assign the result your method returns to any variable. Do `exists = SubjectExistsChecker(input, exists);` and (optionally) remove the `string exists` parameter and declare the variable locally. Or make the method of type `void`, remove the return and add a `ref` before the `string exists` parameter, to modify the variable you passed as `exists` to the method. — Tobias Tengler, Dec 12 '18 at 10:38
Aside: use Dapper instead. There's no good reason to mess around with `SqlDataReader` yourself, especially not if you're going to use it in this way. This will also take care of parameterization, another issue that you currently have. And last but not least: use `IF EXISTS` or `SELECT CASE WHEN ... THEN 1 ELSE 0 END` in your *query*, don't make the C# code do all the heavy lifting -- that's what your database is for. — Jeroen Mostert, Dec 12 '18 at 10:40

score 3 · Answer 1 · edited Dec 12 '18 at 10:42

Firstly, it is a bad practice to directly concatenate SQL strings due to SQL Injection security issues.

You could and should use a parameterized query, and set the value in the parameters.

Example here: https://learn.microsoft.com/en-us/dotnet/api/system.data.sqlclient.sqlcommand.parameters?view=netframework-4.7.2

Secondly. You are not assigning the string to your exists variable outside of the scope of the SubjectExistsChecker method. Strings are immutable.

This should work for your case:

 String exists = string.Empty;
 exists = SubjectExistsChecker(input, exists);

score 1 · Answer 2 · answered Dec 12 '18 at 10:40

Because you are using String and they are immutable in their nature though they are reference types.

You will need to return the string and assign back to your variable:

String exists = string.Empty;
exists = SubjectExistsChecker(input, exists);

and looks like it will be a single record, if yes you don't need to do a while loop.

score 1 · Accepted Answer · answered Dec 12 '18 at 10:41

1

You are not setting your return value:

Add this:

exists = SubjectExistsChecker(input, exists);

answered Dec 12 '18 at 10:41

Tarik Tutuncu

790
4
12

score 1 · Answer 4 · answered Dec 12 '18 at 11:02

The problem is that you're not using the value that you compute in your SubjectExistsChecker(). In order to use what you return from within a method, you could perform an assignment upon the call to the method, like so:

String exists = SubjectExistsChecker(input, exists);

But if we go this way, we don't really need to supply it as a parameter to the method - because if you look at it, nothing depends on this parameter. Also note that we tend to name methods with verbs - as it reflects their nature. For example, your method could be named DoesSubjectExist. So you could adjust the method like so:

private static string DoesSubjectExist(string input)
{
    string exists = "";
    using (SqlConnection connection = new SqlConnection(connectionString))
    {
        connection.Open();
        using (SqlCommand command = new SqlCommand("SELECT Subject_Name FROM Subject WHERE Subject_Name ='" + input + "'", connection))
        {
            using (SqlDataReader reader = command.ExecuteReader())
            {
                while (reader.Read())
                {
                    for (int j = 0; j < reader.FieldCount; j++)
                    {
                        exists = reader.GetValue(j) + "";
                    }
                }
            }
        }
        return exists;
    }
}

But still it is unclear what did you want the result of this method to be? What we can guess from the name, we want to check if a subject exists - we don't have to know what it is. In that case, it would make sense that the method returns either true if subject with given name exists or false otherwise. So, with further adjustments:

private static bool DoesSubjectExist(string input)
{
    using (SqlConnection connection = new SqlConnection(connectionString))
    {
        connection.Open();
        // Note that query will probably need to be changed too for a more optimal solution.
        using (SqlCommand command = new SqlCommand("SELECT Subject_Name FROM Subject WHERE Subject_Name ='" + input + "'", connection)) 
        {
            using (SqlDataReader reader = command.ExecuteReader())
            {
                // Do we need this loop?
                while (reader.Read())
                {
                    // There is at least one match - so the subject exists.
                    if (reader.FieldCount > 0) return true;
                }
            }
        }
        return false; // If we get here, we didn't find anything.
    }
}

So this will be much more expressive. Note your use case:

MethodName()
{
    //some code asking for subjectName which we call `input`

    if (DoesSubjectExist(input))
    {
        //do some code
    }
    else
    {
        //do some code
    }
}

Also, please take the advice other people here are giving and abstract your SQL data access somehow.

C# method with parameters

4 Answers4