Adding object to database, using linq or sql connection

Question

I have a table called tblDelegates in my database which I need to populate with some data. I have created a class called Delegates which has various properties to hold the bulk of the information.

I am fairly new to C#, so I wanted to get the answers to a few questions as to why you should be doing things in a particular way and also to get some advice on how I could improve my code.

Please can someone take a look at the questions below and give me a few pointers as to whether this is the correct way to do things.

1. Is this the correct way to do this, by first creating an instance of an object and then adding the fields to it before adding the object to the database?
2. This will sound like a total noob question (so I apologies), but why bother having to create the delegates class object, why could/should you not just add the fields directly into the database?
3. In my Delegates object, should I also add the fields such as Created_User Created_Dttm Updated_User Updated_Dttm, or am I ok leaving them out of this object like I have done below.
4. Is the most efficient/quickest way to do this using LINQ like I have done below, or should I be using a SQL connection, or is it just preference?
5. Can anyone tell me if the formatting of my code is correct, or if I should separate out the steps into separate functions perhaps?

Code to add data to database

 var del = new Delegates
 {
 Pending = true,
 ManagerAlias = row.Cells[4].Text,
 ManagerName = row.Cells[1].Text + " " + row.Cells[2].Text,
 DelegateAlias = ADLookup.AD("alias"),
 DelegateName = ADLookup.AD("GivenName") + ADLookup.AD("sn"),
 DelegateEmail = ADLookup.AD("mail")
 };
 var dc = new LinqToSqlDataContext();
 var Del = new tblDelegate
 {
 Pending = del.Pending,
 Manager_Alias = del.ManagerAlias,
 Manager_Name = del.ManagerName,
 Delegate_Alias = del.ManagerAlias,
 Delegate_Name = del.DelegateName,
 Delegate_Email = del.DelegateEmail,
 Created_User = ADLookup.ADCurrentUser(),
 Created_Dttm = DateTime.Now,
 Updated_User = ADLookup.ADCurrentUser(),
 Updated_Dttm = DateTime.Now
 };
 dc.tblDelegates.InsertOnSubmit(Del);
 dc.SubmitChanges();

Delegates Class

public class Delegates
{
 public bool Pending { get; set; }
 public string ManagerAlias {get;set;}
 public string ManagerName {get;set;}
 public string DelegateAlias {get;set;}
 public string DelegateName {get;set;}
 public string DelegateEmail {get;set;}
 public Delegates()
 { 
 }
 public Delegates(bool pending, string manageralias, string managername, string delegatealias, string delegatename, string delegateemail)
 {
 Pending = pending;
 ManagerAlias = manageralias;
 ManagerName = managername;
 DelegateAlias = delegatealias;
 DelegateName = delegatename;
 DelegateEmail = delegateemail;
 }
}

Answer 1

As @ckuhn203 pointed out, the naming is pretty misleading, but it seems like your domain model calls that entity a Delegate. To dissipate ambiguity I'd call it something like DelegateEntity.

One convention in modeling, is to keep types singular, since an entity represents one single row in that tblDelegates table.

Now, in your insert code, the del instance is not needed. Leaves us with this (notice the name changes) - I'd recommend passing in your connection string to the context's constructor, instead of relying on the one in the designer:

 var context = new LinqToSqlDataContext(connectionString);
 var item = new tblDelegate
 {
 Pending = true,
 Manager_Alias = row.Cells[4].Text,
 Manager_Name = row.Cells[1].Text + " " + row.Cells[2].Text,
 Delegate_Alias = ADLookup.AD("alias"),
 Delegate_Name = ADLookup.AD("GivenName") + ADLookup.AD("sn"),
 Delegate_Email = ADLookup.AD("mail"),
 Created_User = ADLookup.ADCurrentUser(),
 Created_Dttm = DateTime.Now,
 Updated_User = ADLookup.ADCurrentUser(),
 Updated_Dttm = DateTime.Now // shouldn't this one be null?
 };
 context.tblDelegates.InsertOnSubmit(item);
 context.SubmitChanges();

A very important thing to keep in mind, is that a data context implements an interface called IDisposable, which means any instance of that type should be properly disposed, either by explicitly calling its .Dispose() method, or (the better way) by wrapping its usage inside a using block:

using (var context = new LinqToSqlDataContext())
{
 var item = new tblDelegate
 {
 Pending = true,
 Manager_Alias = row.Cells[4].Text,
 Manager_Name = row.Cells[1].Text + " " + row.Cells[2].Text,
 Delegate_Alias = ADLookup.AD("alias"),
 Delegate_Name = ADLookup.AD("GivenName") + ADLookup.AD("sn"),
 Delegate_Email = ADLookup.AD("mail"),
 Created_User = ADLookup.ADCurrentUser(),
 Created_Dttm = DateTime.Now,
 Updated_User = ADLookup.ADCurrentUser(),
 Updated_Dttm = DateTime.Now // shouldn't this one be null?
 };
 context.tblDelegates.InsertOnSubmit(item);
 context.SubmitChanges();
}

The context instance only exists within the using scope, and when the code exits that scope, context.Dispose() gets called, implicitly. Make it a habit to always put IDisposable instances in a using block.

---

Other than that, you're not following the naming convention for parameters: alllowercase is hard to read - should be camelCase:

public Delegates(bool pending, string managerAlias, string managerName, string delegateAlias, string delegateName, string delegateEmail)
{
 Pending = pending;
 ManagerAlias = managerAlias;
 ManagerName = managerName;
 DelegateAlias = delegateAlias;
 DelegateName = delegateName;
 DelegateEmail = delegateEmail;
}

But, instead of having a constructor with [potentially] dozens of parameters, you should simply remove all constructors (the parameterless one is there by default), and instantiate a Delegates like this instead:

var delegates = new Delegates { 
 Pending = true, 
 ManagerAlias = "foo",
 ManagerName = "bar",
 DelegateAlias = "_foo",
 DelegateName = "_bar",
 DelegateEmail = "[email protected]"
 };

This syntax is the object initializer syntax - notice that you don't need to specify the () after new Delegates, because the syntax is calling the type's default constructor and the compiler knows this, so ()'s are redundant. Of course this syntax can't work if your type doesn't have a default, parameterless constructor.

---

If you have a linq-to-sql data context, you probably have a designer with a .dbml too; in there you can specify new, more C#-friendly names for tblDelegates and its columns, LINQ-to-SQL uses the read-only Source property to know which column it's referring to - this means you can have a table called SOME_UGLY_NAME in your SQL backend, and refer to it as SomeMuchBetterName in your C# code!

...this also means tblDelegates can be mapped to DelegateEntity ;)

• user3284707: Thanks very much for your detailed response. This has really helped my understanding. So can you confirm that all Linq to SQL as well as normal SQL queries should be wrapped in using {} blocks? Would you generally change column names or just table names in the Linq .dnml if the column names are quite descriptive such as Manager_Name for the manager name above?
• Mathieu Guindon: @user3284707 anything that implements the IDisposable interface should have its Dispose method called, whether it's an ADO.NET SqlDataReader, an SqlConnection, an SqlCommand, or a Linq-to-SQL DataContext. I'd change the column names to match the language's naming conventions, so so I'd map a Manager_Name column to a ManagerName property, yes.

Answer 2

If I understand your code (and I'm not 100% certain I do), your Delegates class only returns one object (as opposed to a collection or array of objects) and should thus be named Delegate, but as @Mat'sMug pointed out, it conflicts with the System.Delegate namespace. Since your tblDelegates has nothing to do with that name space, you might want to think up something else. I'm drawing a blank though.

del isn't a very descriptive variable name, but given the context, not too hard to figure out.

There's some inconsistent whitespace here.

public bool Pending { get; set; }
public string ManagerAlias {get;set;}

But that's pretty nit-picky. I'd say this looks pretty good.

• Mathieu Guindon: Naming is hard. Delegate would conflict with System.Delegate.. and the naming is confusing because the Delegates class has nothing to do with System.Delegate!
• user3284707: Thanks very much for your comment, I will take on board your comments about my poor choice of naming conventions :)