Would you change anything in this code?

Be advised, this post contains "long" code listings.

Hello everyone,

I'm using the following code to transfer data from a SQL Server 2005 database to a business object. I'd like to have some feedback to this sample code. Prolly there's plenty of room for improvement ;)

Business Object

using System;namespace PTContact.CCAM.BO{public class Campaign {#region Private variablesprivate string _Id;private string _Alias;private string _SiteId;private string _ManagerMembershipId;private string _AddedByMembershipId;private DateTime _AddedDate;private string _RemovedByMembershipId;private DateTime _RemovedDate;private string _Description;private int _Type;private bool _Status;#endregion #region Public Propertiespublic string Id {get {return _Id; }set { _Id =value; } }public string Alias {get {return _Alias; }set { _Alias =value; } }public string SiteId {get {return _SiteId; }set { _SiteId =value; } }public string ManagerMembershipId {get {return _ManagerMembershipId; }set { _ManagerMembershipId =value; } }public string AddedByMembershipId {get {return _AddedByMembershipId; }set { _AddedByMembershipId =value; } }public DateTime AddedDate {get {return _AddedDate; }set { _AddedDate =value; } }public string RemovedByMembershipId {get {return _RemovedByMembershipId; }set { _RemovedByMembershipId =value; } }public DateTime RemovedDate {get {return _RemovedDate; }set { _RemovedDate =value; } }public string Description {get {return _Description; }set { _Description =value; } }public int Type {get {return _Type; }set { _Type =value; } }public bool Status {get {return _Status; }set { _Status =value; } }#endregion }}

List (using generics)

using System;using System.Collections.Generic;namespace PTContact.CCAM.BO{public class CampaignList : List {public CampaignList() { } }}
 

Business Logic Layer

 
using System;using PTContact.CCAM.BO;namespace PTContact.CCAM.BLL{public class CampaignManager {#region Public methodspublic static CampaignList GetItemList(string SiteId) {
 return CampaignDB.GetItemList(SiteId); }#endregion }}
 
Data Access Layer
 
using System;using System.Data;using System.Data.SqlClient;using System.Configuration;using PTContact.CCAM.BO;namespace PTContact.CCAM.DAL{public class CampaignDB {#region Private methodsprivate static Campaign FillDataRecord(IDataRecord myDataRecord) { Campaign tmpItem =new Campaign(); tmpItem.Id = myDataRecord.GetString(myDataRecord.GetOrdinal("Id")); tmpItem.Alias = myDataRecord.GetString(myDataRecord.GetOrdinal("Alias")); tmpItem.SiteId = myDataRecord.GetString(myDataRecord.GetOrdinal("SiteId")); tmpItem.ManagerMembershipId = myDataRecord.GetString(myDataRecord.GetOrdinal("ManagerMembershipId")); tmpItem.AddedByMembershipId = myDataRecord.GetString(myDataRecord.GetOrdinal("AddedByMembershipId")); tmpItem.AddedDate = myDataRecord.GetDateTime(myDataRecord.GetOrdinal("AddedDate")); tmpItem.RemovedByMembershipId = myDataRecord.GetString(myDataRecord.GetOrdinal("RemovedByMembershipId")); tmpItem.RemovedDate = myDataRecord.GetDateTime(myDataRecord.GetOrdinal("RemovedDate")); tmpItem.Description = myDataRecord.GetString(myDataRecord.GetOrdinal("Description")); tmpItem.Type = myDataRecord.GetInt32(myDataRecord.GetOrdinal("Type")); tmpItem.Status = myDataRecord.GetBoolean(myDataRecord.GetOrdinal("Status"));return tmpItem; }#endregion #region Public methodspublic static CampaignList GetItemList(string SiteId) { CampaignList tmpList =null;using (SqlConnection SqlConn =new SqlConnection(ConfigurationManager.ConnectionStrings["LocalSqlServer"].ConnectionString)) {using (SqlCommand SqlCmd =new SqlCommand("usp_Campaign_GetItemList_For_SiteId", SqlConn)) { SqlConn.Open(); SqlCmd.CommandType = CommandType.StoredProcedure; SqlCmd.Parameters.Add("@dotnet.itags.org.SiteId", SqlDbType.Char, 36).Value = SiteId;using (SqlDataReader SqlReader = SqlCmd.ExecuteReader()) {if (SqlReader.HasRows) { tmpList =new CampaignList();while (SqlReader.Read()) { tmpList.Add(FillDataRecord(SqlReader)); } SqlReader.Close(); } } } }return tmpList; }#endregion }}
 
------ 
If you have any suggestions, at all, about how I could improve this code both in terms of coding or (aparent) design structure, I'd be most welcome.
 
Best regards.

Other then perhaps wrapping some error checking around your calls and making sure you aren't leaving anything unhandled it looks about right. It's a pretty standard layout I think you'll find... there's nothing blatently obvious that I saw...

Curt_C:

Other then perhaps wrapping some error checking around your calls and making sure you aren't leaving anything unhandled it looks about right. It's a pretty standard layout I think you'll find... there's nothing blatently obvious that I saw...

Thank you for your feedback. I've found it to be the standard when I was reading some articles. It's just that this it the first time I'll work directly with generics, thus double checking this code ;)

I found that the code I've posted would have another problem, other than the poor error checking. When using this database design it's possible that some values are returned as null. This said, there needs to be some hand coded checking when it passing the values to the business object. Furthermore, I needed to declare the datetime variables as nullable.

Here's a snipet

if (!myDataRecord["RemovedByMembershipId"].Equals(DBNull.Value) && !myDataRecord["RemovedDate"].Equals(DBNull.Value)) { tmpItem.RemovedByMembershipId = myDataRecord.GetString(myDataRecord.GetOrdinal("RemovedByMembershipId")); tmpItem.RemovedDate = myDataRecord.GetDateTime(myDataRecord.GetOrdinal("RemovedDate")); }else { tmpItem.RemovedByMembershipId =null; tmpItem.RemovedDate =null; }

Best regards.