VBA to MS-Access SQL query set(s) and outputs

I am hoping I can get some guidance on some better "best practice" suggestions on how to handle this kind of code set. I understand that parameterized queries are a thing, but I am not quite there yet, but feel free to drop link to good tutorial on that if you must include that as part of your suggestion.

Here is my code that works well, but I am trying to optimize performance. I thought about dumping RecordSet into an array but i am not sure how helpful that would be? I am pretty open to anything here as I would love to develop best practices in my learning structure.

I know this breaks a cardinal rule about not interacting with the worksheet directly unless needed, but I don't know a way around it with this particular scenario.

```
Private Sub CommandButton1_Click()
Dim fso As FileSystemObject: Dim con As New ADODB.Connection
Dim rs As New ADODB.Recordset: Dim strConnection As String
Dim i As Integer, fld As Object: Dim TotalRec As Long
Dim RecordNum As Long: Dim filelocation1 As String
Dim wBo As Workbook: Dim wsO As Worksheet
Dim answer As Integer: Dim myValue As Variant
Dim count As Long: Dim src As CodeModule
Dim dest As CodeModule: Dim QUV As Long
Dim IID As Long: Dim rCell As Range: Dim rRng As Range
Application.ScreenUpdating = False
Application.DisplayAlerts = False
Application.Calculation = xlCalculationManual
count = 1
myValue = InputBox("What was the last order number?")
RecordNum = myValue
Set wBo = ActiveWorkbook
With wBo
Set wsO = wBo.Sheets("Sheet1")
Sheets(1).Activate
strConnection = "Provider=Microsoft.ACE.OLEDB.12.0;Data Source=C:PathtoFile" & "\" & ".accdb"
con.Open strConnection
rs.Open "SELECT MAX(ORDERNO) AS MaxAmtOrders FROM dbo_ITEMS", con
Sheets(1).Range("Z1").CopyFromRecordset rs
rs.Close
con.Close
TotalRec = wBo.Sheets("Sheet1").Range("Z1").Value
For y = myValue To TotalRec

Variables

• You have a bunch of variables that are declared but never used: fso, filelocation1, wsO, answer, src, dest. These add significantly to the clutter at the top of the procedures, which leads to...

• Try to declare variables close to where you are using them in code. Not only does this break up the monolithic block of declarations at the top of the procedure to make it more readable, it helps in determining what they are used for.

• If you insist on using multiple declarations on the same line, the statement concatenation operator : is superfluous and makes it even more difficult to read (i.e. Dim wBo As Workbook: Dim wsO As Worksheet). VBA already allows comma delimited declarations (like you do here: Dim i As Integer, fld As Object). There isn't a reason to combine them on the same line, and the : operator should be avoided in general because in the vast majority of cultures, text is read from top down.

• Put Option Explicit at the top of the module and make sure that all of your variables are declared. The variable y is never declared.

• Try to use more meaningful variable names in order to help make your code self-documenting. For example, taken out of context myValue could be mean literally anything.

• You're using a hodgepodge of variable naming conventions: Hungarian (strConnection, rRng, etc.), Pascal case (TotalRec, RecordNum), camel case (myValue), and whatever wBo and wsO are. Best would be to follow current convention, but whatever you choose, make it consistent.

Validate Input

RecordNum is declared as a Long, but myValue = InputBox("What was the last order number?") returns a Variant of type String. You then immediately assign it with RecordNum = myValue, which implicitly casts it to a number. If the user cancels the InputBox or types anything other than a number, this will throw a run-time error.

Add Error Handling

This is especially important when you are using external functionality like a database connection. Consider what you would want to happen when a call like con.Open strConnection fails or times out. Currently the wheels would come off, opening the possibility that your database connection never gets closed.

Syntax Consistency

You are using at least 3 different methods of obtaining Worksheet references: Named - Sheets("Sheet1"), by ordinal - Sheets(1), and by object name Sheet5. Not only that, you are repeatedly resolving the same references. Just store them in variables and use the variable so Excel doesn't have to locate them in the Worksheets collection again and again.

Unused With Block

You never make use of the With block here:

With wBo

    '....
End With

Every single reference to wBo inside the block is wBo.Whatever. The point of working inside of a With block is that the reference only needs to be resolved once. If you only use hard references inside of it, it doesn't do anything other than add an additional level of indentation.

Dead and/or Meaningless Code

You are repeatedly calling .Activate on Worksheets, but I can't find anywhere in the code where this actually matters other than the one call to ActiveCell, which should probably be removed. Activating and selecting in Excel is expensive - it is best avoided entirely.

There is no need to use ActiveWorkbook on the line ActiveWorkbook.Sheets("Sheet2").Range("a1").Select (and less reason to select cell A1). Not only do you already have a reference to wBo, you're inside a With block for that reference and there isn't anything in your code that would change the active Workbook.

The "variable" strConnection is never set to anything other than:

"Provider=Microsoft.ACE.OLEDB.12.0;Data Source=C:PathtoFile" & "\" & ".accdb"

You make the exact same assignment 4 times. It should be converted to a constant.

I'm not sure whether this is intentional or not, but I'm not coming up with a good reason to size the columns in a Worksheet...

Sheets(2).Columns("a").ColumnWidth = 6.57

...and then clear the contents:

Sheets(2).Clear

Speaking of resizing, Sheet(5) is repeatedly resized and formatted to the same values inside of a loop. You also repeatedly write the same values to the column headers inside of a loop wBo.Sheets("Sheet1").Cells(Sheet1.Rows.count, 1).End(xlUp).offset(2, 0) = "Item#"

ADO Issues

You repeatedly open and close the same ADO connection in a nested loop. This is incredibly expensive and completely unnecessary. Open the connection when you start, and close it when you finish.

You've already noted that the queries aren't parametrized, so I won't belabour that point other than to mention that this isn't really safe. Check out the VBA documentation page over on SO for an example. On huge benefit this offers for readability is that you can move your queries out of the procedure itself and make them constants. That way your code isn't cluttered with sections lik

StackExchange Code Review Q#135802, answer score: 2