This is a reasonably good starting point.
Good things you did:
- Parameterised query! Thank you for not making me get my SQL injection rant out and for knowing what you're doing :)
- Separation of data model and database access logic. Although V has suggested he wouldn't do it that way, I definitely encourage it.
- Checking whether the insert succeeds
Things to consider:
- Can you use a pre-cooked entity mapping framework (e.g. Entity Framework or NHibernate)? Making a good and robust database access layer can take a reasonable amount of time and effort. It is sometimes the correct decision (I've done it on a project) depending on the database structure but not that often, imo.
- Your return type shouldn't be object, as DamithSL points out. It should be a bool saying whether the insert succeeds. Alternatively, throw an exception if it didn't.
- You will currently have to write INSERT and UPDATE functions manually for every data type. Consider using reflection (with PropertyInfos cached) to determine the values and column names for each object (see below for a sketch).
- You aren't associating the retrieved ID with the object, so it will be difficult to UPDATE (save) it later.
My suggestion for the database access method:
public static bool Insert<T>(string tableName, T user) where T : IIdentifiable
{
using (SqlConnection conn = new SqlConnection(Data.GetConn()))
{
using (SqlCommand cmd = conn.CreateCommand())
{
PropertyInfo[] properties = typeof(T).GetProperties();
IList<string> propertyNames = properties.Select(p => p.Name).Where(n => n != "ID").ToList();
cmd.CommandText = "INSERT INTO " + tableName + " (" + string.Join(",", propertyNames) + ") VALUES (@" + string.Join(",@", propertyNames) + ") SELECT SCOPE_IDENTITY() as NewRecordID";
if (conn.State == ConnectionState.Closed)
conn.Open();
foreach(PropertyInfo property in properties)
cmd.Parameters.AddWithValue("@" + property.Name, property.GetValue(user, null));
var obj = cmd.ExecuteScalar();
int newId;
if(obj!=null && int.TryParse(obj.ToString(), out newId)
{
user.ID = newId;
return true;
}
}
}
return false;
... and modify User:
public interface IIdentifiable {
int ID { get; set; }
}
public class User: IIdentifiable {
public int ID { get; set; }
public string Name { get; set; }
public string Surname { get; set; }
}
IIdentifiable should be in your domain model, not your database layer, which is why it's phrased in domain terms (i.e. "I can identify this object" not "This is the database ID of this object").
This approach does slightly pollute your persistable domain objects, but it isn't too bad. You can easily expand it to allow for attributes to be specified on classes and properties to override the default behaviour (e.g. change the column name associated with a property, exclude properties from persistence, link properties to other object types etc) if you need that.