CodeSOD: A Dropped Down DataSet

While I frequently have complaints about over-reliance on Object Relational Mapping tools, they do offer key benefits. For example, mapping each relation in the database to a type in your programming language at least guarantees a bit of type safety in your code. Or, you could be like Nick L‘s predecessor, and write VB code like this.

For i As Integer = 0 To SQLDataset.Tables(0).Rows.Count - 1
     Try 'Handles DBNull
         Select Case SQLDataset.Tables(0).Rows(i).Item(0)
             Case "Bently" 'Probes
                 Probes_Combobox.Items.Add(SQLDataset.Tables(0).Rows(i).Item(1).ToUpper.ToString.Trim)
             Case "Keyphasor"
                 Keyphasor_Combobox.Items.Add(SQLDataset.Tables(0).Rows(i).Item(1).ToUpper.ToString.Trim)
             Case "Transmitter"
                 Transmitter_Combobox.Items.Add(SQLDataset.Tables(0).Rows(i).Item(1).ToUpper.ToString.Trim)
             Case "Tachometer"
                 Tachometer_Combobox.Items.Add(SQLDataset.Tables(0).Rows(i).Item(1).ToUpper.ToString.Trim.ToUpper.ToString.Trim)
             Case "Dial Therm"
                 DialThermometer_Combobox.Items.Add(SQLDataset.Tables(0).Rows(i).Item(1).ToUpper.ToString.Trim)
             Case "DPS"
                 DPS_Combobox.Items.Add(SQLDataset.Tables(0).Rows(i).Item(1).ToUpper.ToString.Trim)
             Case "Pump Bracket"
                 PumpBracket_Combobox.Items.Add(SQLDataset.Tables(0).Rows(i).Item(1).ToUpper.ToString.Trim)
             Case "Accelerometer"
                 Accelerometer_Combobox.Items.Add(SQLDataset.Tables(0).Rows(i).Item(1).ToUpper.ToString.Trim)
             Case "Velometer"
                 Velometer_Combobox.Items.Add(SQLDataset.Tables(0).Rows(i).Item(1).ToUpper.ToString.Trim)
         End Select
     Catch
         'MessageBox.Show(text:="Error during SetModelNums().", _
         '                caption:="Error", _
         '                buttons:=MessageBoxButtons.OK, _
         '                icon:=MessageBoxIcon.Error)
     End Try
Next

So, for starters, they’re using the ADO .Net DataSet object. This is specifically meant to be a disconnected, in-memory model of the database. The idea is that you might run a set of queries, store the results in a DataSet, and interact with the data entirely in memory after that point. The resulting DataSet will model all the tables and constraints you’ve pulled in (or allow you to define your own in memory).

One of the things that the DataSet tracks is the names of tables. So, the fact that they go and access .Table(0) is a nuisance- they could have used the name of the table. And while that might have been awfully verbose, there’s nothing stopping them from doing DataTable products = SQLDataSet.Tables("Products").

None of this is what caught Nick’s attention, though. You see, the DataTable in the DataSet will do its best to map database fields to .NET types. So it’s the chain of calls at the end of most every field that caught Nick’s eye:

SQLDataset.Tables(0).Rows(i).Item(1).ToUpper.ToString.Trim

ToUpper works because the field in the database is a string field. Also, it returns a string, so there’s no need to ToString it before trimming. Of course, it’s the Tachometer entry that brings this to its natural absurdity:

Tachometer_Combobox.Items.Add(SQLDataset.Tables(0).Rows(i).Item(1).ToUpper.ToString.Trim.ToUpper.ToString.Trim)

All of this is wrapped up in an exception handler, not because of the risk of an error connecting to the database (the DataSet is disconnected after all), but because of the risk of null values, as the comment helpfully states.

We can see that once, this exception handler displayed a message box, but that has since been commented out, presumably because there are a lot of nulls and the number of message boxes the users had to click through were cumbersome. Now, the exception handler doesn’t actually check what kind of exception we get, and just assumes the only thing that could happen was a null value. But that’s not true- someone changed one of the tables to add a column to the front, which meant Item(1) was no longer grabbing the field the code expects, breaking the population of the Pump Bracket combo box. There was no indication that this had happened beyond users asking, “Why are there no pump brackets anymore?”

Source: Read More