HiveBrain v1.2.0
Get Started
← Back to all entries
principlesqlMinor

MS Access VBA code to compare records in a table and combine data where necessary

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
necessarycombinetablerecordsandwherevbacodecomparedata

Problem

I need to process some data which is the output from a CAD system, namely a Bill of Materials.

I've constructed database and written some VBA code to achieve this. An explanation of what this code does is included as comments at the top of the code block below.

This is my first project in Access/VBA and I have expanded upon something which a colleague wrote, so I expect it's pretty awful, but it does work for what I want it to do.

I've just posted one module which does the first step described in the comments block at the start of the code below.

Note I have written "Suspect there is a better way to do this" etc. in the comments of a line which I think is questionable, so if you do a Ctrl+F for "suspect" then you'll find things I am particularly unsure of.

What I'd like to get from this review is:

-
Tighten this all up functionally, perhaps speed the code up where possible. I suspect that maybe some of my loops, ways of moving through recordsets etc. may be inefficient.

-
I think perhaps I could be making better use of SQL queries - currently the code doesn't use them but I suspect that using saved queries etc. in some places (executed through VBA) might be quicker than using recordsets in VBA - eg. the parts where records are compared to one another.

-
Find out where I have done things which are considered bad practice.

I'm less concerned about things like Hungarian notation and making the code pretty.

`Sub condenseOutputDocs()

'If gcfHandleErrors Then On Error GoTo Err_General

Dim strCOMP_NAME As String
Dim strDfltParam As String
Dim strParamsExpOld As String
Dim strParamsExpNew As String
Dim intParamPos As Integer
Dim strREF1 As String
Dim intNB1 As Integer
Dim strCOMP_NAME1 As String
Dim strPAR1 As String
Dim strREF2 As String
Dim strREFnew As String
Dim intNB2 As Integer
Dim strCOMP_NAME2 As String
Dim strPAR2 As String
Dim strCOMMENT1 As String
Dim strCOMMENT2 As String
Dim strCOMMENTnew As String
Dim lngCmntFndPos As Long
Dim intSlashCount As

Solution

First and foremost, use Option Explicit in all of your code modules. It forces you to declare all of your variables. You have about 20 declarations at the top of your module, but haven't declared your recordsets at all.

These are never used I didn't check the rest of them:

Dim strCOMP_NAME As String
Dim strCOMP_NAME1 As String
Dim strCOMP_NAME2 As String


The hungarian notation isn't necessary either. Things like lngCurrDocTblRecordCount aren't necesary in the modern IDE. I'm sure you read somewhere that it's best practice, but it's just clutter. I do like that I know exactly what that variable is though. It's a little long, but its meaning is clear.

I'll reiterate what @Malachi said about the Do While Not loops. Do Until is easier to understand.

On the other hand, this If Right(strCOMMENT2, 1) <> "~" Then is probably more understandable as

If Not Right(strCOMMENT2, 1) = "~" Then


Speaking of strCOMMENT, you have the exact same logic for both 1 & 2. That's a dead give away that you need a function. This one will take a string parameter and return another string.

Private Function markCommentIfEmpty(str as String) As String
    If str = vbNullString Then
        markCommentIfEmpty = "¿"
    Else 
        markCommentIfEmpty = str
    End If
End 

'called like this
strCOMMENT1 = markComment(strCOMMENT1)


Same goes for this logic, but it will take three parameters instead. I'll leave the actual building of that function as an exercise for you. Look for other places where you're repeating the same logic and create subs and functions for those as well.

If Right(strCOMMENT1, 1) <> "~" Then ' if comment does not already contain some comment (from a previous run through) then create new concatenated string
    strCOMMENT1 = strREF1 & "~" & intNB1 & "~" & strCOMMENT1 & "~" ' e.g. "A~3~Cable groove~"
End If


That's a lot to take in, so I'll stop for now. I really encourage you to come back with a follow up question after addressing these things. I didn't get to address your actual questions, but I feel it's important to address these issues first before tackling any performance related questions. Code review can (should?) be an iterative process.

Code Snippets

Dim strCOMP_NAME As String
Dim strCOMP_NAME1 As String
Dim strCOMP_NAME2 As String
If Not Right(strCOMMENT2, 1) = "~" Then
Private Function markCommentIfEmpty(str as String) As String
    If str = vbNullString Then
        markCommentIfEmpty = "¿"
    Else 
        markCommentIfEmpty = str
    End If
End 

'called like this
strCOMMENT1 = markComment(strCOMMENT1)
If Right(strCOMMENT1, 1) <> "~" Then ' if comment does not already contain some comment (from a previous run through) then create new concatenated string
    strCOMMENT1 = strREF1 & "~" & intNB1 & "~" & strCOMMENT1 & "~" ' e.g. "A~3~Cable groove~"
End If

Context

StackExchange Code Review Q#52511, answer score: 5

Revisions (0)

No revisions yet.