SharePoint CRUD class

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)
{

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; }

• Don't you already have an ID and Name property?

• 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. Have you considered returning an IEnumerable instead? Here's what the method would look like (irrelevant code omitted):

public static IEnumerable GetSuppliers()
{
    //...

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

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

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

Using a List 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, leave the method as an IEnumerable and do following:

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

StackExchange Code Review Q#132823, answer score: 8