Code Layout (A2K)

Topic

New Reply

Good Monday Morning Everyone!

I have a table that is imported in. Each field that imports is text. 11 of the 31 fields contains a character on the right that is hex. The hex character needs to be converted to it’s respective equivalent value then these 11 fields needs to be converted from TEXT to CURRENCY and place the converted values in a new column.

I’m attaching code that works fine. I know it’s poorly written and was hoping someone could provide suggestions on how I can modify this code to work more effeciently.

Again, the code works, but should be and can be written better.

Thanks,

Reply | Quote

Replies

Good Afternoon!

A more compact (and hence also less readable) version:

Private Sub cmdOutput_Click()
Dim cn As ADODB.Connection
Dim rs As New ADODB.Recordset
Dim SQL_STR As String
Const C = ","
Const Q = "'"
Dim i As Integer

Set cn = CurrentProject.Connection
rs.CursorLocation = adUseClient ' where the processing begins
rs.Open "tPDE", cn, adOpenDynamic, adLockOptimistic, adCmdTableDirect

With rs
.MoveFirst
Do Until .EOF
' First fields to include
SQL_STR = C & C & Q & .Fields(2).Value & Q
' Concatenate the pairs
For i = 4 To 24 Step 2
SQL_STR = SQL_STR & C & Q & .Fields(i - 1).Value & Q
SQL_STR = SQL_STR & C & FilterNumber(.Fields(i - 1).Value)
Next i
' And the last single ones
For i = 26 To 31
SQL_STR = SQL_STR & C & Q & .Fields(i - 1).Value & Q
Next i
' Assemble SQL command
SQL_STR = "INSERT INTO tbl_Output VALUES (" & SQL_STR & ")"
' Execute it
cn.Execute SQL_STR, , adCmdText
.MoveNext
Loop
.Close
End With
Set rs = Nothing
Set cn = Nothing
End Sub

The downside of such code is that it is more work to modify it if the table structure changes.

Reply | Quote

BTW is it correct that .Fields(4), .Fields(6), …, .Fields(24) aren’t used?

Reply | Quote

Thanks Hans,

By your response, I assume it’s probably best to leave it the way it is? Is that correct?

Per your question about fields .4, .6 and .24. That’s correct.

Reply | Quote

There are a few points from my version that you could incorporate while leaving the general structure as you had it:

– In addition to your C = ",", I used Q = "'", and I defined both as constants.
– Instead of the two lines

Dim rs As ADODB.Recordset
Set rs As New ADODB.Recordset

I used the equivalent single line

Dim rs As New ADODB.Recordset

– Instead of opening a recordset on “SELECT * FROM …”, I opened the table directly in

rs.Open "tPDE", cn, adOpenDynamic, adLockOptimistic, adCmdTableDirect

– Instead of using a second recordset, I used the Execute method of the connection object (you didn’t really use the recordset IIS anyway)

cn.Execute SQL_STR, , adCmdText

Reply | Quote