Repository pattern with Unit of work using ADO.NET in C# to authenticate user from database

Question

So I am using the latest version of Dapper and it works great for mapping my POCO classes but there are some scenarios where I need to return a DataSet instead of strongly typed classes back to my service.

Is this correct approach for repository pattern with Unit of work using ADO.NET in C# to authenticate user from database?

I am following the UoW pattern but I am using ADO.Net, so I am not sure if that is the correct approach. I will give the relevant classes that I am using to setup my infrastructure and Unit of Work objects:

Infrastructure class (where I am setting up my DB connection):

public class ConnectionFactory: IConnectionFactory
{
 private readonly string epaperconnectionString = ConfigurationManager.ConnectionStrings["MyDB"].ConnectionString;
 private MySqlConnection _myDBString;
 public IDbConnection GetMyDBConnection
 {
 get
 {
 _myDBString = new MySqlConnection(epaperconnectionString);
 return _myDBString;
 }
 }
}

Unit of Work class:

public class UnitOfWork : IUnitOfWork
{
 public UnitOfWork(IMyDBRepository mydbRepository)
 {
 MyDBRepository = mydbRepository;
 }
 void IUnitOfWork.Complete()
 {
 throw new NotImplementedException();
 }
 public IMyDBRepository MyDBRepository { get; }
}

MyDBRepository class:

public sealed class MyDBRepository : IMyDBRepository
{
 IConnectionFactory _connectionFactory;
 Helper helper = new Helper();
 
 public MyDBRepository (IConnectionFactory connectionFactory)
 {
 _connectionFactory = connectionFactory;
 }
 
 public async Task<DataSet> checkForUser(string useremail, StringBuilder EncPassword)
 {
 string query = string.Format("SELECT id FROM USER WHERE PASSWORD=@pwd AND Email=@emailid"); 
 DynamicParameters param = new DynamicParameters();
 param.Add("@emailid", useremail.Trim());
 param.Add("@pwd", EncPassword.ToString().Trim());
 IDataReader list = await SqlMapper.ExecuteReaderAsync(_connectionFactory.GetMyDBConnection, query, param, commandType: CommandType.Text);
 DataSet dataset = helper.ConvertDataReaderToDataSet(list);
 return dataset;
 }
}

I am assuming here that SqlMapper automatically takes care of the opening and disposing the SQL connection so I am not explicitly using the technique to take care of disposing of SQL connections.

My helper ConvertDataReaderToDataSet is:

public DataSet ConvertDataReaderToDataSet(IDataReader data)
{
 DataSet ds = new DataSet();
 int i = 0;
 while (!data.IsClosed)
 {
 ds.Tables.Add("Table" + (i + 1));
 ds.EnforceConstraints = false;
 ds.Tables[i].Load(data);
 i++;
 } 
 return ds;
}

My Service class looks like:

public class MyService : IMyService
{
 IUnitOfWork _unitOfWork;
 #region CTOR
 public MyService(IUnitOfWork unitOfWork)
 {
 _unitOfWork = unitOfWork;
 }
 
 public async Task<result> checkForUserService(string useremail, StringBuilder EncPassword)
 {
 var obj = await _unitOfWork.MyDBRepository.checkForUser(useremail, EncPassword);
 if (obj.Tables[0].Rows.Count == 0)
 {
 //Incorrect username/password
 result.flag=false;
 }
 else
 {
 //Do stuff
 result.flag=true;
 }
 }
}

I call the above service from a MVC project (4.6.1):

public class TestController : Controller
{
 IMyService _myService;
 ISubscriptionService _subscriptionService;
 
 public TestController()
 {
 }
 public TestController(IMyService myService)
 {
 _myService = myService;
 }
 
 // GET: check user test
 public ActionResult checkUserTest()
 {
 string EmailId = "[email protected]";
 StringBuilder EncPassword = new StringBuilder();
 //Using a standard hashing algorithm 
 EncPassword.Append("encodedpassword");
 var resultData = _myService.checkForUserService(EmailId, EncPassword).Result;
 if(resultData.flag)
 {
 //Set session variable here
 System.Web.HttpContext.Current.Session["loggedinemail"] = resultData.EmailId;
 }
 string jsonDataSet = JsonConvert.SerializeObject(resultData);
 return Json(jsonDataSet, JsonRequestBehavior.AllowGet);
 } 
}

I wanted any suggestions/design changes that I can do in order to use the repository pattern with Unit of Work with Dapper correctly. It is working for me at the moment but I am not sure if the implementation of the repository pattern is done correctly with Unit of work since I am using ADO.NET.

• greybeard: Welcome to CodeReview@SE. Please heed How do I ask a Good Question? and title your question for what the code presented is to accomplish.
• Rahul Sharma: @greybeard I have updated the title to be more descriptive as required
• Flater: "there are some scenarios where I need to return a DataSet instead of strongly typed classes" Why your data set's columns not be expressed as a concrete class?
• Rahul Sharma: @PeterCsala Thank you for your concern but I might have mislead the readers on that part. I am using a standard hashing algorithm for this purpose. Sorry my bad there.
• Flater: @RahulSharma: "it exists" shouldn't be the main reason why you choose to take a provably inefficient approach.

Answer 1

Side-effects

On its own, this:

private MySqlConnection _myDBString;
public IDbConnection GetMyDBConnection
{
 get
 {
 _myDBString = new MySqlConnection(epaperconnectionString);
 return _myDBString;
 }
}

is a deeply bad idea. Getters should usually be trusted to have no side-effects that mutate the class.

The only exception to this I would make is if you're using _myDBString as a cache, which you're not. To do this you would check to see if that member has already been set, and if so, return the member without a new call to MySqlConnection.

Even so, such caching is not the right way to go about reducing connection load; instead use an actual connection pool. For more information on this approach a reasonable starting point is the MySQL Connector Documentation.

Also _myDBString is the wrong name. It's not a connection string; it's a connection.

• BCdotWEB: The ID is used: ["loggedinemail"] = resultData.EmailId;. But the code is needlessly overcomplicated, which is why it is almost impossible to figure out what it exactly does; yet in its core it is really simple.
• Rahul Sharma: @Reinderien Thank you very much for the review. Could you give me an example of how the correctly implement the above connection initialization?
• Rahul Sharma: @Reinderien Is there any example that you can direct me to handle this properly? Thanks
• Reinderien: @RahulSharma Sure; I linked to the MySQL documentation on connection pooling.
• Rahul Sharma: @Reinderien Thank you very much. I shall take a look into it. Other than that, is there any suggestions that you can give on the above code structure Or does the implementation look right to you?

Answer 2

Some quick remarks:

• epaperconnectionString doesn't follow the proper naming conventions for compound words. Also: why wouldn't you store all your configuration settings in a dedicated class? (I'm not a fan of seeing ConfigurationManager all over the place: if you ever need to change a setting's name you need to look in multiple places, whereas it would be a single location if you'd centralized all those settings.)

• Helper is too generic a class name. So is MyDBRepository. So is resultData. Don't get me started on obj.

• list isn't the correct name for an IDataReader.

• checkForUser doesn't follow the naming guidelines. Ditto checkForUserService (that method name is simply baffling, BTW).

• What is result? Why doesn't it follow the naming conventions? Why does it have a property flag that also doesn't follow naming conventions?

---

But mostly this looks to me like a solution in search of a problem. Why work with DynamicParameters and SqlMapper and IDataReader etc. when you could just as easily use "proper" Dapper code, return a class or an int (whatever type "id" is) and then convert that to a DataSet?

And for what? You end up with var obj = await _unitOfWork.MyDBRepository.checkForUser(useremail, EncPassword); and then based on obj.Tables[0].Rows.Count == 0 you set result.flag to true or false. And so on. I see a massive amount of code that pointlessly uses a DataSet etc. for what (as far as I can deduct) a ridiculously simple piece of logic: you need the ID belonging to a provided email address.

I can throw away 90% of your code (including the overkill of a unit of work and a repository) and replace it with a simple service class that contains a single method with a simple Dapper query that returns an int? (or whatever type that ID is). Why did you overengineer it in the way you did?

---

You say:

there are some scenarios where I need to return a DataSet instead of strongly typed classes

Nothing in the code you provide here proves this.

• Rahul Sharma: Thank you for review on this. I have understood the naming convention and I will do the changes proposed by you. Can you give me an example of getting the configuration in the correct way. I encounter the object not set to an instance of an object error on this line in: ConfigurationManager.ConnectionStrings["MyDB"].ConnectionString;. I am working with DynamicParameters, SqlMapper because they are integral part of Dapper so I need them in my code. You are right about the overengineer part but this is sample piece from a huge library of methods. I wanted a service based model.
• Rahul Sharma: Basically, all the business logic goes into the Service class. The database class (repository) is only for performing CRUD operations. I could have made it into all a simple service class as you proposed but then it would have been cumbersome to put business logic there. This is a small sample from a huge codebase. All I wanted to know is if the repository pattern with unit of work is properly being followed with all resource management being done (in the repository class). Thanks again.
• Rahul Sharma: Also for there are some scenarios where I need to return a DataSet instead of strongly typed classes: I have some scenarios where I need to perform manipulations from the data returned back from the database before finally sending it out to the client. This can be done using a DTO if I were using a service based web api but since this a .NET Standard libary project, I am left with the option to get the dataset and then perform manipulation on it before mapping to the c# class. If there is a better and efficient way to do it, then I would like to know.
• BCdotWEB: I have used Dapper countless times and the only time I have used DynamicParameters was in one exceptional case, and I have never used its SqlMapper. Those things are there for exceptional cases, and your code isn't that.
• BCdotWEB: Nothing is stopping you from having a "DAO" class that still uses DTOs. So what if you need to do data manipulation post-db retrieval? Just return a "raw"/"simplified" DTO and transform that to a more complex DTO in the class with your business logic. A DataSet is IMHO absolutely the wrong thing to use here, not in the least because you lose so much and gain nothing. "since this a .NET Standard library project": why would this be relevant?