Any Improvements to be made? (1 Viewer)

[cheekybuddha]

I thought opening a recordset that huge may have some impact on performance, so I chose the insert method.
Do you think it's better to change it to opening a recordset and use AddNew?

The trick is to open an empty recordset for this:

With CurrentDb.OpenRecordset("SELECT * FROM YourTable WHERE 1=0;")
  For i = 1 To NumRecsToAdd
    .AddNew
      .Fields("x") = "x"
      .Fields("y") = "y"
      .Fields("z") = "z"
    .Update
  Next i
  .Close
End With

(I think you can also use dbAppendOnly - did not read all the answers before replying )

 

[CJ_London]

In addition to other responses, I would include comments in the code explaining what the code is intended to do. Otherwise when you revisit this code in a years time, you will spend time working out what it is actually doing and why. Document as you go.

'objective/purpose of code: to insert records in the xxxx table from the yyyy table that were created in last 7 days from today
'open the yyyy table as a recordset with criteria to limit to the last 7 days
'verify there are records
'loop through the yyyy recordset to verify the customer exists and if so insert record to xxxx table
'check customer exists
'if exists
'create sql
'execute sql
'loop to to next record

When I'm writing code in earnest I start with a script such as above for what I want to achieve, then I fill in with code. A bit like when writing an if, case, loop whatever I always include the terminating code (end if/end select/loop etc) before filling in the working code. Obviously modify the script if you encounter issues not initially considered such as finding you have to handle missing data

 

[Josef P.]

In addition to other responses, I would include comments in the code explaining what the code is intended to do. Otherwise when you revisit this code in a years time, you will spend time working out what it is actually doing and why. Document as you go.

I don't want to write against comments at all at this point, but give an example where the code itself can be the comment.
The danger with comments is: The code is revised, but the comment is not adapted. This adjustment must be made with comments absolutely, so that in the code happens what is in the comment.

                'Update the wire reel to reflect the cut  ' <--- here is the code comment
                Set RS2 = CurrentDb.OpenRecordset("SELECT * FROM tblWireRoom WHERE [ReelID] = " & vReelID & "")
                With RS2
                    'make sure we are working on the correct reel
                    If ![ReelID] = vReelID Then
                        RS2.Edit
                        vCurrentLength = ![CurrentLength]
                        RS2!CurrentLength = vCurrentLength - TotalLength
                        RS2.Update
                    End If
                End With

vs:

                CutWireReel DB, vTicketID, !WireReelID, TotalCutLength

... Name of the procedure may be better named, of course, if there is a better content labeling.

[OT]

When I'm writing code in earnest I start with a script such as above for what I want to achieve, then I fill in with code. A bit like when writing an if, case, loop whatever I always include the terminating code (end if/end select/loop etc) before filling in the working code. Obviously modify the script if you encounter issues not initially considered such as finding you have to handle missing data

This in combination with GitHub CoPilot in VS Studio or VS Code enables fast code writing. Unfortunately not for VBA.

 

[ebs17]

Set RS2 = CurrentDb.OpenRecordset("SELECT * FROM tblWireRoom WHERE [ReelID] = " & vReelID & "")
With RS2
'make sure we are working on the correct reel
If ![ReelID] = vReelID Then ...

Is it normal not to trust your own filter? Why should it work on the second try if it fails on the first?
After filtering, you should rather test if the recordset is empty, which can potentially always occur.
And why are all table fields loaded via * when only one field (CurrentLength) is edited?

General: I do not overlook the meaning of all the code. But my first effort would be mostly to use action queries over everything instead of recordset loops with single entries, i.e. one query per table to be edited.

 

[Josef P.]

General: I do not overlook the meaning of all the code. But my first effort would be mostly to use action queries over everything instead of recordset loops with single entries, i.e. one query per table to be edited.

Exactly, across all records of a ticket, not looping across the records of a ticket.

But see #7:

I originally had tried to do this whole operation with queries, but kept running into problem after problem so decided to make it an exercise in writing out a module using DAO.

 

[CJ_London]

give an example where the code itself can be the comment

agree not everything needs to be commented - sometimes the code is clear enough. My point is write a script so you know what you are doing before you start - it is a micro project plan.

I give variables/function names such as booleans a name which when read in code implies true

if recExists(tablename, criteria) then

or

if isTaxed then

The danger with comments is: The code is revised, but the comment is not adapted.

that comes down to good discipline, as with naming conventions. On which I never use the underscore (personal preference, I find it much easier to read LastMonthValues.FieldOne than Last_Month_Values.Field_One)

For nested ifs I usually include a comment after the end if

if x=1 then
    if y=2 then
         ....
        ....
    end if 'y=2
     ....
     ,,,,,
end if 'x=1

 

[Josef P.]

if recExists(tablename, criteria) then
or
if isTaxed then

This is a very good example. The procedure name isTaxed describes the business content and not the logic of the code.
When I read that, I recognize what is being checked. How this is implemented is not relevant in this context. That is the task of isTaxed.

Even if the code of IsTaxed is only short, I think it is better to outsource that a procedure than do the check directly.

if isTaxed then ...
+
private property get isTaxed() as Boolean
     isTaxed = not Isnull(TaxCode)
end property

vs.

if not Isnull(TaxCode) then

 

[CJ_London]

don't disagree but it could be a function instead. I tend to use properties in forms when I want to get or let/set form variables from elsewhere without exposing all the workings of the form.

 

[tmyers]

So many replies since yesterday. I read everyone's thoughts and will start making some revisions based on the input and see what I come back with.

It makes sense to not open the recordset every single time within the loop, so that is most definitely an adjustment I will make. I will also add the ByRef and other suggestions you all have made and should hopefully report back later today with a revised module

 

[ebs17]

I originally had tried to do this whole operation with queries, but kept running into problem after problem

Maybe with help it would be different.
In retrospect, a comparison of recordset loops versus query solution would also be quite memorable and insightful.

 

[tmyers]

They are not required. They are used as a shortcut. If you have a vvvvvvvvvveeeeeeeeeeeeeeeerrrrrrrrrrryyyyyyyyyLong Recordset name, no one wants to have to keep typing that so the With is a way of substituting a ! for the name. And that's fine if the names are exceptionally long but no one forces you to create long names for this type of object. Also, if there is only one object you are using a With for in any procedure, there is no confusion but if i'm reading code and I see !somename in a procedure that uses multiple With clauses, I have to translate that to somerecordset!somename by reading backwards through the code to find the controlling With statement. Easy to get to the wrong one. Today, you remember which fields are in which recordset but how about next month or next year.

So, to summarize; I use short rs names and I never use With, if I have multiple objects I am working with at the same time to avoid confusion.

So to remove the With's, rather than doing With....!FieldName I would just remove the With and do RS.FieldName?

Edit:
Figured this one out quickly enough.

 

[tmyers]

Here are the changes I have made to the loop so far. I changed the recordset variables again to have better names (I have a really bad habit with keep them as short as possible).

I dropped all the With blocks per Pats suggestion. I also removed the INSERT INTO query I was creating and running during every iteration of the loop per I think also Pats suggestion.

I couldn't think of a better way to handle the WireRoomRecordset due to it being dependent on the current vReelID we are working with, but added that the recordset closes after every iteration of the loop. I also was unsure if I should close it each time I was done working with it or just at the end with the other recordsets.

I also moved the workspace to include the opening and closing of the recordsets. I also added in the Rollback to the ErrHandler although I didn't think it was needed as if the Commit isn't done, it was automatically rolled back.

On Error GoTo ErrHandler

vTicketID = [Forms]![frmWireRoom]![TicketID]
Set wrk = DBEngine.Workspaces(0)
Set DB = CurrentDb()

wrk.BeginTrans

    Set MultiRS = DB.OpenRecordset("SELECT MultiID, WireReelID, CutLength, CutNum, MultiComplete FROM tblMultiConductorCut " _
                                    & "WHERE TicketID = " & vTicketID & " AND MultiComplete = False")
    Set LogRS= DB.OpenRecordset("SELECT * FROM tblCutLog")
 
 
        Do While Not MultiRS.EOF
 
            vMultiID = MultiRS("MultiID")
            vReelID = MultiRS("WireReelID")
            vCutLength = MultiRS"CutLength")
            vCutNum = MultiRS("CutNum")
         
            'If number of cuts is null then set total to the cut length else multiply cut length times number of cuts
            If IsNull(vCutNum) Then
                TotalLength = vCutLength
            Else
                TotalLength = vCutLength * vCutNum
            End If
                 
                Set WireRS = DB.OpenRecordset("SELECT ReelID, CurrentLength FROM tblWireRoom WHERE [ReelID] = " & vReelID & "")
                 
                WireRS.Edit
                    vCurrentLength = WireRS("CurrentLength")
                    WireRS("CurrentLength") = vCurrentLength - TotalLength
                WireRS.Update
                WireRS.Close
                     
                MultiRS.Edit
                    MultiRS("MultiComplete") = True
                MultiRS.Update
             
                LogRS.AddNew
                    LogRS("CutReelID").Value = vReelID
                    LogRS("StartingLength").Value = vCurrentLength
                    LogRS("CutLength").Value = TotalLength
                    LogRS("EndLength").Value = vCurrentLength - TotalLength
                    LogRS("TicketID").Value = vTicketID
                LogRS.Update
                                       
                 
            MultiRS.MoveNext
         
        Loop

    MultiRS.Close
    LogRS.Close

wrk.CommitTrans


NoError = True
Exit Sub

ErrHandler:
NoError = False
    Select Case Err.Number
     
        Case 3317
            'one of the choosen reels did not have enough length
            MsgBox "One of the choosen source(s) has insufficient length for Multi-Conductor's. Please choose a different source. Operation has been canacelled and no changes have been made", vbOKOnly, "Error"
            wrk.Rollback
            NoError = False
            Exit Sub
         
        Case Else
            MsgBox Err.Description, vbOKOnly, Err.Number
            wrk.Rollback
         
     End Select

 

[Pat Hartman]

@tmyers

(I have a really bad habit with keep them as short as possible).

They were very short to begin with. You made them very long AND you used the longest possible reference style PLUS you are inconsistent even with that. MultiRS, WireRS, LogRS is a medium ground with the minimum number of characters to type. That leaves us with:
LogRS!TicketID = vTicketID

 

[tmyers]

Im so bad at naming. I went from one extreme to the other and didnt even notice it.

 

[Pat Hartman]

I have not looked at the code but generally it is rare that a query would not be able to do the whole job in one hit. Using recordsets and loops is best avoided if possible.

Looks like the code is generating multiple records from one. You need 50 feet of wire, the open roll has 20 so you take those. Then go to the next roll and subtract 30 from a full one. Both actions are logged. I'm not sure why the first transaction is being marked as closed though.

 

[tmyers]

Looks like the code is generating multiple records from one. You need 50 feet of wire, the open roll has 20 so you take those. Then go to the next roll and subtract 30 from a full one. Both actions are logged. I'm not sure why the first transaction is being marked as closed though.

I apologize as I am being yanked in multiple directions by coworkers so my mind is beginning to scramble, but what are you referring to by marking as closed? The MultiComplete?

 

[Pat Hartman]

I was referring to this code:

                With RS
                    'once again make sure we havent deviated from the current record for whatever reason
                    If ![MultiID] = vMultiID Then
                        RS.Edit
                        ![MultiComplete] = True
                        RS.Update
                    End If
                
                End With

 

[tmyers]

I was referring to this code:

                With RS
                    'once again make sure we havent deviated from the current record for whatever reason
                    If ![MultiID] = vMultiID Then
                        RS.Edit
                        ![MultiComplete] = True
                        RS.Update
                    End If
               
                End With

Got it. That IF block was removed and is now just:

MultiRS.Edit
    MultiRS("MultiComplete") = True
MultiRS.Update

But that field is from a previous iteration when the process was done one instance at a time, so it would flag it true to provide the person doing it a visual indicator that they had printed the label and such for that transaction. I have since repurposed it to help me know which items I have handled in the loop since I haven't figured out how to do an entire batch in one go.

So outside of providing me a visual check while testing code and giving me a field to more easily filter by, it doesn't really serve a purpose anymore. That is why the other two actions don't have it as they never needed it in all past iterations.

Looks like the code is generating multiple records from one. You need 50 feet of wire, the open roll has 20 so you take those. Then go to the next roll and subtract 30 from a full one.

Being able to take from multiple sources to fulfill one wire order is planned but I haven't yet come up with a way to handle it so it is currently only allowing them to take from a single source during the process and manually adjust it if taken from multiple.

 

[Pat Hartman]

Being able to take from multiple sources to fulfill one wire order is planned but I haven't yet come up with a way to handle it so it is currently only allowing them to take from a single source during the process and manually adjust it if taken from multiple.

There's no point in writing all this code if you are not going to do it right. Just use a query and ditch the code. You never asked how to fix the code.

 

[tmyers]

No, my intention was just to get feedback on a larger module that, for once, I wrote with nearly no help and see how bad/well I did. By the looks and sounds of it, I did not do a very good job.