C# sending select command from one class to another and return result?

Question

I am beginner in C#, so this question may be a bit stupid but...

I would like to send some SQL command as string from one class to another, execute and then return result to the first class.

    class MainWindow
private void btnLogin_Click(object sender, RoutedEventArgs e)
        {

            TestConnection checkLogin = new TestConnection();
            checkLogin.SimpleQuery("SELECT Name FROM Names WHERE ID='1'", "");

            //MessageBox .Show(checkLogin.SimpleQuery(response:ToString));
        }

And class TestConnection

  public string SimpleQuery(string request, string response)
    {


        using (SqlConnection conn = new SqlConnection())
        {
            conn.ConnectionString = "Server=SQLOLEDB.1;User ID=" +
                Constants.DATABASE_USERNAME + ";Password=" +
                Constants.DATABASE_PASSWORD + ";Initial Catalog=" +
                Constants.DATABASE_CATALOG + ";Data Source=" +
                Constants.SERVER_ADRESS;

            SqlCommand command = new SqlCommand(request, conn);

            {
                conn.Open();
                response = Convert.ToString (command.ExecuteScalar());
                conn.Close();
                return response; 
            }

        }

Is something like this even an OK idea? I am learning and I am testing ideas..

Thank you!

Fine as a concept. NB: you've defined `response` as a parameter on your method: `SimpleQuerry(string request, string response)`. Do you envisage ever passing in a response? If not, amend this code to `SimpleQuerry(string request)`, then place `string response;` inside the method. Alternatively you can get rid of `string reponse` completely if you use `return Convert.ToString (command.ExecuteScalar());`. Though that would exit the method before you called `conn.Close()` you'd still be OK, since your `using` statement ensures that the connection would be correctly disposed of. — JohnLBevan, Oct 03 '18 at 13:14
This "pattern" is not handy in that you have to use string concatenation to execute a sql statement with user input (values) which is frowned upon (to put it mildly). See also [bobby tables](https://xkcd.com/327/). Sql connection strings are also best kept in the `app.config` and retrieved by name. — Igor, Oct 03 '18 at 13:16
See also https://stackoverflow.com/q/35163361/1260204 for how to use parameters in ado.net. — Igor, Oct 03 '18 at 13:26
Also if you want to create an abstraction layer for your DB I would strongly consider using an off the shelf ORM framework like Entity Framework or Dapper (there are others as well). — Igor, Oct 03 '18 at 13:28
Rather than adding another answer to the already existing & informational answers, I'll just post here. On top of what Igor has mentioned for ORM's, I'd also recommend looking at the [Repository pattern](https://medium.com/falafel-software/implement-step-by-step-generic-repository-pattern-in-c-3422b6da43fd), which is what Stefan & Igor use in their answers. The Repository pattern is a good way of separating concerns and abstracting any entity logic behind a reusable & testable component. — ColinM, Oct 03 '18 at 14:16

Stefan · Accepted Answer · 2018-10-03T13:38:54.747

The thing is, usually you want your classes to encapsulate functionality.

In your case, it makes more sense if you keep the query in the object and expose a method which name correspond with the actual functionality, e.g.:

//your class, also: the name must describe it's reason for existence
public class UserRepository { //previously known as TestConnection

//the method name explains its function.
//the query is stored within the function
//therefore the functionality is encapsulated
public string CheckLogin(int id)
{
    //note: tricky: almost SQL injection here: that must be fixed.
    //I just left it here so you can see the basic idea
    var request = "SELECT Name FROM Names WHERE ID ="  + id.ToString();

    //another note: response doesn't have to be passed as parameter.
    var response = string.Empty;
    using (SqlConnection conn = new SqlConnection())
    {
        conn.ConnectionString = "Server=SQLOLEDB.1;User ID=" +
            Constants.DATABASE_USERNAME + ";Password=" +
            Constants.DATABASE_PASSWORD + ";Initial Catalog=" +
            Constants.DATABASE_CATALOG + ";Data Source=" +
            Constants.SERVER_ADRESS;

        SqlCommand command = new SqlCommand(request, conn);
        {
            conn.Open();
            response = Convert.ToString (command.ExecuteScalar());
            conn.Close();
            return response; 
        }
    }

There are some further enhancements which can be made, but for now I think this will give you enough to think about.

SQL injection issue (see comment)

To prevent SQL injection, a good approach is to use parameters. There is an article about it here.

Basically it come down on using parameterized inputs:

disclaimer: copied from link:

 using (SqlCommand command = 
              new SqlCommand("SELECT * FROM Dogs1 WHERE Name LIKE @Name", connection))
 {
     // Add new SqlParameter to the command.
     command.Parameters.Add(new SqlParameter("Name", dogName));

For more information about how and why, see this link.

Another benefit here is that it's keeping any database code, or SQL statements, out of the presentation layer where it could possibly be susceptible to injection attack depending on the level of sanitizing performed on the input, plus there's the obvious re-usability benefit across multiple frameworks (WinForms, WPF, ASP.NET etc). Though on second inspection, I now see your SQL Injection comment in code — ColinM, Oct 03 '18 at 13:12
You did nothing to address the use of string concatenation to build a sql statement. This *can* lead to sql injection vulnerabilities in the code. At the very least you should include this as a statement, how to circumvent it and why it is important to use parameters. Considering the OP has little/no experience with ado.net (hence the question) IMO this is an important topic to address in an answer to this question as the OP might take this answer as an acceptable pattern for all future queries (and thus could be passing in strings as paremeters as well). — Igor, Oct 03 '18 at 13:23
Thank you very much! Can you recommend some reading materials on avoiding sql injection vulenrabilities? — Bodul, Oct 03 '18 at 13:28
@Bodul See the [following link](https://learn.microsoft.com/en-us/dotnet/api/system.data.sqlclient.sqlparameter?view=netframework-4.7.2) — ColinM, Oct 03 '18 at 13:33

score 2 · Answer 2 · answered Oct 03 '18 at 13:56

Here is a working example with fixes for the non parameterized sql and suggestions on how to better store the connection string.

MainWindow.cs

class MainWindow
{
    private void btnLogin_Click(object sender, RoutedEventArgs e)
    {
        SomeRepository repo = new SomeRepository();
        var userName = repo.GetUserName(1);
        MessageBox.Show(userName ?? "User not found!");
    }
}

SomeRepository.cs

public sealed class SomeRepository
{
    private readonly string connectionString;
    public SomeRepository()
    {
      // the ideal location for a connection string is in the application's app.config (or web.confic)
      connectionString = System.Configuration.ConfigurationManager.ConnectionStrings["MyConnection"].ConnectionString;

      // Or uncomment this
      // connectionString = "Server=SQLOLEDB.1;User ID=" +
        //      Constants.DATABASE_USERNAME + ";Password=" +
        //      Constants.DATABASE_PASSWORD + ";Initial Catalog=" +
        //      Constants.DATABASE_CATALOG + ";Data Source=" +
        //      Constants.SERVER_ADRESS;
    }

    public string GetUserName(int id)
    {
        const string sqlRequest = "SELECT Name FROM Names WHERE ID = @id";

        using (SqlConnection conn = new SqlConnection(this.connectionString))
        using (SqlCommand command = new SqlCommand(sqlRequest, conn))
        {
            // if this is an integer in the schema, which it looks like it should be, then you need to pass it as an int and not a string
            command.Parameters.Add("@id", SqlDbType.Int).Value = id;

            // if it is a string then specify the type as varchar and specify the varchar length in the schema and pass a string
            // command.Parameters.Add("@id", SqlDbType.VarChar, 20).Value = id.ToString();

            conn.Open();
            return command.ExecuteScalar()?.ToString();
        }
    }
}

app.config

<?xml version="1.0"?>
<configuration>

  <connectionStrings>
    <add name="MyConnection" connectionString="YOUR CONNECTION STRING HERE" providerName="System.Data.SqlClient"/>
  </connectionStrings>

</configuration>

Thank you Igor. I was testing an idea here hence why connectionstring is in method and that is also a reason on passing string instead of int. However, I learn more than i asked for and that is a very good thing. — Bodul, Oct 03 '18 at 14:03

score 0 · Answer 3 · edited Dec 02 '18 at 20:56

You have to store your return result from SimpleQuery() to finally show this in a MessageBox.

    private void btnLogin_Click(object sender, RoutedEventArgs e)
    {
        TestConnection checkLogin = new TestConnection();
        string result = checkLogin.SimpleQuery("SELECT Name FROM Names WHERE ID='1'", "");

        MessageBox.Show(result);
    }

Alter your method to return the result:

public string SimpleQuery(string request, string response)
{
        using (SqlConnection conn = new SqlConnection())
        {
            conn.ConnectionString = "Server=SQLOLEDB.1;User ID=" +
                Constants.DATABASE_USERNAME + ";Password=" +
                Constants.DATABASE_PASSWORD + ";Initial Catalog=" +
                Constants.DATABASE_CATALOG + ";Data Source=" +
                Constants.SERVER_ADRESS;

            SqlCommand command = new SqlCommand(request, conn);

            conn.Open();
            string response = Convert.ToString(command.ExecuteScalar());
            conn.Close();

            return response; 
        }
}

C# sending select command from one class to another and return result?

3 Answers3

SQL injection issue (see comment)