Get data from Access SQL database with ADODB and weather data using web service

I have a program, that works, I just feel that it is running slower than it should and I feel that it is a bit more unstable than it should be. I am looking for tips on writing "better" code and making my program more stable.

I am looking to better this part of my code for now:

`Private Sub Worksheet_Activate()
Application.ScreenUpdating = False

'Removes shapes already there that will be updated by the getWeather function
For Each delShape In Shapes
If delShape.Type = msoAutoShape Then delShape.Delete
Next delShape

'Calls a function to get weather data from a web service
Call getWeather("", "Area1")
Call getWeather("", "Area2")
Call getWeather("", "Area3")

'Starting to implement the first connection to a SQL Access database.
Dim cn As Object
Dim rs As Object

'Set cn and sqlConnect as ADODB-objects. Set rs as recordset
Set cn = CreateObject("ADODB.Connection")
Set sqlConnect = New ADODB.Connection
Set rs = CreateObject("ADODB.RecordSet")

'Set sqlConnect as connection string
sqlConnect.ConnectionString = "Provider=Microsoft.ACE.OLEDB.12.0;Data Source=C:\databases\database.accdb;Persist Security Info=False;"

'Open connection string via connection object
cn.Open sqlConnect

'Set rs.Activeconnection to cn
rs.ActiveConnection = cn

'Get a username from the application to be used further down
Brukernavn = Application.userName

'This part of the code re-arranges the date format from american to european
StartDate = Date
EndDate = Date - 7

midStartDate = Split(StartDate, ".")
midEndDate = Split(EndDate, ".")

StartDate2 = "" & midStartDate(1) & "/" & midStartDate(0) & "/" & midStartDate(2) & ""
EndDate2 = "" & midEndDate(1) & "/" & midEndDate(0) & "/" & midEndDate(2) & ""

'SQL statement to get data from the access database
rs.Open "SELECT [RefNr], [Registrert Av],[Nettstasjon], [Meldt Dato] , [Bestilling], [Sekundærstasjon], [Avgang], [Hovedkomponent], [HovedÅrsak], [Status Bestilli

Worksheet_Activate is doing waaaay too many things. It's an entry point, so the abstraction level should be fairly high. Something like this:

Private Sub Worksheet_Activate()
RemoveExistingWeatherShapes
UpdateWeatherData
UpdateFoobarData 'whatever the Access queries do
End Sub


There's a lot to cover, so I'll just grab the low-hanging fruit here:

• Indentation isn't always consistent.

• Procedure names should be PascalCase

• Call keyword is not needed to make a procedure call; it's obsolete/deprecated.

-
This chunk is locale-dependent; it involves implicit string conversions and will fail to run on a machine that is configured to use a different date format:

StartDate = Date
EndDate = Date - 7

midStartDate = Split(StartDate, ".")
midEndDate = Split(EndDate, ".")

StartDate2 = "" & midStartDate(1) & "/" & midStartDate(0) & "/" & midStartDate(2) & ""
EndDate2 = "" & midEndDate(1) & "/" & midEndDate(0) & "/" & midEndDate(2) & ""


It's not clear why the end date would be a week before the start date: a comment is required here, to explain that. Otherwise, it looks like a bug (or is it one?).

It's not clear where or whether the variables are declared at all. If they're declared, their scope needs to be reduced and their declaration belongs inside the procedure they're used in. If they're not declared, declare them. All. And put Option Explicit at the top of every single module, so that VBA refuses to compile code that doesn't declare its variables. Without it, you're asking for trouble, since VBA will happily compile and run code with typos.

Declared or not, StartDate and EndDate are both assigned a Date value, so at that point they're Date variables (or Variant/Date if undeclared). This means everything else is treating dates as strings, and that's very frail and bug-prone. Use the Year, Month and Day functions to retrieve the year, month and day parts of a Date value, respectively; that Split thing is not going to work on a workstation that uses / to separate date parts.

Watch the naming, too: midStartDate means nothing. StartDate2 is unclear. Consider startDateParts and formattedStartDate, respectively (although, as noted above, midStartDate and midEndDate should probably be removed anyway).

-
Comments should say why, not what. Consider extracting "chunks of code" under a "this chunk does XYZ" comment, into their own procedures.

-
You're referencing the ADODB type library, so you don't need late-binding.

Dim cn As Object
Set cn = CreateObject("ADODB.Connection")

Set sqlConnect = New ADODB.Connection

Dim rs As Object
Set rs = CreateObject("ADODB.RecordSet")


Instead of declaring your cn As Object, declare cn As ADODB.Connection, and then do the same for rs As Object, which should be rs As ADODB.Recordset. You'll get IntelliSense/auto-complete for all member calls, and you'll reduce runtime overhead. Don't use CreateObject when you can New things up directly.

It's not clear why you need two ADODB.Connection objects. You use one and assign its connection string, and then never open it; instead you do this:

cn.Open sqlConnect


That's implicitly doing this:

cn.Open sqlConnect.ConnectionString


Might as well just do:

sqlConnect.Open


And work off sqlConnect then: both cn and sqlConnect are the same type, and have the same connection string: one of them is superfluous.

You have this pattern thrice in your code, using 3 connections to the same database. You could remove the 2nd and 3rd connections, and reuse the connection for the other 2 queries, reducing connection overhead.

-
This is redundant:

If Not rs.EOF Then
rs.MoveFirst
End If


-
This EOF check is redundant...

Do
If Not rs.EOF Then


...but only because you've made it a Do loop, which ensures at least 1 iteration. Flip it around, put the condition at the top:

Do While Not rs.EOF
'loop body
Loop


Doing that removes a whole indentation level, a GoTo jump and a line label.

Your query uses Application.UserName, but Application.UserName can be written to by anyone and can contain anything: as far as your code is concerned, it should be considered user input, and treated as such.

Consider what would happen if a user executed this:

Application.UserName = "Bob'; DROP TABLE tblDatabase --"


And then ran your macro.

rs.Open "SELECT [RefNr], [Registrert Av],[Nettstasjon], [Meldt Dato] , [Bestilling], [Sekundærstasjon], [Avgang], [Hovedkomponent], [HovedÅrsak], [Status Bestilling] FROM [tblDatabase]" & _
"WHERE [Registrert Av] = '" & Brukernavn & "' AND [Loggtype] <> 'Beskjed' AND [Meldt Dato] BETWEEN #" & StartDate2 & "# AND # " & EndDate2 & "#" & _
"ORDER BY [Meldt Dato] DESC;", _
cn, adOpenStatic


When the above instruction hits the database, it looks something like this:

` SELECT [RefNr], [Registrert Av],[Nettstasjon], [Meldt Dato] , [Bestilling], [Sekundærstasjon], [Avgang]

StackExchange Code Review Q#160682, answer score: 4