Writing quality database code is a challenge. Most of your commands need to be expressed in SQL, which is a mildly complicated language made more complicated by minor variations across databases. Result sets often have a poor mapping to our business logic’s abstractions, especially in object-oriented languages. Thus, we have Object-Relational-Mapping tools, like Microsoft’s EntityFramework.

With an ORM, you use an object-oriented approach to fetching your objects, and could write something like: IList<HJFRate> rates = db.HJFRates.where(rate=>rate.typeOfUse == typeOfUse) to return all the rows as objects. There’s no concern about SQL injections, no need to process the result set directly. While ORMs can generate poor SQL, or create really inefficient data-access patterns, their ease-of-use is a big selling point.

Which is why Bob Zim was surprised to find this EntityFramework code in a C# web-service:

public ActionResult GetHJFUseTypeInfo(string HJFtypeOfUse)
{
    String query = "SELECT * FROM [dbo].[HJFFeeRateSchedule] u WHERE u.typeOfUse ='" + HJFtypeOfUse + "'";
    System.Data.Entity.Infrastructure.DbSqlQuery<HJFFeeRateSchedule> selectedUse = 
        db.HJFRates.SqlQuery(query);

    string TypeOfUse;
    decimal unitValue2016;
    decimal unitValue2017;

    TypeOfUse = selectedUse.FirstOrDefault().TypeOfUse;
    unitValue2016 = selectedUse.First().FieldUnitValueRate2016;
    unitValue2017 = selectedUse.First().FieldUnitValueRate2017;

    List<decimal> HJFUseTypeValues = new List<decimal>();
    HJFUseTypeValues.Add(unitValue2016);
    HJFUseTypeValues.Add(unitValue2017);

    return Json(HJFUseTypeValues, JsonRequestBehavior.AllowGet);
}

Pretty much everything here is completely wrong. The obvious issue, blinking like a neon sign, is the obvious SQL injection vulnerability. A vulnerability that, as implied by my “ORM 101” segment above, is completely unnecessary.

Keep in mind, further, that selectedUse is a query, not a data object. Each call to .First() re-executes the query, meaning this takes three round trips to the database. Also, mixing .First() (return the first result or error if there isn’t one) and .FirstOrDefault() (return the first result or a safe default value, typically null) is a bizarre choice.

Then, of course, we actually return the data, not as an object, but as an array of decimal values. Judging from the names of some of these fields, it looks like this code may have to change in 2018.

It’s a lot of bad to cram into one handler for an HTTP request, which brings us to our last problem with this code: controllers shouldn’t be doing data access directly. Normally, breaking that rule is worthy of a slap on the wrist, but in the context of this pile of everything is wrong, it might as well be brought up.

Bob adds:

This code was written by the senior dev on the project as well. He doesn’t work here anymore so I can’t ask him what his reasoning was.. but I did send him an email with the text “WHY!?!?!?” and a screenshot of this code. No response.

[Advertisement] BuildMaster allows you to create a self-service release management platform that allows different teams to manage their applications. Explore how!