Is this a proper implementation of n-layer architecture?

Question

I have been learning C# for the last year or so and trying to incorporate best practices along the way. Between StackOverflow and other web resources, I thought I was on the right track to properly separating my concerns, but now I am having some doubts and want to make sure I am going down the right path before I convert my entire website over to this new architecture.

The current website is old ASP VBscript and has a existing database that is pretty ugly (no foreign keys and such) so at least for the first version in .NET I do not want to use and have to learn any ORM tools at this time.

I have the following items that are in separate namespaces and setup so that the UI layer can only see the DTOs and Business layers, and the Data layer can only be seen from the Business layer. Here is a simple example:

productDTO.cs

public class ProductDTO
{
    public int ProductId { get; set; }
    public string Name { get; set; }

    public ProductDTO()
    {
        ProductId = 0;
        Name = String.Empty;
    }
}

productBLL.cs

public class ProductBLL
{

    public ProductDTO GetProductByProductId(int productId)
    {
        //validate the input            
        return ProductDAL.GetProductByProductId(productId);
    }

    public List<ProductDTO> GetAllProducts()
    {
        return ProductDAL.GetAllProducts();
    }

    public void Save(ProductDTO dto)
    {
        ProductDAL.Save(dto);
    }

    public bool IsValidProductId(int productId)
    {
        //domain validation stuff here
    }
}

productDAL.cs

public class ProductDAL
{
    //have some basic methods here to convert sqldatareaders to dtos


    public static ProductDTO GetProductByProductId(int productId)
    {
        ProductDTO dto = new ProductDTO();
        //db logic here using common functions 
        return dto;
    }

    public static List<ProductDTO> GetAllProducts()
    {
        List<ProductDTO> dtoList = new List<ProductDTO>();
        //db logic here using common functions 
        return dtoList;
    }

    public static void Save(ProductDTO dto)
    {
        //save stuff here
    }

}

In my UI, I would do something like this:

ProductBLL productBll = new ProductBLL();
List<ProductDTO> productList = productBll.GetAllProducts();

for a save:

ProductDTO dto = new ProductDTO();
dto.ProductId = 5;
dto.Name = "New product name";
productBll.Save(dto);

Am I completely off base? Should I also have the same properties in my BLL and not pass back DTOs to my UI? Please tell me what is wrong and what is right. Keep in mind I am not a expert yet.

I would like to implement interfaces to my architecture, but I am still learning how to do that.

Answer 1

Cade has a good explination. In order to avoid the Anemic domain model some things you could consider doing:

• make the DTO object your domain object (just call it "Product")
• IsValidProductId could then be on the Product, and when the setter is called you could verify that it is valid, and throw if it is not
• implement some sort of rules about the Name
• if there are any other objects that interact with Product we could have more interesting things to talk about

Answer 2

Things you want to consider adding: validation, property change notification, data binding, etc... One common issue when separating each class in multiple classes (DAL, BLL, etc...) is often you end up with a lot of code you need to duplicate. Another problem is if you need some intimacy between those classes, you'll have to create internal members (interfaces, fields, etc.)

This is what I would do, build a unique consistent Domain Model, something like this:

public class Product: IRecord, IDataErrorInfo, INotifyPropertyChanged
{
    // events
    public event PropertyChangedEventHandler PropertyChanged;

    // properties
    private int _id;
    public virtual int Id
    {
        get
        {
            return _id;
        }
        set
        {
            if (value != _id)
            {
                _id = value;
                OnPropertyChanged("Id");
            }
        }
    }

    private string _name;
    public virtual string Name
    {
        get
        {
            return _name;
        }
        set
        {
            if (value != _name)
            {
                _name = value;
                OnPropertyChanged("Name");
            }
        }
    }

    // parameterless constructor (always useful for serialization, winforms databinding, etc.)
    public Product()
    {
        ProductId = 0;
        Name = String.Empty;
    }

    // update methods
    public virtual void Save()
    {
       ValidateThrow();
       ... do save (insert or update) ...
    }

    public virtual void Delete()
    {
       ... do delete ...
    }    

    // validation methods
    public string Validate()
    {
       return Validate(null);
    }

    private void ValidateThrow()
    {
      List<Exception> exceptions = new List<Exception>();
      SummaryValidate(exceptions,memberName);
      if (exceptions.Count != 0)
         throw new CompositeException(exceptions);
    }

    public string Validate(string memberName)
    {
      List<Exception> exceptions = new List<Exception>();
      SummaryValidate(exceptions,memberName);
      if (exceptions.Count == 0)
        return null;

      return ConcatenateAsString...(exceptions);
    }

    string IDataErrorInfo.Error
    {
      get
      {
         return Validate();
      }
    }

    string IDataErrorInfo.this[string columnName]
    {
      get
      {
        return validate(columnName);
      }
    }

    public virtual void SummaryValidate(IList<Exception> exceptions, string memberName)
    {
       if ((memberName == null) || (memberName == "Name"))
       {
         if (!... validate name ...)
            exceptions.Add(new ValidationException("Name is invalid");
       }
    }

    protected void OnPropertyChanged(string name)
    {
       OnPropertyChanged(new PropertyChangedEventArgs(name));
    }

    // property change notification
    protected virtual void OnPropertyChanged(PropertyChangedEventArgs e)
    {
        if ((PropertyChanged != null)
            PropertyChanged(this, e);
    }

    // read from database methods
    protected virtual Read(IDataReader reader)
    {
      Id = reader.GetInt32(reader.GetOrdinal("Id"));
      Name = = reader.GetString(reader.GetOrdinal("Id"));
      ...
    }

    void IRecord.Read(IDataReader reader)
    {
      Read(reader);
    }

    // instance creation methods
    public static Product GetById(int id)
    {
        // possibly use some cache (optional)
        Product product = new Product();
        using (IDataReader reader = GetSomeReaderForGetById...(id))
        {
            if (!reader.Read())
              return null;

            ((IRecord)product).Read(reader);
            return product;
        }
    }

    public static List<Product> GetAll()
    {
        // possibly use some cache (optional)
        List<Product> products = new List<Product>(); // if you use WPF, an ObservableCollection would be more appropriate?
        using (IDataReader reader = GetSomeReaderForGetAll...(id))
        {
            while (reader.Read())
            {
              Product product = new Product();
              ((IRecord)product).Read(reader);
              products.Add(product);
            }
        }
        return products;
    }
}

// an interface to read from a data record (possibly platform independent)
public interface IRecord
{
  void Read(IDataReader reader);
}

Answer 3

Anemic domain is when a product or other class doesn't really implement anything more than data setters and getters - no domain behavior.

For instance, a product domain object should have some methods exposed, some data validations, some real business logic.

Otherwise, the BLL version (the domain object) is hardly better than a DTO.

http://martinfowler.com/bliki/AnemicDomainModel.html

ProductBLL productBll = new ProductBLL();
List<ProductDTO> productList = productBll.GetAllProducts();

The problem here is that you are pre-supposing your model is anemic and exposing the DTO to the business layer consumers (the UI or whatever).

Your application code generally wants to be working with <Product>s, not any BLL or DTO or whatever. Those are implementation classes. They not only mean little to the application programmer level of thought, they mean little to domain experts who ostensibly understand the problem domain. Thus they should only be visible when you are working on the plumbing, not when you are designing the bathroom, if you see what I mean.

I name my BLL objects the name of the business domain entity. And the DTO is internal between the business entity and the DAL. When the domain entity doesn't do anything more than the DTO - that's when it's anemic.

Also, I'll add that I often just leave out explcit DTO classes, and have the domain object go to a generic DAL with organized stored procs defined in the config and load itself from a plain old datareader into its properties. With closures, it's now possible to have very generic DALs with callbacks which let you insert your parameters.

I would stick to the simplest thing that can possibly work:

public class Product {
    // no one can "make" Products
    private Product(IDataRecord dr) {
        // Make this product from the contents of the IDataRecord
    }

    static private List<Product> GetList(string sp, Action<DbCommand> addParameters) {
        List<Product> lp = new List<Product>();
        // DAL.Retrieve yields an iEnumerable<IDataRecord> (optional addParameters callback)
        // public static IEnumerable<IDataRecord> Retrieve(string StoredProcName, Action<DbCommand> addParameters)
        foreach (var dr in DAL.Retrieve(sp, addParameters) ) {
            lp.Add(new Product(dr));
        }
        return lp;
    }

    static public List<Product> AllProducts() {
        return GetList("sp_AllProducts", null) ;
    }

    static public List<Product> AllProductsStartingWith(string str) {
        return GetList("sp_AllProductsStartingWith", cm => cm.Parameters.Add("StartsWith", str)) ;
    }

    static public List<Product> AllProductsOnOrder(Order o) {
        return GetList("sp_AllProductsOnOrder", cm => cm.Parameters.Add("OrderId", o.OrderId)) ;
    }
}

You can then move the obvious parts out into a DAL. The DataRecords serve as your DTO, but they are very short-lived - a collection of them never really exists.

Here's a DAL.Retrieve for SqlServer which is static (you can see it's simple enough to change it to use CommandText); I have a version of this which encapsulates the connection string (and so it's not a static method):

    public static IEnumerable<IDataRecord> SqlRetrieve(string ConnectionString, string StoredProcName,
                                                       Action<SqlCommand> addParameters)
    {
        using (var cn = new SqlConnection(ConnectionString))
        using (var cmd = new SqlCommand(StoredProcName, cn))
        {
            cn.Open();
            cmd.CommandType = CommandType.StoredProcedure;

            if (addParameters != null)
            {
                addParameters(cmd);
            }

            using (var rdr = cmd.ExecuteReader())
            {
                while (rdr.Read())
                    yield return rdr;
            }
        }
    }

Later you can move on to full blown frameworks.

Answer 4

What others have said about using an ORM - as your model expands, you are going to have a lot of code repetition without one. But I wanted to comment on your "what about 5,000" question.

Copying a class does not create 5,000 copies of its methods. It only creates a copy of the data structures. There is no lost efficiency to having the business logic in the domain object. If some of the business logic is not applicable, then you could create subclasses that decorate the object for specific purposes, but the purpose of this is to create objects that match your intended use, not efficiency. An anemic design model is not more efficient.

Also, think about how you will use data in your application. I can't think of a single time I've ever used a method like "GetAllOfSomething()", except for maybe a reference list. What is the purpose of retrieving everything in your database? If it's to do some process, data manipulation, report, you should be exposing a method that performs that process. If you need to expose a list for some external use, like populating a grid, then expose an IEnumerable and provide methods for subsetting data. If you start with the idea that you work with complete lists of data in memory you'll have serious performance problems as the data grows.