[blank_program]

Would anyone be able to critique this code for me? What it is for is a simple application that just takes a name and updates a database.

The database is laid out as such:
Table: VISITOR
Fields: ID (Primary key int autoincrement), and NAME (string)

Table: ATTENDANCE
Fields: VISITID (int autoincrement) VISITORID (ID from VISITOR table), and DATE (current date)

I feel I am doing things very wrong and also want to add in a check to see if the database exists and if not then create it. This is called from a simple WPF window which has one text box and one button on it. I know I don't really have it sanitize my input because I am unsure how and this was made using Google and lots of trial and error. I also have never worked with a database up until now so I am learning that at the same time and am looking for constructive feedback on my code if possible.

Thank you

using System;
using System.Data;
using System.Data.SqlServerCe;
using System.Xml;

namespace client
{
    internal class DbEngine
    {
        internal void CheckVisitor(string VisitorName)
        {
            int NameMatches;

            SqlCeConnection DbConnection = new SqlCeConnection(@"Data Source=.\members.sdf;Persist Security Info=False;");
            
            using (DbConnection)
            {
                if (DbConnection.State != ConnectionState.Open)
                {
                    DbConnection.Open();
                }

                SqlCeCommand SearchCommand = new SqlCeCommand("select * from VISITORS where NAME = (@VisitorName)", DbConnection);
                SearchCommand.Parameters.AddWithValue("@VisitorName", VisitorName);

                SqlCeDataAdapter adapter = new SqlCeDataAdapter();
                using (adapter)
                {
                    adapter.SelectCommand = SearchCommand;

                    DataTable dt = new DataTable();
                    adapter.Fill(dt);

                    NameMatches = dt.Rows.Count;

                    if (NameMatches == 1)
                    {
                        AddVisitorDate(int.Parse(dt.Rows[0]["ID"].ToString()));
                    }
                    else
                    {
                        AddVisitor(VisitorName);
                        CheckVisitor(VisitorName);
                    }
                }
            }
        }

        internal void AddVisitor(string VisitorName)
        {
            SqlCeConnection DbConnection = new SqlCeConnection(@"Data Source=.\members.sdf;Persist Security Info=False;");

            using (DbConnection)
            {
                DbConnection.Open();

                SqlCeCommand SqlCeInsertComm = new SqlCeCommand("insert into VISITORS (NAME) VALUES (@VisitorName)", DbConnection);
                using (SqlCeInsertComm)
                {
                    SqlCeInsertComm.Parameters.AddWithValue("@VisitorName", VisitorName);

                    SqlCeInsertComm.ExecuteNonQuery();
                }
            }
        }

        internal void AddVisitorDate(int VisitorID)
        {
            SqlCeConnection DbConnection = new SqlCeConnection(@"Data Source=.\members.sdf;Persist Security Info=False;");

            using (DbConnection)
            {
                DbConnection.Open();

                SqlCeCommand SqlCeInsertComm = new SqlCeCommand("insert into ATTENDANCE (VISITOR, DATE) VALUES (@VisitorId, @VisitDate)", DbConnection);
                using (SqlCeInsertComm)
                {
                    SqlCeInsertComm.Parameters.AddWithValue("@VisitorId", VisitorID);
                    SqlCeInsertComm.Parameters.AddWithValue("@VisitDate", DateTime.Now.ToShortDateString());

                    SqlCeInsertComm.ExecuteNonQuery();
                }
            }
        }
    }
}

[RexGrammer]

I'm trying to find something wrong here, but... I can't!

It's really nice, and if it's your first time dealing with databases even better!

The only thing I would do differently here is: define the DbConnection in a global scope. That way you won't need to constantly declare it.

Don't see a reason for your: 'I feel I am doing things very wrong...' feeling.

I'm interested to see some input from some more experienced members, but I think this is tip-top! (By my standards)

This post has been edited by RexGrammer: 01 March 2012 - 10:51 AM

[eclipsed4utoo]

There are couple of things I might have done differently, but that doesn't mean your way is wrong. All in all, it looks good.

Just one thing to say. You shouldn't do "SELECT * FROM...". You should define the fields that you want to return.

[Curtis Rutland]

Another simple suggestion is to move your declaration of your disposables into the Using statement. So instead of this:

SomeDisposable disposable = new SomeDisposable()
using(disposable) {
    
}

use this:

using(SomeDisposable disposable = new SomeDisposable()) {
    
}

The difference is minor, but it creates the variable inside the scope of the using block, instead of outside. That way, you can't even try to use it once it's disposed. It's considered a best practice to only use using blocks for what you create inside them.

[The Architect 2.0]

hm, when I was using SQLCE, I was using LINQ-to-SQL rather than writing plain SQL.

any reason why plain SQL would be better? The only thing I could see was it took a *little* while to generate the classes(a couple of minutes roughly); maybe SQL has potential to be optimized better?

[eclipsed4utoo]

It's a trade-off between inline SQL and L2S. Inline SQL can be better optimized, depending on the query. L2S is easier to write code with, but sometimes, can create horrible queries and hurt performance.

You could argue that neither way is "better".