ExecuteScalar maybe?



  • My coworker decided to write a little helper function to supplement the already written GetData()

     

        private DataTable GetData(string sql, string connectionString)
        {
            DataSet ds = new DataSet();
            using (SqlConnection con = new SqlConnection(connectionString))
            {
                con.Open();

                using (SqlCommand cmd = new SqlCommand(sql, con))
                {
                    using (SqlDataAdapter adapt = new SqlDataAdapter(cmd))
                    {
                        adapt.Fill(ds);
                    }
                }
                con.Close();
            }
            return ds.Tables[0];
        }
        private string GetOneItemFromDB(string sql, string connectionString)
        {
            DataTable dt = GetData(sql, connectionString);
            return dt.Rows[0][0].ToString();

        }



  • I know how you feel. I once saw something like that, only the command was hard coded to "SELECT * FROM [some table]". And just like your case, they used it to get the value in first cell from the first returned row. That was used to fill some field in a form. People started wondering why the application grew slower with time (the table used in the query had accumulated over a hundred thousand lines over the years...)



  •  Well, to be fair, it doesn't say inside the function that it fetches all rows. You COULD call GetOneItemFromDB with sql = "... LIMIT 1" or something. Not that I had much hope anyone who wrote this would do that.

    Maybe you should extend GetOneItemFromDB() so that it checks for LIMIT clauses (or whatever your equivalent is) and throws a RetardAtKeyboardException if not present.



  • I'm assuming that is C#, which I have never used, and I did my very first proper (yet very basic) SQL yesterday (loading a table from a flat file using PHP) but the problem with that code leaped out at me as soon as I read it.



  • @GettinSadda said:

    I'm assuming that is C#(...)

    It is. Many people praise it for ADO.NET, the set of libraries used for working with databases. Well, you've just seen a horrible case of abuse with one of its most used classes, the DataSet. And as they say, it's not the language nor the framework that makes the coder...



  • I don't really see the WTF. I've written a similar function, for the occasions when I know the query is only going to return one result. It's a useful function to have, as evidenced by the fact that there's a library function to do it. The only problem I can see is the programmer hadn't heard of ExecuteScalar. I don't know how forgivable that is since I've never programmed in C# before so I don't know how easy it would be to notice that this function exists. But it's not as though they reimplemented it in a hugely WTFy buggy way, it's just a simple two-line wrapper. So it's not a WTF, just presumably a beginner with this framework who is not familiar with all the functionality yet and didn't search the documentation well enough. Yeah, maybe they should have searched better, maybe they should have had some kind of assertion if the query returned more than one row, but I wouldn't call it a WTF. Unless I missed something. You may now proceed to tell me what an idiot I am for missing whatever it is.



  • What an idiot you are for missing whatever it is...



  • Only real mistakes I can see right from that is not disposing the datatable and not using the built-in methods that exist explicitly for the purpose, but really in itself it's not too bad depending on the SQL that goes in. True it's re-inventing the wheel, but it's a very simple wheel and it spins just fine so who cares? ;)

    GetData I could say a few more for, but we're looking for WTF's purely related to the GetOneItem method so that's all I got.



  • Wrapping a SQL statement around a connection doesn't sound WTFey to me. I probably would've put in a connection retry attempt loop as well just to be extra paranoid. It's hard to say if GetOneItemFromDB is a WTF or not without knowing how the SQL server driver works internally (the datatable may be set up to fetch each row one at a time with a cursor in which case the performance hit from retriving subsequent datarows may be optimized out). But I think we all know the real enterprisey solution to this is to use a LINQ to SQL adapter for this database or possibly typed data sets if you're stuck in .NET Framework 2.0-land.


Log in to reply