SharePoint CRUD class

Question

I am writing an application where I have some objects like customer, supplier, product, etc.

I have written a class for the object 'supplier' and wanted to ask if this is a good design. I have put in the class the object itself, some methods for saving and deleting, ... and some static methods for example to return all suppliers or for example delete a specific supplier other than the current object.

This is my class:

using System;
using System.Data;
using System.Data.SqlClient;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using MyApplication.Administration.Utility;
using Microsoft.SharePoint;

namespace MyApplication.Administration.Model
{
    public class Supplier
    {
        public int id { get; set; }
        public string name { get; set; }

        public int su_id { get; set; }
        public string su_name { get; set; }
        public string portal { get; set; }
        public string sort_order { get; set; }
        public bool active { get; set; }

        public Supplier() {

        }

        public static Supplier GetSupplier(int supplierId)
        {
            Supplier supplier = new Supplier();

            Microsoft.SharePoint.SPSecurity.RunWithElevatedPrivileges(delegate ()
            {
                string connString = Factories.Database.GetConnectionString();

                using (SqlConnection conn = new SqlConnection(connString))
                {
                    SqlCommand cmd = conn.CreateCommand();
                    cmd.CommandText = "SELECT supplier.*, supplier_status.name AS su_name FROM supplier INNER JOIN supplier_status ON supplier.su_id = supplier_status.id WHERE supplier.id = @id";
                    cmd.Parameters.AddWithValue("@id", SqlDbType.Int).Value = supplierId;

                    conn.Open();

                    using (SqlDataReader dr = cmd.ExecuteReader())
                    {
                        if (dr.HasRows)
                        {
                            // Hier ggf. nur das erste Element nehmen
                            while (dr.Read())
                            {
                                supplier.id = SqlReaderHelper.GetValue<int>(dr, "id");
                                supplier.name = SqlReaderHelper.GetValue<string>(dr, "name");
                                supplier.su_id = SqlReaderHelper.GetValue<int>(dr, "su_id");
                                supplier.su_name = SqlReaderHelper.GetValue<string>(dr, "su_name");
                                supplier.portal = SqlReaderHelper.GetValue<string>(dr, "portal");
                                supplier.sort_order = SqlReaderHelper.GetValue<string>(dr, "sort_order");
                                supplier.active = SqlReaderHelper.GetValue<bool>(dr, "active");
                            }
                        }
                        else
                        {
                            supplier = null;
                        }
                    }
                }
            });

            return supplier;
        }


        public static List<Supplier> GetSuppliers()
        {
            List<Supplier> supplierList = new List<Supplier>();

            Microsoft.SharePoint.SPSecurity.RunWithElevatedPrivileges(delegate ()
            {
                string connString = Factories.Database.GetConnectionString();

                using (SqlConnection conn = new SqlConnection(connString))
                {
                    SqlCommand cmd = conn.CreateCommand();
                    cmd.CommandText = "SELECT supplier.*, supplier_status.name AS su_name FROM supplier INNER JOIN supplier_status ON supplier.su_id = supplier_status.id";

                    conn.Open();

                    using (SqlDataReader dr = cmd.ExecuteReader())
                    {
                        if (dr.HasRows)
                        {
                            while (dr.Read())
                            {
                                Supplier supplier = new Supplier();
                                supplier.id = SqlReaderHelper.GetValue<int>(dr, "id");
                                supplier.name = SqlReaderHelper.GetValue<string>(dr, "name");
                                supplier.su_id = SqlReaderHelper.GetValue<int>(dr, "su_id");
                                supplier.su_name = SqlReaderHelper.GetValue<string>(dr, "su_name");
                                supplier.portal = SqlReaderHelper.GetValue<string>(dr, "portal");
                                supplier.sort_order = SqlReaderHelper.GetValue<string>(dr, "sort_order");
                                supplier.active = SqlReaderHelper.GetValue<bool>(dr, "active");

                                supplierList.Add(supplier);
                            }
                        }
                        else
                        {
                            supplierList = null;
                        }
                    }
                }
            });

            return supplierList;
        }

        /// <summary>
        /// Create new Supplier.
        /// </summary>
        public void Save()
        {
            Microsoft.SharePoint.SPSecurity.RunWithElevatedPrivileges(delegate ()
            {
                string connString = Factories.Database.GetConnectionString();

                using (SqlConnection conn = new SqlConnection(connString))
                {
                    SqlCommand cmd = conn.CreateCommand();
                    cmd.CommandText = "INSERT INTO supplier (name, su_id, portal, sort_order, active) VALUES (@name, @su_id, @portal, @sort_order, @active);";
                    cmd.Parameters.AddWithValue("@name", SqlDbType.NVarChar).Value = name;
                    cmd.Parameters.AddWithValue("@su_id", SqlDbType.Int).Value = su_id;
                    cmd.Parameters.AddWithValue("@portal", SqlDbType.NVarChar).Value = portal;
                    cmd.Parameters.AddWithValue("@sort_order", SqlDbType.NVarChar).Value = sort_order;
                    cmd.Parameters.AddWithValue("@active", SqlDbType.Bit).Value = active;

                    conn.Open();
                    cmd.ExecuteNonQuery();
                }
            });
        }

        /// <summary>
        /// Delete the Supplier itself.
        /// </summary>
        public void Delete()
        {
            Microsoft.SharePoint.SPSecurity.RunWithElevatedPrivileges(delegate ()
            {
                string connString = Factories.Database.GetConnectionString();

                using (SqlConnection conn = new SqlConnection(connString))
                {
                    SqlCommand cmd = conn.CreateCommand();
                    cmd.CommandText = "DELETE FROM supplier WHERE id = @id";
                    cmd.Parameters.AddWithValue("@id", SqlDbType.Int).Value = id;

                    conn.Open();
                    cmd.ExecuteNonQuery();
                }

            });
        }

        /// <summary>
        /// Delete a Supplier by id.
        /// </summary>
        /// <param name="supplierId"></param>
        public static void Delete(int supplierId)
        {
            Microsoft.SharePoint.SPSecurity.RunWithElevatedPrivileges(delegate ()
            {
                string connString = Factories.Database.GetConnectionString();

                using (SqlConnection conn = new SqlConnection(connString))
                {
                    SqlCommand cmd = conn.CreateCommand();
                    cmd.CommandText = "DELETE FROM supplier WHERE id = @id";
                    cmd.Parameters.AddWithValue("@id", SqlDbType.Int).Value = supplierId;

                    conn.Open();
                    cmd.ExecuteNonQuery();
                }
            });
        }

        /// <summary>
        /// Update a Supplier.
        /// </summary>
        public void Update()
        {
            Microsoft.SharePoint.SPSecurity.RunWithElevatedPrivileges(delegate ()
            {
                string connString = Factories.Database.GetConnectionString();

                using (SqlConnection conn = new SqlConnection(connString))
                {
                    SqlCommand cmd = conn.CreateCommand();
                    cmd.CommandText = "UPDATE supplier SET name = @name, su_id = @su_id, portal = @portal, sort_order = @sort_order, active = @active WHERE id = @id";
                    cmd.Parameters.AddWithValue("@id", SqlDbType.Int).Value = id;
                    cmd.Parameters.AddWithValue("@name", SqlDbType.NVarChar).Value = name;
                    cmd.Parameters.AddWithValue("@su_id", SqlDbType.Int).Value = su_id;
                    cmd.Parameters.AddWithValue("@portal", SqlDbType.NVarChar).Value = portal;
                    cmd.Parameters.AddWithValue("@sort_order", SqlDbType.NVarChar).Value = sort_order;
                    cmd.Parameters.AddWithValue("@active", SqlDbType.Bit).Value = active;

                    conn.Open();
                    cmd.ExecuteNonQuery();
                }
            });
        }
    }
}

Is this a good design? How could I optimize this? What would you prefer?

Answer 1

There was a lot of duplicate code, I changed it to be more clean:

using System;
using System.Data;
using System.Data.SqlClient;
using System.Collections.Generic;

namespace MyApplication.Administration.Model
{
    public class Supplier
    {
        public int Id { get; set; }
        public string Name { get; set; }

        public int SuId { get; set; }
        public string SuName { get; set; }
        public string Portal { get; set; }
        public string SortOrder { get; set; }
        public bool Active { get; set; }

        public static Supplier GetSupplier(int supplierId) => RunWithElevatedPrivileges(() => GetSupplierInternal(supplierId));

        private static Supplier GetSupplierInternal(int supplierId)
        {
            var supplier = new Supplier();

            return ExecuteSql(cmd =>
            {
                cmd.CommandText =
                    "SELECT supplier.*, supplier_status.name AS su_name FROM supplier INNER JOIN supplier_status ON supplier.su_id = supplier_status.id WHERE supplier.id = @id";
                cmd.Parameters.AddWithValue("@id", SqlDbType.Int).Value = supplierId;

                using (var dr = cmd.ExecuteReader())
                {
                    if (!dr.HasRows)
                        return null;

                    // Hier ggf. nur das erste Element nehmen
                    while (dr.Read())
                        MapSupplierData(supplier, dr);
                }

                return supplier;
            });
        }

        private static void MapSupplierData(Supplier supplier, SqlDataReader dr)
        {
            supplier.Id = SqlReaderHelper.GetValue<int>(dr, "id");
            supplier.Name = SqlReaderHelper.GetValue<string>(dr, "name");
            supplier.SuId = SqlReaderHelper.GetValue<int>(dr, "su_id");
            supplier.SuName = SqlReaderHelper.GetValue<string>(dr, "su_name");
            supplier.Portal = SqlReaderHelper.GetValue<string>(dr, "portal");
            supplier.SortOrder = SqlReaderHelper.GetValue<string>(dr, "sort_order");
            supplier.Active = SqlReaderHelper.GetValue<bool>(dr, "active");
        }


        public static List<Supplier> GetSuppliers() => RunWithElevatedPrivileges(GetSuppliersInternal);

        private static List<Supplier> GetSuppliersInternal()
        {
            return ExecuteSql(cmd =>
            {
                List<Supplier> supplierList;

                cmd.CommandText =
                    "SELECT supplier.*, supplier_status.name AS su_name FROM supplier INNER JOIN supplier_status ON supplier.su_id = supplier_status.id";

                using (var dr = cmd.ExecuteReader())
                {
                    if (!dr.HasRows)
                        return null;

                    supplierList = new List<Supplier>();

                    while (dr.Read())
                    {
                        var supplier = new Supplier();

                        MapSupplierData(supplier, dr);

                        supplierList.Add(supplier);
                    }
                }

                return supplierList;
            });
        }

        /// <summary>
        /// Create new Supplier.
        /// </summary>
        public void Save() => Microsoft.SharePoint.SPSecurity.RunWithElevatedPrivileges(SaveInternal);

        private void SaveInternal()
        {
            ExecuteSql(cmd =>
            {
                cmd.CommandText =
                    "INSERT INTO supplier (name, su_id, portal, sort_order, active) VALUES (@name, @su_id, @portal, @sort_order, @active);";
                cmd.Parameters.AddWithValue("@name", SqlDbType.NVarChar).Value = Name;
                cmd.Parameters.AddWithValue("@su_id", SqlDbType.Int).Value = SuId;
                cmd.Parameters.AddWithValue("@portal", SqlDbType.NVarChar).Value = Portal;
                cmd.Parameters.AddWithValue("@sort_order", SqlDbType.NVarChar).Value = SortOrder;
                cmd.Parameters.AddWithValue("@active", SqlDbType.Bit).Value = Active;
            });
        }

        /// <summary>
        /// Delete the Supplier itself.
        /// </summary>
        public void Delete() => Microsoft.SharePoint.SPSecurity.RunWithElevatedPrivileges(DeleteInternal);

        private void DeleteInternal()
        {
            ExecuteSql(cmd =>
            {
                cmd.CommandText = "DELETE FROM supplier WHERE id = @id";
                cmd.Parameters.AddWithValue("@id", SqlDbType.Int).Value = Id;
            });
        }

        /// <summary>
        /// Delete a Supplier by id.
        /// </summary>
        /// <param name="supplierId"></param>
        public static void Delete(int supplierId) => Microsoft.SharePoint.SPSecurity.RunWithElevatedPrivileges(() => DeleteInternal(supplierId));

        private static void DeleteInternal(int supplierId)
        {
            ExecuteSql(cmd =>
            {
                cmd.CommandText = "DELETE FROM supplier WHERE id = @id";
                cmd.Parameters.AddWithValue("@id", SqlDbType.Int).Value = supplierId;
            });
        }

        /// <summary>
        /// Update a Supplier.
        /// </summary>
        public void Update() => Microsoft.SharePoint.SPSecurity.RunWithElevatedPrivileges(UpdateInternal);

        private void UpdateInternal()
        {
            ExecuteSql(cmd =>
            {
                cmd.CommandText =
                    "UPDATE supplier SET name = @name, su_id = @su_id, portal = @portal, sort_order = @sort_order, active = @active WHERE id = @id";
                cmd.Parameters.AddWithValue("@id", SqlDbType.Int).Value = Id;
                cmd.Parameters.AddWithValue("@name", SqlDbType.NVarChar).Value = Name;
                cmd.Parameters.AddWithValue("@su_id", SqlDbType.Int).Value = SuId;
                cmd.Parameters.AddWithValue("@portal", SqlDbType.NVarChar).Value = Portal;
                cmd.Parameters.AddWithValue("@sort_order", SqlDbType.NVarChar).Value = SortOrder;
                cmd.Parameters.AddWithValue("@active", SqlDbType.Bit).Value = Active;
            });
        }

        // To a different class:

        private static T RunWithElevatedPrivileges<T>(Func<T> action)
        {
            var result = default(T);

            Microsoft.SharePoint.SPSecurity.RunWithElevatedPrivileges(() => result = action());

            return result;
        }

        private static void ExecuteSql(Action<SqlCommand> action)
        {
            string connectionString = Factories.Database.GetConnectionString();

            using (var conn = new SqlConnection(connectionString))
            {
                var cmd = conn.CreateCommand();

                action(cmd);

                conn.Open();
                cmd.ExecuteNonQuery();
            }
        }

        private static T ExecuteSql<T>(Func<SqlCommand, T> action)
        {
            T result;

            string connectionString = Factories.Database.GetConnectionString();

            using (var conn = new SqlConnection(connectionString))
            {
                var cmd = conn.CreateCommand();

                result = action(cmd);

                conn.Open();
                cmd.ExecuteNonQuery();
            }

            return result;
        }
    }
}

Answer 2

As per the Microsoft Guidelines, properties use PascalCase. Following line:

public string name { get; set; }

becomes:

public string Name { get; set; }

---

Not that it is prohibited, but I'd leave out the _ in a property name, I'd change sord_order to:

public string SortOrder { get; set; }

---

This is also more of a personal choice but I tend to place Is or Has before boolean names:

public bool IsActive { get; set; }

Here's an example from .NET libraries where the name Selected is used both as a boolean name and an event name; which might cause confusion:

bool Selected; //indicating whether something is selected or not
event Selected; //indicating an event that occurs after something has been selected

---

I take it su means supplier in following properties:

public int su_id { get; set; }
public string su_name { get; set; }

1. Don't you already have an ID and Name property?
2. If still needed I'd change the names to SupplierId and SupplierName

---

Is there a reason for the empty constructor? If you don't specifically need it, leave it out. The compiler will generate one for you and your code is cleaner.

---

I'm not a fan of mixing repository code and business objects. Extract all the query methods and place them in a (static) class SupplierRepository for example.

---

As stated in the comments, SqlCommand should be wrapped in a using statement as it also implements the IDisposable interface. More reading on this, here's an answer that explains: SqlConnection SqlCommand SqlDataReader IDisposable.

---

Your method GetSuppliers returns a List<Supplier>. Have you considered returning an IEnumerable<Supplier> instead? Here's what the method would look like (irrelevant code omitted):

public static IEnumerable<Supplier> GetSuppliers()
{
    //...

    using (SqlDataReader dr = cmd.ExecuteReader())
    {
        //...

        while (dr.Read())
        {
            var supplier = new Supplier();
            supplier.id = SqlReaderHelper.GetValue<int>(dr, "id");
            //...

            //Here's the important line:
            yield return supplier;
        }
    }
}

Using a List<T> you don't leave the caller the option for lazy evaluation/deferred execution. Your method will always place all results in-memory, even if this isn't needed. If you still need a List<T>, leave the method as an IEnumerable<T> and do following:

var supplierList = GetSuppliers().ToList();
//or:
List<T> supplierList = GetSuppliers();

Answer 3

On top of the other good answers I would like to point out that it can be dangorous to have a public setter for the id property.
Assume you accidently change that value, each update or delete call will target the wrong data which can lead to serious problems, for the data and probably for your job.

Another problem I see is in the Save () method. You are using a INSERT command but afterwards you e.g will update some values by calling Update() but your id has still the value 0 which will for sure result in some kind of sql exception.

Answer 4

While you seem to be following the Active Record Pattern, the Repository Pattern is probably used more because it promotes testable code. That would involve breaking out the data access code from the Supplier class and putting it into a SupplierRepository class. The SupplierRepository class would then implement an interface called ISupplierRepository like so:

public interface ISupplierRepository
{
    Supplier Find(int id);
    IEnumerable<Supplier> FindAll();
    void Add(Supplier entity);
    void Update(Supplier entity);
    void Delete(Supplier entity);
}

Then any other class that needs suppliers needs an ISupplierRepository object:

public class Foo
{
    public Foo(ISupplierRepository repository)
    {
        this.repository = repository;
    }

    private ISupplierRepository repository;

    public void DoSomething()
    {
        Supplier supplier = repository.Find(32);

        // change supplier

        repository.Update(supplier);
    }
}