asp.net mvc 5 - creating instance of same class in every method in a class

Question

Im have a question.

I have just started a job with a new company that has one "Senior developer".

In a class he is doing this (Example):

class CarController{


public Cars GetCars(){

    **Connection connect = new Connection(); // Create DB connection**

    //and here some fetching of the data and returning it with a HTTP Response
}



public Cars GetCar(int id){

    **Connection connect = new Connection(); // Create DB connection**

    //and here some fetching the car with id, returning it with a HTTP Response
}

//and this pattern continues here...

}

I cant imagine that this is a good practise to keep creating the same instance of a class in each and every method to make call to the Database?? Isn't it also bad for the memory to keep creating instances everywhere?

In my opinion here we could use the repository pattern and only give access to the database class to the class the implements the Interface. Right now we expose the Database class to the controller and the code is tight coupled!

He says that he cannot understand the concept of the respository an why Interfaces should be used in this case.

How do you convince a senior to refactor the code and also prove to him that his coding has no good structure?

How can i explain the cons in his code above in another way?

Answer 1

I cant imagine that this is a good practice to keep creating the same instance of a class in each and every method to make call to the Database? Isn't it also bad for the memory to keep creating instances everywhere?

It actually IS the best practice in this case. .NET pools database connections, so creating them generally isn't an expensive process. Reusing connections, however, can cause problems.

However, you should be disposing the connection as soon as you're done with it. A handy way of doing that is with a using statement:

using(Connection connect = new Connection()) // Create DB connection
{
       //and here some fetching of the data and returning it with a HTTP Response
}  // the connection is closed and disposed of here.

In my opinion here we could use the repository pattern and only give access to the database class to the class the implements the Interface. Right now we expose the Database class to the controller and the code is tight coupled!

Sure, that might be an improvement, but it's orthogonal to your first question. The repository should create the connection when needed and dispose of it when done.

How do you convince a senior to refactor the code and also prove to him that his coding has no good structure?

If you've just started, you might not be able to if you approach it that way. Just remember that most people (regardless of experience) don't like others to criticize their work. Instead, offer an alternative solution and offer some reasons why it's an improvement over the current design. A pragmatic approach would be to provide a situation in which the pattern in place causes problems (unit testing is the most obvious) and illustrate why a different design would solve that problem.

Answer 2

Instantiating objects directly isn't always bad

Creating object instances without using an IoC container is not always bad (imagine if we used Container.Resolve every time we wanted to create a string!) If a dependency is not a volatile dependency then it may not be a great candidate for DI.

No it is not expensive

Creating and disposing database connections over and over is not expensive (they are allocated from a pool and kept open anyway) and in fact is considered best practice.

Inject a DB connection?

In the case of a database connection, that connection must be tightly handled and disposed in a quick timeframe, so there is an argument to be made not to inject it. You certainly would not want the caller (or the composition root) to instantiate the connection and then require the consumer of the connection to Dispose it; that is a violation of of two commonly-held principles:

1. Scope that creates an instance should also destroy it
2. Injected instances should have longer lifespan than the object using the injected instance

Inject a factory?

Perhaps you could inject a database connection factory, but this is a little problematic because IDBConnection doesn't contain all the members found in SqlConnection. So if you want to use DI here you'd either need to supply a concrete instance (which is sealed, by the way, so no shims) or you'd need to strip away all the SQL-only features from your implementation. Same is true of SqlCommand -- IDBCommand only has a very small subset of the properties, and none of the async methods at all.

What would it gain you?

Would it get you loose coupling? Not really. You'd have sort of a fake loose coupling, but there is an implicit logical coupling between the data access classes and the SQL server database (for example, they have a schema in common). It's not like you could replace your SqlClient with an OracleClient and expect it to work. I've seen lots of folks try it, for years; if you are still trying you haven't caught on. It doesn't work. A feature-rich database client is going to have some hard dependencies on the database platform, period.

Would it be easier to unit test? Not sure. As I noted above, you can't really write shims or stubs because the SqlConnection, SqlCommand, SqlParameter, etc. are all sealed, and all have concrete methods not found in the interfaces. so no matter what, you are still going to be stuck mocking instead of stubbing.

What is the long term plan?

Maybe the plan is to replace all that low level SQL stuff with an EF at some point in the future. If so, it would be waste of time to retrofit DI into the DAL at this point, because it would all be thrown away in the end.

My conclusion

This is a judgment call. Your colleague is entitled to make it. I would focus on other, more obvious, lower-hanging fruit for refactoring.