Working on Code efficiency...

[craigwg]

This should be an interesting discussion. I have a few procedures written that work great, but there is a lot of
repetitive code amongst them. I want to consolidate them and put the repetitive code in one procedure and
then just call that procedure as needed. Pretty standard stuff, right?

The problem is that variables are created and called through out the procedures and if I start splitting up the
code it stops working. I turn to you guys (and gals) for help! How can I make my code less repetitve? I have
posted one block of code below. The code connects to an SQL server and then inserts data on the XLS
worksheet to the SQL database. I have another block that deletes, updates, and selects. I only posted the
insert block below.

[vba]
Public Sub Insert()
'This is the connection code that I want stuffed in a seperate proc.
'Before running you must go to
'Tools--> References --> and add Micorsoft ActiveX Data Objects 2.8 Library In the VB editor
'define a DSN-less connections string:
Const stADO As String = "Provider=SQLOLEDB.1;Integrated Security=SSPI;" & "Persist Security Info=True;" & _
"Initial Catalog=books;" & "Data Source=ISTSLCD3" 'Initial Catalog is db, datasource is servername
'Define and set the worksheet
Dim wbBook As Workbook
Dim wsSheet As Worksheet
Set wbBook = ActiveWorkbook
Set wsSheet = wbBook.Worksheets("Sheet1")
'Open a connection to the database. It's important that the connection CursorLocation
'property be adUseServer.
Dim con As ADODB.Connection
Set con = New ADODB.Connection
With con
.CursorLocation = adUseServer
.Open stADO
.CommandTimeout = 0
End With
'This is the end of the connection code. The connection is closed later. Below is the actual code that does
'the inserting.
'Notice that strings are quoted in '""' and numbers are quoted with only ""
Dim strSQL As String
Dim irow As Integer
For irow = Selection(1).row To (Selection(1).row + Selection.Rows.Count - 1)
If WorksheetFunction.CountA(Selection) <= 7 Then 'this verifies that minimum number of columns is selected
MsgBox "Please select the rows you want to insert into SQL.", vbOKOnly, "SQL Insert"
Exit Sub
End If
strSQL = "Insert into dbo.authors (au_id, au_fname, au_lname, phone, address, city, state, zip) values _
('" & wsSheet.Cells(irow, 1).Value & "','" & wsSheet.Cells(irow, 2).Value & "', '" & wsSheet.Cells(irow, 3).Value _
& "', '" & wsSheet.Cells(irow, 4).Value & "', '" & wsSheet.Cells(irow, 5).Value & "', '" & wsSheet.Cells(irow, 6).Value _
& "', '" & wsSheet.Cells(irow, 7).Value & "', " & wsSheet.Cells(irow, 8).Value & ")"
'execute the insert statement
con.Execute strSQL
Next irow
''''''''''''''''''''''''''This closes the connection and could be in a seperate procedure as well.
'Clean up the connection:
con.Close
Set con = Nothing
MsgBox "The selected row(s) have been added succesfully to the SQL database.", vbOKOnly, "SQL Success"
End Sub
[/vba]

So what do ya'll think?

[mdmackillop]

I wouldn't say your code is repetitive, but to split of the Insertion, try this.
BTW, You should really check that the database has additional records before you assert that the code is successful.

[vba]
Option Explicit
Dim con As ADODB.Connection
Public Sub Insert()

'This is the connection code that I want stuffed in a seperate proc.
'Before running you must go to
'Tools--> References --> and add Micorsoft ActiveX Data Objects 2.8 Library In the VB editor
'define a DSN-less connections string:
Const stADO As String = "Provider=SQLOLEDB.1;Integrated Security=SSPI;" & "Persist Security Info=True;" & _
"Initial Catalog=books;" & "Data Source=ISTSLCD3" 'Initial Catalog is db, datasource is servername

'Open a connection to the database. It's important that the connection CursorLocation
'property be adUseServer.
Set con = New ADODB.Connection
With con
.CursorLocation = adUseServer
.Open stADO
.CommandTimeout = 0
End With

DoInsert

con.Close
Set con = Nothing
MsgBox "The selected row(s) have been added succesfully to the SQL database.", vbOKOnly, "SQL Success"
End Sub

Sub DoInsert()
Dim strSQL As String
Dim irow As Integer
Dim wbBook As Workbook
Dim wsSheet As Worksheet

Set wbBook = ActiveWorkbook
Set wsSheet = wbBook.Worksheets("Sheet1")
For irow = Selection(1).Row To (Selection(1).Row + Selection.Rows.Count - 1)
If WorksheetFunction.CountA(Selection) <= 7 Then 'this verifies that minimum number of columns is selected
MsgBox "Please select the rows you want to insert into SQL.", vbOKOnly, "SQL Insert"
Exit Sub
End If
strSQL = "Insert into dbo.authors (au_id, au_fname, au_lname, phone, address, city, state, zip) values _
('" & wsSheet.Cells(irow, 1).Value & "','" & wsSheet.Cells(irow, 2).Value & "', '" & wsSheet.Cells(irow, 3).Value _
& "', '" & wsSheet.Cells(irow, 4).Value & "', '" & wsSheet.Cells(irow, 5).Value & "', '" & wsSheet.Cells(irow, 6).Value _
& "', '" & wsSheet.Cells(irow, 7).Value & "', " & wsSheet.Cells(irow, 8).Value & ")"
con.Execute strSQL
Next irow
End Sub

[/vba]

[p45cal]

Maybe try this..(it might go against so-called programming rules)

I created a sub called EstablishConnection which needs two arguments. This sub is called with a connection object and
a worksheet object, both initially Nothing. On return from the sub, these are now set to something.

I've tested this as far as I can and it seems to work.

A couple of notes:
I changed the location of the If..Then..Else statement so that your Exit Sub doesn't skip the setting of con to nothing and the closing of con.
I've made a suggested change to the line verifying that at least 8 columns are selected as stated in your comment; Worksheet Function CountA counts the number of cells that are not empty in a range. If this range were a vertical selection of 8 cells in a single column it would still pass your test.
I shortened the line beginning 'strSQL = "Insert into' by putting it into a With..End With so that you don't need to refer to wsSheet multiple times.
I removed all references to wbBook by setting wsSheet in one line.
[vba]Public Sub Insert()
'If WorksheetFunction.CountA(Selection) <= 7 Then 'this verifies that minimum number of columns is selected
If Selection.Columns.Count <= 7 Then 'this verifies that minimum number of columns is selected
MsgBox "Please select the rows you want to insert into SQL.", vbOKOnly, "SQL Insert"
Else
Dim con As ADODB.Connection
Dim wsSheet As Worksheet
EstablishConnection con, wsSheet

'This is the end of the connection code. The connection is closed later. Below is the actual code that does the inserting.
'Notice that strings are quoted in '""' and numbers are quoted with only ""
Dim strSQL As String
Dim irow As Integer
For irow = Selection(1).Row To (Selection(1).Row + Selection.Rows.Count - 1)
' Exit Sub 'note this skips the cleaning up operation of con, so I've changed the location of the if-then-else.
With wsSheet
strSQL = "Insert into dbo.authors (au_id, au_fname, au_lname, phone, address, city, state, zip) _
values ('" & .Cells(irow, 1).Value & "','" & .Cells(irow, 2).Value & "', '" & .Cells(irow, 3).Value & "', '" & _
.Cells(irow, 4).Value & "', '" & .Cells(irow, 5).Value & "', '" & .Cells(irow, 6).Value & "', '" & .Cells(irow, 7).Value _
& "', " & .Cells(irow, 8).Value & ")"
End With
'execute the insert statement
con.Execute strSQL
Next irow

'This closes the connection and could be in a seperate procedure as well.
'Clean up the connection:
con.Close
Set con = Nothing
MsgBox "The selected row(s) have been added succesfully to the SQL database.", vbOKOnly, "SQL Success"
End If
End Sub
Sub EstablishConnection(con, wsSheet)
'''''''''This is the connection code that I want stuffed in a seperate proc.
'Before running you must go to
'Tools--> References --> and add Micorsoft ActiveX Data Objects 2.8 Library In the VB editor
'define a DSN-less connections string:
Const stADO As String = "Provider=SQLOLEDB.1;Integrated Security=SSPI;" & "Persist Security Info=True;" & _
"Initial Catalog=books;" & "Data Source=ISTSLCD3" 'Initial Catalog is db, datasource is servername
'Define and set the worksheet
Set wsSheet = ActiveWorkbook.Worksheets("Sheet1")
'Open a connection to the database. It's important that the connection CursorLocation
'property be adUseServer.
Set con = New ADODB.Connection
With con
.CursorLocation = adUseServer
.Open stADO
.CommandTimeout = 0
End With
End Sub
[/vba]
One more thing, straight after calling EstablishConnection, you could check that both arguments have been set to
something with the likes of:
[vba] EstablishConnection con, wsSheet
If con Is Nothing Or wsSheet Is Nothing Then
'error MsgBox
Else
'strut your stuff
End If
[/vba]