Interfaces vs Abstract Classes

[[)roi(]]

I hijacked his comment: it's the maturity, skill, experience and moral of the team (read: human factor) that has the biggest influence on the state of the codebase.

This.

Could you guys elaborate on this... Or should a new thread be created?

Two things; first is related to the "law of the instrument" (Maslow quote)

"I suppose it is tempting, if the only tool you have is a hammer, to treat everything as if it were a nail."

Having mastered DI and IoC it now becones the defacto "hammer" for every problem, and nobody questions it. Like the Maslow quote, you can beat everything with a hammer, but fairly often it's absolutely the wrong tool.

Second is the "jenga" reference (also related to maturity and experience). Some teams hammer out code without ever writing a test; so inevitably someone ends up strapping on tests at the end... but when a codebase isn't approached with testability in mind, you inevitably end up having to break and redesign parts. Like jenga the more you tug, the more likely the whole construct is going to tumble.

 

[Solarion]

Been away for a few days. What's going on in this thread.

Oh wow!

/gets coffee, lots of it!

 

[Solarion]

Taking Spacerats and Roi's earlier example, could I do something like this

 public class MyDAL
    {
        private readonly IDBManager db;
        SqlParameter[] parameters; //Parameters list
        string sqlQuery; // Is actually just the name of the Stored Proc eg.  spInsertEmployee


        public MyDAL(IDBManager db, SqlParameter[] parameters, string sqlQuery)
        {
            this.db = db;  //  <---- Contructor injection (DI)
            this.sqlParameter = sqlParameter;
            this.sqlQuery = sqlQuery;
        }

        public void InsertEmployee()
        {
            try
            {
                db.executeInsert(sqlQuery, parameters);
            }
            catch (Exception exc)
            {
                //handle the exception with the Logger
            }
        }
    }

Am I on the right track here?

 

[Hamster]

No, you can't have half the parameters injected and the other half done yourself.

Move the SqlParameters to the InsertEmployee function and the sqlQuery to a constant so only the IDBManager parameter is left in the constructor.

 

[Bernie]

Off Topic:

One of the most informative, educational and enjoyable threads I have seen on MyBB (for me at least). Thank you to all who have contributed - Back to lurking now

 

[Randy_The_Squirrel]

Now this is the sort of thing where I get a bit upset:

string sqlQuery; // Is actually just the name of the Stored Proc eg.  spInsertEmployee

sqlQuery is the name of a string object, not the name of the stored proc. The stored proc is built in there and then the whole string is sent to the server. I see these confusing comments a lot, and it confuses me on first reading.

This is why I struggle- because of all the over-complication that takes place a lot of the time. Like someone said to me last week- this stuff is very abstract.

 

[Hamster]

Now this is the sort of thing where I get a bit upset:

string sqlQuery; // Is actually just the name of the Stored Proc eg.  spInsertEmployee

sqlQuery is the name of a string object, not the name of the stored proc. The stored proc is built in there and then the whole string is sent to the server. I see these confusing comments a lot, and it confuses me on first reading.

This is why I struggle- because of all the over-complication that takes place a lot of the time. Like someone said to me last week- this stuff is very abstract.

How the hell does that confuse you?!

 

[Spacerat]

This.


Second is the "jenga" reference (also related to maturity and experience). Some teams hammer out code without ever writing a test; so inevitably someone ends up strapping on tests at the end... but when a codebase isn't approached with testability in mind, you inevitably end up having to break and redesign parts. Like jenga the more you tug, the more likely the whole construct is going to tumble.

Yes, testability (and of course extensibilty) should be part of the design process..

 

[Spacerat]

Taking Spacerats and Roi's earlier example, could I do something like this

 public class MyDAL
    {
        private readonly IDBManager db;
        SqlParameter[] parameters; //Parameters list
        string sqlQuery; // Is actually just the name of the Stored Proc eg.  spInsertEmployee


        public MyDAL(IDBManager db, SqlParameter[] parameters, string sqlQuery)
        {
            this.db = db;  //  <---- Contructor injection (DI)
            this.sqlParameter = sqlParameter;
            this.sqlQuery = sqlQuery;
        }

        public void InsertEmployee()
        {
            try
            {
                db.executeInsert(sqlQuery, parameters);
            }
            catch (Exception exc)
            {
                //handle the exception with the Logger
            }
        }
    }

Am I on the right track here?

Hmm no. At least to me...

1) Where is your transaction control? Or is that done at higher level? If your app does multiple updates to the db then you need to do trx control in order to ensure updates are atomic.

2) Where is the logger? Is it Ambient Context like your previous example or injected (i dont see it injected). How is the calling method supposed to know if the method was successful or not? You are not rethrowing or returning a value to indicate such.

3) As Hamster said, your ctor should not be accepting all those params. Those should be internal to the method. I would expect only the dbmanager to be injected...

 

[Spacerat]

OK maybe it is a good idea to create a step by step thread on the development of a generic DAL. I could if there is interest.
I write one many many years ago before EF existed. It runs on Compact Framework 1 right up to current version of .NET Framework and is database agnostic. It is pretty fast as it uses Relfection.Emit to build and compile the data reader at runtime. It also needs to be refactored a bit but you will get the picture.

You use it like this:

You define the entity structure as below. I created attributes that allows the DAL to determine the DB-related metadata, although you can also get it from any other source and inject it into the dal ctor.

[DbTableName("SOME_TABLE")]
[DbSelectViewName("SOME_TABLE_OR_VIEW")]
public class MyTableRow
{
  [DbColumnName("ID")]
  public long Id { get; set; }

  [DbColumnName("NAME")]
  public string Name { get; set; }

  // etc
}

Then to use the DAL you do

IDbConnected myConnector = new SqlDbConnector(connectionString);
myConnector.Open();

ObjectDal<MyTableRow> dal = new ObjectDal<MyTableRow>(myConnector);  // <--- Allows for any database
IList<MyTableRow> list = dal.Get(); // <--- returns all

or

MyTableRow row = new MyTableRow
{
  Id = 1,
  Name = "Name",
};
dal.Insert();

 

[Solarion]

No, you can't have half the parameters injected and the other half done yourself.

Move the SqlParameters to the InsertEmployee function and the sqlQuery to a constant so only the IDBManager parameter is left in the constructor.

Roger that!

Hmm no. At least to me...

1) Where is your transaction control? Or is that done at higher level? If your app does multiple updates to the db then you need to do trx control in order to ensure updates are atomic.

2) Where is the logger? Is it Ambient Context like your previous example or injected (i dont see it injected). How is the calling method supposed to know if the method was successful or not? You are not rethrowing or returning a value to indicate such.

3) As Hamster said, your ctor should not be accepting all those params. Those should be internal to the method. I would expect only the dbmanager to be injected...

It is higher level yes. The next level up is:

 public void executeInsertQuery(SqlParameter[] sqlParameter, string _query)
        {
            using (SqlConnection connection = new SqlConnection(base.ConnectionString))
            {
                connection.Open();

                SqlCommand command = connection.CreateCommand();
                SqlTransaction transaction;

                transaction = connection.BeginTransaction();

                command.Connection = connection;
                command.Transaction = transaction;

                if (sqlParameter != null)
                {
                    command.Parameters.AddRange(sqlParameter);
                }

                try
                {
                    command.CommandType = CommandType.StoredProcedure;
                    command.CommandText = _query;
                    command.ExecuteNonQuery();

                    transaction.Commit();
                }
                catch (Exception ex)
                {
                    transaction.Rollback();
                    ErrorLogger.LogMessage("Job Manager", "DBManager", "executeInsertQuery()", ex.Message);
                    throw;
                }
            } 
        }

And roger that too!

Edit: The above will eventually be fixed up with a proper Logging system like you guys have detailed earlier. This is just to give you an idea of what's going on.

 

[Solarion]

Somewhat fixed. It's still wrong I think.

public class MyDAL
{
    private readonly IDBManager db;

    public MyDAL(IDBManager db)
    {
        this.db = db;
    }

    public void Insert(SqlParameter[] sqlParameter, string sqlQuery)
    {
        db.executeInsertQuery(sqlQuery, sqlParameter);
    }

    public void Update(SqlParameter[] sqlParameter, string sqlQuery)
    {
        db.executeUpdateQuery(sqlQuery, sqlParameter);
    }
}

As I mentioned earlier, there is still one level up which actually handles the transactions, inside DBManager.

Edit: As per your earlier example spacerat, think of DBManager as one of your ConsoleLogger or FileLogger classes; the final link in the chain where the actual transactions or handling get done.

Edit: I still think I need to back track a little and this is where my confusion comes in. I know what I want done, but I'm lost in the woods as to what is a better way to do it. I reworked the above to do this. Do you see my confusion?

public class MyBAL
{
    private readonly IDAL db;

    public MyBAL(IDAL db)
    {
        this.db = db;
    }

    public void InsertEmployee(SqlParameter[] sqlParameter, string sqlQuery)
    {

        db.InsertEmployee(sqlQuery, sqlParameter);
        // Alternately do the INSERT right here and then?
    }

    public void UpdateJob(SqlParameter[] sqlParameter, string sqlQuery)
    {
        db.UpdateJob(sqlQuery, sqlParameter);
        // and also do the UPDATE right here?
    }
}

 

[Solarion]

One question I do have. Do you guys handle all the CRUD inside your DAL or do you have another layer, a final layer that does that? If that's the case then, in my last example above, that would be were the buck stops. Everything gets finalized inside the DAL.

 

[Spacerat]

One question I do have. Do you guys handle all the CRUD inside your DAL or do you have another layer, a final layer that does that? If that's the case then, in my last example above, that would be were the buck stops. Everything gets finalized inside the DAL.

The low-level db crub stuff typically happens in the DAL. The IxxxDAL should be ctor-injected into the higher layer so that the higher layer is not coupled to a particular implementation of the DAL. I.e. the IxxxDAL interface abstracts the higher layer from the DAL implementation. The DAL can be any implementation. A Mock<IxxxDAL>, or a DbDal or an implementation that does remote JSON/REST calls to a webservice if need be. Or a combo of them.

I prefer to make the IxxxDAL interface very clear as to what each method's intent is. I.e.

void Insert(Entity ent);
void Update(Entity ent);
void Delete(Entity ent);
IList<Entity> GetAll();
IList<Entity> GetAllForParent(ParentEntity parentEntity);

Thus, every implementation of this interface needs to return exactly what the caller expects. Whether it comes from a db or webservice etc.
Returning IQueryable is considered a code smell. http://blog.ploeh.dk/2012/03/26/IQueryableTisTightCoupling/ BTW Mark Seeman is one of my most read bloggers. Big on FP as well...

 

[_kabal_]

The low-level db crub stuff typically happens in the DAL. The IxxxDAL should be ctor-injected into the higher layer so that the higher layer is not coupled to a particular implementation of the DAL. I.e. the IxxxDAL interface abstracts the higher layer from the DAL implementation. The DAL can be any implementation. A Mock<IxxxDAL>, or a DbDal or an implementation that does remote JSON/REST calls to a webservice if need be. Or a combo of them.

I prefer to make the IxxxDAL interface very clear as to what each method's intent is. I.e.

void Insert(Entity ent);
void Update(Entity ent);
void Delete(Entity ent);
IList<Entity> GetAll();
IList<Entity> GetAllForParent(ParentEntity parentEntity);

Thus, every implementation of this interface needs to return exactly what the caller expects. Whether it comes from a db or webservice etc.
Returning IQueryable is considered a code smell. http://blog.ploeh.dk/2012/03/26/IQueryableTisTightCoupling/ BTW Mark Seeman is one of my most read bloggers. Big on FP as well...

Listen to this guy, he speaks sense.

extending on the above example, you can make your DAL implementations fool proof.

interface IDAL<T, K> {
   void save(T entity);
   T findOne(K identifier);
   //etc
}

you can now go 2 routes:

1. You know you will most likey never change how data is accessed, ie you "hard-code" to use a SQL DB

class CustomerDAL extends BaseDBDAL implements IDAL<Customer, Long> {

    //now get your IDE to auto generate the below method implementations
    void save(Customer entity) {
        throw new NotYetImplemented();
    }
    Customer findOne(Long identifier) {
        throw new NotYetImplemented();
    }

}

2. You make your DAL's semi datasource agnostic. (I would argue that this usecase is mostly not needed, and all you really want is a consitent programming model)

interface CustomerDAL extends IDAL<Customer, Long> {
}

class DBCustomerDAL extends BaseDBDAL implements CustomerDAL {
    
    //now get your IDE to auto generate the below method implementations
    void save(Customer entity) {
        throw new NotYetImplemented();
    }

    Customer findOne(Long identifier) {
        throw new NotYetImplemented();
    }
}
OR
class WebServiceCustomerDAL extends BaseWebServiceDAL implements CustomerDAL {
  
  //now get your IDE to auto generate the below method implementations
    void save(Customer entity) {
        throw new NotYetImplemented();
    }

    Customer findOne(Long identifier) {
        throw new NotYetImplemented();
    }
}

 

[[)roi(]]

Yes, testability (and of course extensibilty) should be part of the design process..

Yet none of the approaches here specifically address that!
More so the over reliance of action patterns, partial functions is an anti-pattern in this regard.

 

[Spacerat]

Yet none of the approaches here specifically address that!
More so the over reliance of action patterns, partial functions is an anti-pattern in this regard.

DI and abstraction through interfaces support mocking, a widely used and accepted test strategy.

 

[[)roi(]]

DI and abstraction through interfaces support mocking, a widely used and accepted test strategy.

DI isn't the issue; the DI concept is nothing new (refer Schönfinkelization) and exists across multiple paradigms, for example: in FP it's called currying & partial application.

Implementation is however a different matter, for example: building this with action patterns, hidden input / output, no preconditions, etc. is always going to make testing this a pain; functionality that cannot be isolated is never easy to test; mocking frameworks certainly don't change that.

 

[Spacerat]

DI isn't the issue; the DI concept is nothing new (refer Schönfinkelization) and exists across multiple paradigms, for example: in FP it's called currying & partial application.

Implementation is however a different matter, for example: building this with action patterns, hidden input / output, no preconditions, etc. is always going to make testing this a pain; functionality that cannot be isolated is never easy to test; mocking frameworks certainly don't change that.

Dont get what you are trying to say. Care to expand? By means of an example?

 

[[)roi(]]

Dont get what you are trying to say. Care to expand? By means of an example?

Sure,
As we know, a function whose return value is determined solely by its input values and has no side effects; is always easy to test. Inversely actions without a return value, with hidden input / outputs and unmanaged side effects is very difficult to test.

Simple example is how we'd go about automating a test for "WriteLine"; because whilst it's inputs are clearly defined, it has no obvious output; and its return type is "void"; yet it affects state (stdout) & can throw exceptions. Writing tests for WriteLine, or wrapping every WriteLine in a try/catch block seems like a foolish endeavour because rarely do we care if it works or not; but most actions are certainly not the same.

Surely ascertaining whether "db.executeInsertQuery(sqlQuery, sqlParameter)" worked is important; yet its design makes it inherently difficult to test. It becomes a "set up the world" scenario, where complex pre/post test actions have to accompany a test in order to recreate the "perfect world" scenario prior to testing.

These design issues are rarely contained at methods; meaning anti-test patterns most often repeat. Test Mocks are non production plugin alternatives for testing, they certainly don't address design limitations.