6
\$\begingroup\$

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?

\$\endgroup\$
  • 1
    \$\begingroup\$ SqlCommand requires a using too. \$\endgroup\$ – t3chb0t Jun 23 '16 at 11:34
  • \$\begingroup\$ Just a small note 'sort_order' sounds like an enum variable rather rhan just a string. \$\endgroup\$ – Denis Jun 23 '16 at 17:26
  • \$\begingroup\$ @denis its an int value to sort the data by an individual order and is a column in the db table \$\endgroup\$ – STORM Jun 23 '16 at 17:38
7
\$\begingroup\$

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;
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ If something of my changes isn't clear, I can add explanations. \$\endgroup\$ – Stefano d'Antonio Jun 23 '16 at 12:52
  • \$\begingroup\$ Wow, thank you @Uno. One additional question to public static List<Supplier> GetSuppliers() => RunWithElevatedPrivileges(GetSuppliersInternal); is is ok to have such "factory" methods within such a class or is it better to put such methods in another classes? \$\endgroup\$ – STORM Jun 23 '16 at 13:05
  • \$\begingroup\$ To be honest, it depends on your architecture. The DTO/Repository pattern is great, more readable and follows the SRP. But, every system has exceptions, in your case might be useless. Only you can be sure, have a look at the advantages/disadvantages of the pattern on Wikipedia and see if it suits your needs. \$\endgroup\$ – Stefano d'Antonio Jun 23 '16 at 13:18
  • \$\begingroup\$ If you are unsure, I would adopt it if it's easy to keep consistency across the whole application. I would also consider using a small ORM to save yourself the mapping of database entities. \$\endgroup\$ – Stefano d'Antonio Jun 23 '16 at 13:20
  • \$\begingroup\$ If you are unsure, I would adopt it if it's easy to keep consistency across the whole application. I would also consider using a small ORM to save yourself the mapping of database entities could you give me an idea of it? \$\endgroup\$ – STORM Jun 23 '16 at 13:34
8
\$\begingroup\$

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();
\$\endgroup\$
  • \$\begingroup\$ Thanks for the detailed infos. Do you mean with 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. only extracting the static methods or even the "other" ones. And one more question: Would you prefer to separate the static methods from the "other" methods? \$\endgroup\$ – STORM Jun 23 '16 at 12:50
  • \$\begingroup\$ I meant all methods, also the non-static ones. You'll have to change them a bit, for example, supply necessary parameters for the Save or Update method. \$\endgroup\$ – Abbas Jun 23 '16 at 13:34
3
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ thanks for your answer. I understand the first part and in fact, it makes no sense to have the id public. How to split in public and private? I do not understand the second part. Could you explain how you mean it? Do you mean to return the id fron db after insert and set the id in object? \$\endgroup\$ – STORM Jun 23 '16 at 17:50
  • \$\begingroup\$ For the first: just add private in front of the set and for the second: yes \$\endgroup\$ – Heslacher Jun 23 '16 at 17:55
1
\$\begingroup\$

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);
    }
}
\$\endgroup\$
  • \$\begingroup\$ Thanks for your answer. Im reading now the hole about func, action, delegates etc and i understand it now i think. But your pattern is currently not so clear for me. I would have the supplier itself as model and could separate the layers then an interface and additional class which implements this interface, but why? Why do i need an interface? Couldnt i implement a "normal" second class which does the database things? \$\endgroup\$ – STORM Jun 23 '16 at 20:18
  • \$\begingroup\$ I would encourage you to read up on the repository pattern. I provided a link to get you started, but there are lots of examples and resources on the Web. \$\endgroup\$ – Greg Burghardt Jun 23 '16 at 20:35

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.