Sunday, October 19, 2014

C# var abuse

What happened to programming to an interface?



You have this DLL that contains this interface and implementation:
namespace UnitTestFriendlyDal
{
    public interface IDomainAccess : IDisposable
    {
        object Save(object transientObject);
        IQueryable<T> Query<T>();
        T Get<T>(object id);
        T Load<T>(object id);
        void Evict<T>(object id);
    }

    public class DomainAccess : IDomainAccess
    {

        NHibernate.ISessionFactory _sessionFactory;
        NHibernate.ISession _session;
        NHibernate.ITransaction _transaction;


        public DomainAccess(NHibernate.ISessionFactory sessionFactory)
        {
            _sessionFactory = sessionFactory;
            _session = _sessionFactory.OpenSession();
            _transaction = _session.BeginTransaction();
        }

        // IDomainAccess implementation...
        


        public Save(object transientObject)
        {
            return _session.Save(transientObject);
        }

        
        IQueryable<T> Query<T>()
        {
            return _session.Query<T>();
        }


        public T Get<T>(object id)
        {
            return _session.Get<T>(id);
        }

        public T Load<T>(object id)
        {
            return _session.Load<T>(id);
        }



        public void Evict<T>(object id)
        {
            _sessionFactory.Evict(typeof(T), id);
        }
        

        
        // Because transaction is a cross-cutting concern. It should be automated
        void IDisposable.Dispose()
        {
             // http://www.hibernatingrhinos.com/products/nhprof/learn/alert/donotuseimplicittransactions
                    
            _transaction.Commit();
            _transaction.Dispose();
            _session.Dispose();
        }


        //... IDomainAccess implementation
    }
}


Developers will use DomainAccess directly instead:
using (var da = new DomainAccess(_sessionFactory))
{    
    var c = Customer.Get(domainAccess: da, id: 1);
}


So what happen now to programming to an interface principle? Prior to C# version 3, you can still convince your colleagues to use variable declaration that adheres to programming to an interface principle: IDomainAccess da = new DomainAccess(), instead of: DomainAccess da = new DomainAccess(). interface variable declaration is just one letter away from its concrete class variable declaration :-)


Now, some developers will steadfastly refuse to type the type anymore. Code's obviousness to other developers is not given an attention, worse yet most won't care anymore if the code is not adhering to programming to an interface principle anymore


Another problem with var, when you generate method stub using the da variable from a var declaration, Visual Studio will generate the method stub based on da's concrete class, not based on its interface:
public static Get(DomainAccess domainAccess, int id)
{
}

When it should ideally be interface-based instead:
public static Customer Get(IDomainAccess domainAccess, int id) 
{
}


What you wanted your fellow colleagues to practice:
using (IDomainAccess da = new DomainAccess(_sessionFactory))
{    
    var c = Customer.Get(domainAccess: da, id: 1);    
}


There's a slim chance that you can convince your colleagues who are fans of var to declare variables based on their interface

Surely, most C# developers didn't heed this Microsoft warning:
However, the use of var does have at least the potential to make your code more difficult to understand for other developers. For that reason, the C# documentation generally uses var only when it is required


Another good argument against var abuse. Bradley Smith wrote:
var encourages Hungarian Notation

If the ommission of type names from variable declarations forces us to more carefully name our variables, it follows that variable names are more likely to describe not only their purpose, but also their type:

var dtIndividuals = GetContacts(ContactTypes.Individuals);

This is precisely the definition of Hungarian Notation, which is now heavily frowned upon as a practice, especially in type-safe languages like C#.


dtIndividuals is DataTable individuals. Imagine when there was no var, variable declaration like DataTable dtIndividuals are frowned upon; but now there's var, it's ok to use Hungarian notation again?


var is the most abused language feature


How I wish that compilers could at least advise the developer to use the concrete class' interface instead when declaring variables


To solve the problem above without incurring friction with var everything camp, you can make your concrete classes implement interfaces explicitly, thereby hiding the concrete class' implemented interface methods.
public class DomainAccess : IDomainAccess
{
.
.
. 
    // Implement interfaces explicitly. This will make Save not available on DomainAccess instance
    IDomainAccess.Save(object transientObject)
    {
        return _session.Save(transientObject);
    }
.
.
.
}


So when using the DomainAccess directly (via var), accessing its method will result to compile error..
using (var da = new DomainAccess(_sessionFactory))
{        
    da.Save(new Company{ ... }); // this will result to compile error, Save is inaccessible from concrete class DomainAccess
}


..so that would compel the developer to program to an interface instead::
using (IDomainAccess da = new DomainAccess(_sessionFactory))
{       
    da.Save(new Company{ ... }); // this will compile, Save is accessible from the IDomainAccess interface
}


Problem solved, that's programming to an interface. Using EIMI as a solution to this problem still has wrinkles in it, programming to an interface can be enforced but it's still easy to accidentally use the DomainAccess concrete class directly via the var keyword, the developer shall then have to change the variable declaration to use the interface instead, wasting time


There's a better approach, we can make the DomainAccess non-public, then just give the user an instance of IDomainAccess implementation from a factory
// remove the public keyword
class DomainAccess : IDomainAccess
{
    // implement interface methods
}


When used outside of DomainAccess project, this will not compile:
var da = new DomainAccess(_sessionFactory); // DomainAccess is not visible to other projects



So to give the user an IDomainAccess object, the dev shall use this pattern instead:
public class CompaniesController : Controller
{
    IDomainAccessFactory _daf;
    
    public CompaniesController(IDomainAccessFactory daf)
    {
        _daf = daf;
    }
    
    public JsonResult CompanySave(CompanyDto dto)
    {
        int savedId = 0;
        // OpenDomainAccess returns IDomainAccess instead of returning DomainAccess
        using (var da = _daf.OpenDomainAccess())
        {
            savedId = da.Save(new Company{...});            
        }                
        return Json(new { savedId = savedId });
    }        
}


DomainAccess cannot be instantiated directly anymore, it shall be built from a factory instead and return it through interface. Here's an implementation:
namespace UnityTestFriendlyDal
{
    public interface IDomainAccessFactory
    {
        IDomainAccess OpenDomainAccess();
    }
    
    public class DomainAccessFactory : IDomainAccessFactory
    {
        NHibernate.ISessionFactory _sessionFactory;

        public DomainAccessFactory(NHibernate.ISessionFactory sessionFactory)
        {
            _sessionFactory = sessionFactory;
        }
        
        // return through DomainAccess concrete class through IDomainAccess interface
        public IDomainAccess OpenDomainAccess()
        {
            return new DomainAccess(_sessionFactory);
        }
    }

    public interface IDomainAccess : IDisposable
    {
        object Save(object transientObject);
        IQueryable<T> Query<T>();
        T Get<T>(object id);
        T Load<T>(object id);
        void Evict<T>(object id);
    }
    
    // Don't make this public
    class DomainAccess : IDomainAccess
    {
        // Though you can use public/internal on method implementations (as long as you don't make the class DomainAcess public), 
        // it's advisable to use explicit interface method implementation instead

        // interface implementations here
    }
}


When you generate a method stub from a variable declaration using var that receives an interface, the generated method stub will naturally generates a method with a parameter of interface. You can reap the benefit of using interfaces when you can properly enforce the use of interfaces everywhere



Good read on "To var or not to var" :

http://shahinalborz.se/2011/01/to-var-or-not-to-var-c-implicit-variable-declaration/

http://www.brad-smith.info/blog/archives/336

http://weblogs.asp.net/stevewellens/can-the-c-var-keyword-be-misused

Good read on Programming To An Interface:

http://stackoverflow.com/questions/383947/what-does-it-mean-to-program-to-an-interface/384067#384067

http://www.codeproject.com/Articles/392516/Why-I-use-explicit-interface-implementation-as-a-d




Happy Coding!

1 comment:

  1. Nice article. And I agree, var is often overused. I see it in some code for almost every variable, but I end up missing ready recognition of the Types of all the vars that show up everywhere. In my own codeing I save it for when it is appropriate and/or necessary.

    ReplyDelete