1
\$\begingroup\$

I have a simple app which is checking occurrences of values. If there is already occurrences which I am looking for, it display that. If not, it adds.

create table Strings
(
    StringID INT IDENTITY(1,1) NOT NULL PRIMARY KEY
    , StringValue VARCHAR(100) NOT NULL
)

create table Links
(
    LinkID INT IDENTITY(1,1) NOT NULL PRIMARY KEY
    ,Link VARCHAR(255) NOT NULL
)

CREATE TABLE Occurences
(
    OccurenceID INT IDENTITY(1,1) NOT NULL PRIMARY KEY
    ,StringID INT NOT NULL
    ,LinkID INT NOT NULL
)

Class:

private SqlConnection conn;


public void Open()
{
    this.conn = new SqlConnection();
    this.conn.ConnectionString = @"Server=PC\SQLEXPRESS;Database=example;User=sa;Pwd=example;";
    this.conn.Open();
}

public void Close()
{
    this.conn.Close();
}


public string Obtain(string code)
{
    SqlConnection sqlConnection = new SqlConnection();
    sqlConnection.ConnectionString = @"Server=PC\SQLEXPRESS;Database=example;User=sa;Pwd=example;";
    SqlCommand sqlCommand = new SqlCommand();
    sqlCommand.Connection = sqlConnection;
    sqlCommand.CommandText = code;
    sqlConnection.Open();
    SqlDataReader sqlDataReader = sqlCommand.ExecuteReader();
    sqlDataReader.Read();
    string result = sqlDataReader.GetValue(0).ToString();
    sqlConnection.Close();
    return result;
}


public void Insert(string StringID, string LinkID)
{
    SqlConnection sqlConnection = new SqlConnection();
    sqlConnection.ConnectionString = @"Server=PC\SQLEXPRESS;Database=example;User=sa;Pwd=example;";
    SqlCommand sqlCommand = new SqlCommand();
    sqlCommand.Connection = sqlConnection;
    sqlCommand.CommandText = string.Concat(new string[]
    {
        "INSERT INTO Occurences VALUES ('",
        StringID,
        "', '",
        LinkID,
        "')"
    });
    sqlConnection.Open();
    sqlCommand.ExecuteNonQuery();
    sqlConnection.Close();
}

Button click:

private void button1_Click(object sender, EventArgs e)
{

    try
    {
        sql.Open();
        StringID = sql.Obtain("SELECT StringID from dbo.Strings where StringValue='" + textBox1.Text + "'");
        LinkID = sql.Obtain("SELECT LinkID from dbo.Links where Link='" + textBox2.Text + "'");
        Occurence = sql.Obtain("SELECT OccurenceID from dbo.Occurences where StringID='" + StringID + "' and LinkID = '" + LinkID + "'");
        if (Convert.ToInt32(Occurence) > 0)
        {
            MessageBox.Show(Occurence);
        }
    }

    catch
    {
        sql.Insert(StringID, LinkID);
    }

    finally
    {
        sql.Close();



    }
}

Is this coded well? Should I change my catch to more specific? Should I do something like these?

  1. If there is no StringValue in dbo.Strings then insert StringValue
  2. If there is no Link in dbo.Links then insert Link
\$\endgroup\$
4
  • \$\begingroup\$ What if the put aa'; drop table Links in TextBox1 \$\endgroup\$
    – paparazzo
    Aug 13, 2016 at 15:35
  • 2
    \$\begingroup\$ You should be using parameterized queries, not open and close three times to get 3 items, and you don't need a reader to get one value. \$\endgroup\$
    – paparazzo
    Aug 13, 2016 at 15:38
  • \$\begingroup\$ For the sql connection, shouldn't you be using "using" keyword that implicitly calls IDisposable or maybe use try..catch to explicitly call Dispose(). \$\endgroup\$
    – Tolani
    Aug 15, 2016 at 8:36
  • \$\begingroup\$ FYI "Occurences" is spelled incorrectly. \$\endgroup\$
    – BCdotWEB
    Sep 12, 2016 at 8:26

1 Answer 1

4
\$\begingroup\$

Usually when we do something similar we use a repository (pattern). It allows us to encapsulate the entire interaction with the database so that we don't have to think about the queries again and again.

For example:

class MyDatabase
{
    public MyDatabase(string connectionString = null)
    {       
        // use the specified or the default connection string
        ConnectionString = connectionString ?? @"Server=PC\SQLEXPRESS;Database=example;User=sa;Pwd=example;";
    }

    public string ConnectionString { get; private set; }

    public int GetStringID(string stringValue)
    {
        // here comes the select and connection manintenance
    }
}

When working with database objects you should always use the using statement to free the resources. Always check the documentaiton if something implements the IDisposable interface.

Your actual code should look like this:

using(var sqlConnection = new SqlConnection(ConnectionString))
{
    // work with the connection where
}

There are few more types that are disposable too (i.e. the SqlCommand or the SqlDataReader).

With the repository your button1_Click could look like this:

private void button1_Click(object sender, EventArgs e)
{
    var db = new MyDatabase();
    StringID = db.GetStringID(textBox1.Text);
}

Naming

Even when working with the simplest projects you should always use meaningful names and button1 or textBox1 are really bad. I don't know what the button1 does but the text-box could be named stringValueTextBox or the textBox2 should be called linkTextBox etc.

\$\endgroup\$

Your Answer

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

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