This post is part of a series of blog posts about bad codes or bad practices I’ve come across. Note that this is my personal opinion and some of the things I mention could be debatable.
Post 1: Code review, why it matters
Post 2: Not including JS, CSS and static content files in project
Post 3: Let’s just not unit test our code
Post 4: Don’t pass dependencies into helper methods
Post 5: Inline SQL query statement in static helper class
In my previous project, when I reviewed code, I came across these ‘helper’ classes many times .i.e
public static class ArticleHelper
{
private const string ConnectionName = "SampleDatabase";
public static string GetArticleNameById(Guid articleId)
{
using (var conn = new SqlConnection(DbUtilities.GetConnString(ConnectionName)))
{
var query = @"
select Name
from dbo.Article
where Id = @ArticleId;";
return conn.Query<string>(query, new { ArticleId = articleId }).FirstOrDefault();
}
}
}
There are many problems doing it this way:
- It’s not abstracted. It means whatever uses this method depends on the concrete implementation of this class. It also violates the “Dependency Inversion principle” from the S.O.L.I.D principles which states that Depend on Abstraction not on concretions.
- It’s not unit testable. You cannot unit test the helper class itself and any method that uses this class.
- Inline SQL is never a good idea, use stored procedure at least or an ORM like EF.
The right way to do this is to create an interface and inject the dependency via constructor wherever it is used.
public interface IArticleRepository
{
string GetArticleNameById(Guid articleId);
}
public class ArticleRepository : IArticleRepository
{
public string GetArticleNameById(Guid articleId)
{
// Your code goes here
}
}