code critique - am I creating a Rube Goldberg machine?

Question

I'm making a fair amount of calls to database tables via ADO. In the spirit of keeping things DRY, I wrote the following functions to return an array of values from a recordset. Is this hare brained? I use it mainly for grabbing a set of combo-box values and the like, never for enormous values. Example Usage (error handling removed for brevity):

Function getEmployeeList()
    getEmployeeList= Array()
    strSQL =  "SELECT emp_id, emp_name from employees"
    getEmployeeList = getSQLArray( strSQL, "|" )
End Function

Then I just do whatever I want with the returned array.

Function getSQLArray( SQL, delimiter )
'*************************************************************************************
' Input a SQL statement and an optional delimiter, and this function
' will return an array of strings delimited by whatever (pipe defaults)
' You can perform a Split to extract the appropriate values. 
' Additionally, this function will return error messages as well; check for 
' a return of error & delimiter & errNum & delimiter & errDescription
'*************************************************************************************
    getSQLArray = Array()
    Err.Number = 0
    Set objCon = Server.CreateObject("ADODB.Connection") 


    objCon.Open oracleDSN


    Set objRS = objCon.Execute(SQL)

    if objRS.BOF = false and objRS.EOF = false then
     Do While Not objRS.EOF
      for fieldIndex=0 to (objRS.Fields.Count - 1)
        If ( fieldIndex <> 0 ) Then
         fieldValue = testEmpty(objRS.Fields.Item(fieldIndex))
         recordString = recordString & delimiter & fieldValue
        Else 
         recordString = CStr(objRS.Fields.Item(fieldIndex))
        End If
      Next 
      Call myPush( recordString, getSQLArray )
      objRS.MoveNext
     Loop
    End If
    Set objRS = Nothing
  objCon.Close
  Set objCon = Nothing
End Function

Sub myPush(newElement, inputArray)
    Dim i
    i = UBound(inputArray) + 1
    ReDim Preserve inputArray(i)
    inputArray(i) = newElement                                          
End Sub


Function testEmpty( inputValue )
    If (trim( inputValue ) = "") OR (IsNull( inputValue )) Then 
      testEmpty = ""
    Else
     testEmpty = inputValue 
    End If
End Function

The questions I'd have are: Does it make sense to abstract all the recordset object creation/opening/error handling into its own function call like this? Am I building a Rube Goldberg machine, where anyone maintaining this code will curse my name?

Should I just suck it up and write some macros to spit out the ADO connection code, rather than try doing it in a function?

I'm very new to asp so I have holes in its capabilities/best practices, so any input would be appreciated.

Answer 1

Yes, it makes sense to factor out common tasks. I don't see anything wrong with the general idea. I'm wondering why you're returning an array of strings separated by a delimiter; you might as well return an array of arrays.

Answer 2

There's nothing wrong with doing it your way. The ADO libraries were not really all that well designed, and using them directly takes too many lines of code, so I always have a few utility functions that make it easier to do common stuff. For example, it's very useful to make yourself an "ExecuteScalar" function that runs SQL that happens to return exactly one value, for all those SELECT COUNT(*)'s that you might do.

BUT - your myPush function is extremely inefficient. ReDim Preserve takes a LONG time because it has to reallocate memory and copy everything. This results in O(n²) performance, or what I call a Shlemiel the Painter algorithm. The recommended best practice would be to start by dimming, say, an array with room for 16 values, and double it in size whenever you fill it up. That way you won't have to call ReDim Preserve more than Lg₂n times.

Answer 3

I wonder why you are not using GetRows? It returns an array, you will find more details here: http://www.w3schools.com/ado/met_rs_getrows.asp

A few notes on GetRows:

Set objRS = Server.CreateObject ("ADODB.Recordset")
objRS.Open cmd, , adOpenForwardOnly, adLockReadOnly

If Not objRS.EOF Then
   astrEmployees = objRS.GetRows()
   intRecFirst   = LBound(astrEmployees, 2)
   intRecLast    = UBound(astrEmployees, 2)

   FirstField  = 0
   SecondField = 1
End If

'2nd field of the fourth row (record) '
Response.Write (SecondField, 3)