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 logics abstractions, especially in object-oriented languages. Thus, we have Object-Relational-Mapping tools, like Microsofts EntityFramework.

With an ORM, you use an object-oriented approach to fetching your objects, and could write something like: IList rates = db.HJFRates.where(rate=>rate.typeOfUse == typeOfUse) to return all the rows as objects. Theres 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 selectedUse = 
        db.HJFRates.SqlQuery(query);

    string TypeOfUse;
    decimal unitValue2016;
    decimal unitValue2017;

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

    List HJFUseTypeValues = new List();
    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 isnt 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.

Its a lot of bad to cram into one handler for an HTTP request, which brings us to our last problem with this code: controllers shouldnt 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 doesnt work here anymore so I cant 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.

https://thedailywtf.com/articles/object-relational-mangling