Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I currently have a method in my repository that will run sql and map the reader to objects.

protected IEnumerable<T> Query<T, TFactory>(string sql, List<IDbDataParameter> parameters = null) where TFactory : IFactory<T>
{
    List<T> list = new List<T>();

    var factory = Activator.CreateInstance<TFactory>();
    var connection = (Db == null) ? Connection : Db.Connection;

    using (var manager = new DbCommandManager(connection, sql))
    {
        if (parameters != null)
            foreach (var parameter in parameters)
                manager.AddParameter(parameter);

        using (var reader = manager.GetReader())
        {
            while(reader.Read())
                list.Add(factory.CreateTFromReader(reader));
        }
    }

    return list;
}

To use the following you simply do:

public IEnumerable<Code> GetCodeWType(string CodeType, string Code)
{
    var sql = @"Select CODE_TYPE, CODE, DESCRIPTION From Codes  Where (CODE_TYPE = :1) AND (CODE = :2)";

    var parameters = new List<IDbDataParameter>();
    parameters.Add(DbFactory.GetParameter(":1", CodeType, DbType.String));
    parameters.Add(DbFactory.GetParameter(":2", Code, DbType.String));

    return this.Query<Code, CodeFactory>(sql, parameters);
}

Would love to hear thoughts and feedback on how I can improve.

share|improve this question
add comment

2 Answers

up vote 2 down vote accepted

It looks ok for me so, just a couple of suggestions:

First: given it is a protected method and not a private, it can be called be derived classes which probably could not be in the same assembly (this is true only if the class is public) then, you shouldn´t use optional parameters, they force you to recompile all when you change them.

Instead, use an overloaded method as follow:

protected List<T> Query<T, TFactory>(string query) where TFactory : IFactory<T>
{
    return Query<T, TFactoty>(query, Enumerable.Empty<IDbDataParameter>());
}

protected List<T> Query<T, TFactory>(string query, IEnumerable<IDbDataParameter> parameters) 
    where TFactory : IFactory<T>
{

It also help you to remove the checking for nulls.

Second, if you return an IEnumerable, the callers will have less features. The rule is you you should require the most generic and return the most specific implementation so, if you have a list you could return a List<>, IList<> or a Collection. Then, callers have more options.

My attempts is this one:

protected List<T> Query<T, TFactory>(string query) where TFactory : IFactory<T>
{
    return Query<T, TFactoty>(query, Enumerable.Empty<IDbDataParameter>());
}

protected List<T> Query<T, TFactory>(string query, IEnumerable<IDbDataParameter> parameters) 
    where TFactory : IFactory<T>
{
    var collection = new List<T>();

    var factory = Activator.CreateInstance<TFactory>();
    var connection = (Db == null) ? Connection : Db.Connection; // this line looks rare for me

    using (var manager = new DbCommandManager(connection, sql))
    {
        foreach (var parameter in parameters)
            manager.AddParameter(parameter);

        using (var reader = manager.GetReader())
        {
            while (reader.Read())
                list.Add(factory.CreateTFromReader(reader));
        }
    }

    return list;
}

Update:

One more thing about optional parameters, when you call the Query method with one argument, you really are passing two: the query and a null in 'parameters'. You have never pass nulls (or at least you have to try) because if you pass nulls then, you need to check for null values.

One more thing about the protected modifier, privates methods can trust that their parameter won´t be null but protected methods cannot do it. This is because private members will be called by other methods in the same class but protected method can be called by other methods in other classes.

share|improve this answer
    
+1 I would say that is a very general rule to return the most specific. As with most things it always depends... –  dreza Dec 11 '12 at 22:53
    
yes, they are just guides. –  lontivero Dec 12 '12 at 1:22
    
Thanks for all the good feedback. I am going to make said adjustments. The var connection line allows the user to either pass an open connection, or a wrapper object that contains a connection and transaction. I can post that class tomorrow so you can see it. Db would eventually turn into a unit of work. –  Chace Fields Dec 12 '12 at 2:17
add comment

As of code itself it is quite good, the only thing to note is that you can add a new() constraint on TFactory and just use var factory = new TFactory();.

But as with all self-written DB access frameworks I would suggest to switch to mature ORM frameworks like Entity Framework or NHibernate (if you haven't worked with them previously Entity Framework would probably be easier for you). They have a well-developed environment, proper unit-of-work management, good querying and tuning capabilities.

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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