4
\$\begingroup\$

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.

\$\endgroup\$
10
  • \$\begingroup\$ 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. \$\endgroup\$
    – greybeard
    Commented Dec 9, 2020 at 8:15
  • \$\begingroup\$ @greybeard I have updated the title to be more descriptive as required \$\endgroup\$ Commented Dec 9, 2020 at 10:59
  • \$\begingroup\$ "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? \$\endgroup\$
    – Flater
    Commented Dec 9, 2020 at 11:21
  • 1
    \$\begingroup\$ @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. \$\endgroup\$ Commented Dec 12, 2020 at 19:59
  • 1
    \$\begingroup\$ @RahulSharma: "it exists" shouldn't be the main reason why you choose to take a provably inefficient approach. \$\endgroup\$
    – Flater
    Commented Dec 13, 2020 at 11:59

2 Answers 2

5
+50
\$\begingroup\$

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.

\$\endgroup\$
8
  • \$\begingroup\$ 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. \$\endgroup\$
    – BCdotWEB
    Commented Dec 16, 2020 at 7:53
  • \$\begingroup\$ @BCdotWEB I didn't catch that; thanks. \$\endgroup\$
    – Reinderien
    Commented Dec 16, 2020 at 20:01
  • \$\begingroup\$ @Reinderien Thank you very much for the review. Could you give me an example of how the correctly implement the above connection initialization? \$\endgroup\$ Commented Dec 17, 2020 at 17:37
  • \$\begingroup\$ @Reinderien Is there any example that you can direct me to handle this properly? Thanks \$\endgroup\$ Commented Dec 18, 2020 at 15:42
  • \$\begingroup\$ @RahulSharma Sure; I linked to the MySQL documentation on connection pooling. \$\endgroup\$
    – Reinderien
    Commented Dec 18, 2020 at 16:58
4
\$\begingroup\$

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.

\$\endgroup\$
6
  • \$\begingroup\$ 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. \$\endgroup\$ Commented Dec 12, 2020 at 19:53
  • \$\begingroup\$ 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. \$\endgroup\$ Commented Dec 12, 2020 at 19:55
  • \$\begingroup\$ 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. \$\endgroup\$ Commented Dec 12, 2020 at 20:16
  • \$\begingroup\$ 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. \$\endgroup\$
    – BCdotWEB
    Commented Dec 13, 2020 at 15:46
  • \$\begingroup\$ 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? \$\endgroup\$
    – BCdotWEB
    Commented Dec 13, 2020 at 15:51

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.