VBA - connection to sql database

[Shamas]

Hi all, please tell me if this is the wrong forum for this. I have some code which I sort of understand and would be grateful if someone could help me break it down (explain the sections to me in simple terms pls)

I would also be grateful if you could tell me if the code is 'good' (meaning layout and memory usage etc) Could I improve it or am I re-inventing the wheel.

Thanks in advance

Sub emailsConnection()
   Dim cnn As ADODB.Connection
   Dim MyRecordSet As ADODB.Recordset
   Dim emailSqlString1 As String
   Set cnn = New ADODB.Connection

   dateFromSQL = Sheets("Update").Range("D4").Value
   dateToSQL = Sheets("Update").Range("D4").Value + 7

   ' Set properties of the Connection.
   cnn.ConnectionString = "Provider=xxxxxx;Data Source=xxxxxx;user ID=xxxxxx;password=xxxxxx;"
   cnn.ConnectionTimeout = 90

   ' Open the connection.
   cnn.Open

   ' Find out if the attempt to connect worked.
   If cnn.State = adStateOpen Then
   
   Else
      MsgBox "Sorry. Connection Failed"
   End If
   'Sql String here dynamic
   Dim emailSglString As String
      
   emailSqlString1 = "SELECT TRUNC(creationdate) AS create_date, assignedto, COUNT(*) " & _
   "FROM xxxxxx WHERE channel = 'outbound email' AND lower(status) ='closed' AND " & _
   "creationdate BETWEEN TO_DATE('" & dateFromSQL & "','dd/mm/yyyy') AND TO_DATE('" & _
   dateToSQL & "','dd/mm/yyyy') GROUP BY TRUNC(creationdate), assignedto"
   
   'Sql string temp
   'sqlString = emailSqlString1
    
  'MsgBox (sqlString)

   Set MyRecordSet = New ADODB.Recordset
   MyRecordSet.Open emailSqlString1, cnn, adOpenDynamic, adLockOptimistic
   
   'ClearPrevoiusData
   Sheets("emailSQL").Select
   Range("A3:M65536").Select
   Selection.ClearContents
   
   'Destination
   Sheets("emailSQL").Cells(3, 1).CopyFromRecordset MyRecordSet
   ' Close the connection.
   cnn.Close
    Set cnn = Nothing
    Set MyRecordSet = Nothing

 

[Ruddles]

[COLOR=red]Defines the subroutine [/COLOR]Sub emailsConnection()
[COLOR=red]Declares a variable to hold information about a database connection [/COLOR]Dim cnn As ADODB.Connection
[COLOR=red]Declares a variable to hold information about a set of database records [/COLOR]Dim MyRecordSet As ADODB.Recordset
[COLOR=red]Declares a string variable to hold an SQL query [/COLOR]Dim emailSqlString1 As String
[COLOR=red]Creates a new database connection and stores information about it in cnn [/COLOR]Set cnn = New ADODB.Connection
 
[COLOR=red]Fetches cell D4 from worksheet Update and stores its value in an undeclared (variant type) variable [/COLOR]dateFromSQL = Sheets("Update").Range("D4").Value
[COLOR=#ff0000]Fetches cell D4 from worksheet Update and stores its value in another undeclared (variant type) variable [/COLOR]dateToSQL = Sheets("Update").Range("D4").Value + 7
 
[COLOR=red]' Set properties of the Connection.[/COLOR]
cnn.ConnectionString = "Provider=xxxxxx;Data Source=xxxxxx;user ID=xxxxxx;password=xxxxxx;"
cnn.ConnectionTimeout = 90
 
[COLOR=red]' Open the connection.[/COLOR]
cnn.Open
 
[COLOR=red]' Find out if the attempt to connect worked.[/COLOR]
If cnn.State = adStateOpen Then
 
Else
MsgBox "Sorry. Connection Failed"
End If
 
'Sql String here dynamic
[COLOR=red]Declares another string variable, apparently to hold some SQL, but not used in this program [/COLOR]Dim emailSglString As String
 
[COLOR=red]Creates an SQL string and stores it in the string variable [/COLOR]emailSqlString1 = "SELECT TRUNC(creationdate) AS create_date, assignedto, COUNT(*) " & _
"FROM xxxxxx WHERE channel = 'outbound email' AND lower(status) ='closed' AND " & _
"creationdate BETWEEN TO_DATE('" & dateFromSQL & "','dd/mm/yyyy') AND TO_DATE('" & _
dateToSQL & "','dd/mm/yyyy') GROUP BY TRUNC(creationdate), assignedto"
 
[COLOR=red]Some comments which do nothing[/COLOR]
'Sql string temp
'sqlString = emailSqlString1
'MsgBox (sqlString)
 
[COLOR=red]Creates a pointer to a new recordset [/COLOR]Set MyRecordSet = New ADODB.Recordset
[COLOR=red]Opens the recordset using the SQL set up earlier [/COLOR]MyRecordSet.Open emailSqlString1, cnn, adOpenDynamic, adLockOptimistic
 
[COLOR=red]'ClearPrevoiusData[/COLOR]
Sheets("emailSQL").Select
Range("A3:M65536").Select
Selection.ClearContents
 
'Destination
[COLOR=red]Copy the recordset to the worksheet [/COLOR]Sheets("emailSQL").Cells(3, 1).CopyFromRecordset MyRecordSet
[COLOR=red]' Close the connection[/COLOR].
cnn.Close
 
[COLOR=red]Reset these variables[/COLOR]
Set cnn = Nothing
Set MyRecordSet = Nothing

 

[Shamas]

Thanks makes sense.

I have now declared dateFromSQL and dateToSQL:

Dim dateFromSQL, dateToSQL As String

Is there a best practice guide on how to layout code? and tips and tricks to help the code run more efficiently?

Thanks again

 

[Ruddles]

You're declaring emailSglString and not using it. This is untidy but won't have any effect on the program. Also, common practice is to declare all your variables at the top of the program rather than the middle. (Dim statements aren't executed at run time, they're resolved at compile time, so it doesn't actually matter where they go: this is merely convention.)

dateFromSQL and dateToSQL appear not to be declared, although they could be declared outside this subroutine. If they are undeclared, they will be of type Variant which (a) isn't the most efficient data type and (b) is more tolerant of mismatched data. This is not necessarily good: if you know it's always going to be a number, you should declare it as a number.

The End Sub is missing.

You have a reference to a function called TO_DATE but the coding for this function doesn't appear in your post. Your code will not run without it. Presumably it just contains a Format command and returns a string containing a date.

I would always recommend heading code modules with the Option Explicit directive. This will ensure that undeclared variables are detected at compile time before your module starts to execute and forces you to think more carefully about your data types.

 

[Shamas]

Thank a lot.

I had a good tidy up and now have the following:

Option Explicit

Sub EmailsSQL()

'   Declare variables using Dim
    Dim dateFromSQL, dateToSQL As String
    Dim EmailSQLQuery As String
    Dim Cnct As ADODB.Connection
    Dim MyRecordset As ADODB.Recordset
       
    Set Cnct = New ADODB.Connection

'   Get date range
    dateFromSQL = Sheets("Update").Range("D4").Value
    dateToSQL = Sheets("Update").Range("D4").Value + 7

'   Set properties of the Connection
    Cnct.ConnectionString = "Provider=xxxxxx;Data Source=xxxxxx;user ID=xxxxxx;password=xxxxxx;"
    Cnct.ConnectionTimeout = 90

'   Open the connection
    Cnct.Open

'   Find out if the attempt to connect worked
    If Cnct.State = adStateOpen Then
   
    Else
        MsgBox "Sorry. Connection Failed"
    End If
  
    EmailSQLQuery = "SELECT TRUNC(creationdate) AS create_date, COUNT(*) " & _
    "FROM xxxxxx " & _
    "WHERE channel = 'inbound email' " & _
    "AND status ='open' " & _
    "AND assignedto = 'all' " & _
    "AND creationdate BETWEEN TO_DATE('" & dateFromSQL & "','dd/mm/yyyy') AND TO_DATE('" & dateToSQL & "','dd/mm/yyyy') " & _
    "GROUP BY TRUNC(creationdate), assignedto"
   
'   Set properties of the Recordset
    Set MyRecordset = New ADODB.Recordset
    MyRecordset.Open EmailSQLQuery, Cnct, adOpenDynamic, adLockOptimistic
   
'   Clear prevoius data
    Sheets("emailSQL").Select
    Range("A3:M65536").Select
    Selection.ClearContents
   
'   Destination
    Sheets("emailSQL").Cells(3, 1).CopyFromRecordset MyRecordset
   
'   Close the connection
    Cnct.Close
    
    Set cnn = Nothing
    Set MyRecordset = Nothing

End Sub

 

[Ruddles]

Your code is probably as efficient as it can be. You're not using any loops which contain repetitive code and it's such a small subroutine I can't see any scope for improving it.

If it runs slowly, that's probably because it's taking that long to access the database.

Format your code so that that statements inside code blocks (FOR..NEXT, IF..ELSE..END IF, DO..LOOP, SELECT..END SELECT, WITH..END WITH) is indented: when you get to the end of your Sub or Function and the indenting hasn't ended up back at the left margin, or it gets there before the end of the Sub or Function, you know you've made a mistake.

Here's an example of what I mean:-

[FONT=Fixedsys]Option Explicit[/FONT]
[FONT=Fixedsys][/FONT] 
[FONT=Fixedsys]Public Sub DoSomething()[/FONT]
[FONT=Fixedsys][/FONT] 
[FONT=Fixedsys]  Dim arr(100) As String
  Dim jPtr As Long
  Dim kPtr As Long
  Dim sTemp As String[/FONT]
[FONT=Fixedsys][/FONT] 
[FONT=Fixedsys]  For jPtr = 1 To 100
    arr(jPtr) = Range("A1").Offset(jPtr - 1, 0) 
  Next jPtr[/FONT]
[FONT=Fixedsys][/FONT] 
[FONT=Fixedsys]  For jPtr = 1 To 99
    For kPtr = jPtr + 1 To 100
      If arr(jPtr) > arr(kPtr) Then
        sTemp = arr(jPtr)
        arr(jPtr) = arr(kPtr)
        arr(kPtr) = sTemp
      End If
    Next kPtr
  Next jPtr[/FONT]
[FONT=Fixedsys][/FONT] 
[FONT=Fixedsys]  For jPtr = 1 To 100
    Range("B1").Offset(jPtr - 1, 0) = arr(jPtr)
  Next jPtr[/FONT]
[FONT=Fixedsys][/FONT] 
[FONT=Fixedsys]  MsgBox "Done!"[/FONT]
[FONT=Fixedsys][/FONT] 
[FONT=Fixedsys]End Sub[/FONT]

I tend to indent my code two spaces for each block I start and then 'outdent' (?) it back two spaces when the block ends. The standard appears to be four but as long as you do it, anything around those values will be fine. Many more than that and your code will start disappearing off the right edge of your screen.

In addition to indenting, the occasional blank line helps to separate the
component parts of the module. Leave a blank line between Subs and Functions, and before and after your Dim statements. If the code has some 'set-up' statements - like initialising variables, creating objects, etc - insert a blank line before and after these, and if there's a 'wrap-up' section - like displaying a "Finished!" message, insert a blank line before and after this also. It's usually obvious where the blank lines should go to improve readability.

Check some of the other code posts in this forum (particularly, of course, mine!) and copy what looks neatest to you.

Lengthy lines should be broken up with the underscore continuation character so that you can read the code without any unnecessary left-right scrolling. People hate that!

If you have repetitive blocks of code - bits of code which appear to be doing the same thing but with different inputs, consider making these separate procedures. This will shorten your code, make it easier to read, and also easier to maintain since the code only appears in one place instead of several places.

The evaluation of complex conditions can be relegated to private functions which return a single value, again reducing the size of your code, aiding readability and making modification easier if at some point in the future the condition changes - you'll only have to change the code in a single place instead of having to check all of your code.

Finally, any single, large operations which can stand alone, should. For example, if you're opening an Access database and extracting some information, the code for this could run to a couple of dozen lines: do it in a separate module and Call the module - as before, the code then won't contributing to the size limit, you won't have to press PgDn so often when viewing your main program code, the 'Access related' code will all be in one place.

 

[RoryA]

Also, please learn to use code tags when posting code on the forum - it's a lot easier to read.

 

[Shamas]

Thanks both.

Learned a lot today.

Will also start using code tags