0

Is there a reason why my script takes ages to run? This is only part of the code, but it is the part that slows it down. The sheet Report is a report coming from an e-patient system. it contains visit dates and those dates need to be compared with the dates in the sheet PtLog. In the PtLog each line is one patient, as for the sheet Report each visit is a line. So patient can be on several lines in the sheet Report. there are 11 possible visit dates and about 700 possible patients. Meaning about 7700 dates need to be checked. I hope i made myself somewhat clear...

thx in advance

 Application.ScreenUpdating = False
 Application.EnableEvents = False
 Application.Calculation = xlCalculationManual

 For colPtLog = 11 To 20

    For rowPtLog = 2 To lastRowUsedPtLog

        Sheets("PtLog").Select
        patientNrPtLog = Cells(rowPtLog, 5).Value
        nrVisitPtLog = Cells(1, colPtLog).Value
        dateVisitPtLog = Cells(rowPtLog, colPtLog).Value

        Sheets("Report").Select

        For rowReport = 2 To lastRowUsedReport

            Sheets("Report").Select
            dateVisitReport = Sheets("Report").Cells(rowReport, 6)
            patientNrReport = Sheets("Report").Cells(rowReport, 2)
            nrVisitReport = Sheets("Report").Cells(rowReport, 4)


            If patientNrPtLog = patientNrReport And nrVisitPtLog = nrVisitReport Then

                If dateVisitPtLog <> dateVisitReport Then

                    If dateVisitPtLog > 0 And dateVisitReport = 0 Then

                        Sheets("CONTROL").Select
                        lastRowUsedControlVisitNoDate = lastRowUsedControlVisitNoDate + 1
                        Cells(lastRowUsedControlVisitNoDate, 2) = patientNrPtLog
                        Cells(lastRowUsedControlVisitNoDate, 3) = nrVisitPtLog

                    End If


                    If dateVisitPtLog = 0 And dateVisitReport > 0 Then

                        Sheets("PtLog").Cells(rowPtLog, colPtLog) = dateVisitReport
                        With Sheets("PtLog").Cells(rowPtLog, colPtLog).Font
                            .Color = -1003520
                            .TintAndShade = 0
                        End With

                    End If


                    If dateVisitPtLog > 0 And dateVisitReport > 0 Then

                        Sheets("CONTROL").Select
                        lastRowUsedControlDateNoMatch = lastRowUsedControlDateNoMatch + 1
                        Cells(lastRowUsedControlDateNoMatch, 9) = patientNrPtLog
                        Cells(lastRowUsedControlDateNoMatch, 10) = nrVisitPtLog
                        Cells(lastRowUsedControlDateNoMatch, 11) = dateVisitReport
                        Cells(lastRowUsedControlDateNoMatch, 12) = dateVisitPtLog

                    End If

                End If

                Exit For

            End If

        Next rowReport

    Next rowPtLog

Next colPtLog

Application.ScreenUpdating = True
Application.EnableEvents = True
Application.Calculation = xlCalculationAutomatic

2 Answers 2

3

There are several things you can do to improve your code:

(1) Do not select sheets in your code but rather directly assign the value to the variables. So instead of:

Sheets("PtLog").Select
patientNrPtLog = Cells(rowPtLog, 5).Value
nrVisitPtLog = Cells(1, colPtLog).Value
dateVisitPtLog = Cells(rowPtLog, colPtLog).Value

you should try this:

With Sheets("PtLog")
    patientNrPtLog = .Cells(rowPtLog, 5).Value
    nrVisitPtLog = .Cells(1, colPtLog).Value
    dateVisitPtLog = .Cells(rowPtLog, colPtLog).Value
End With

(2) Do not use .Value but rather .Value2 if possible. So, for the above snippet this would mean that you can further improve the code as follows.

With Sheets("PtLog")
    patientNrPtLog = .Cells(rowPtLog, 5).Value2
    nrVisitPtLog = .Cells(1, colPtLog).Value2
    dateVisitPtLog = .Cells(rowPtLog, colPtLog).Value2
End With

(3) Declare all variables that you are using in your code. If you do not declare variables then VBA will automatically assume the variables to be of type variant which are the least performant. So, you should write (before all Subs) the following line:

Option Explicit

And in your sub you should declare all variables. Here are some examples.

Dim rowPtLog As Long
Dim lastRowUsedReport As Long
Dim dateVisitPtLog As Date
Dim dateVisitReport As Date

(4) When you write back to the sheet then you should also be explicit and write out that you want to assign the .Value2 to the cell. So, instead of

Sheets("PtLog").Cells(rowPtLog, colPtLog)

you should write

Sheets("PtLog").Cells(rowPtLog, colPtLog).Value2

Note, that VBA / Excel is very fast at processing data in memory. But writing data back to the sheets is slowing down your code. Try to limit these lines (if at all possible).

(5) Make sure that lastRowUsedPtLog and lastRowUsedReport are not too high. These are two inner loops. So, if the first is a large number (5 or more digits) and the second number is also very large then this can easily result in a couple million iterations which will slow down your code too.

(6) Skip rows if possible. If the above loops cannot be avoided then you should try to skip rows which are not necessary to process. For example, if there is not patientNrPtLog in column 5 then maybe there is not need to go through this row. So, you could include another if..then to only process the line if necessary or skip it otherwise.

The above points should be already to get you started. Let us know how things are improving afterwards and possibly also implement time trackers in your code to see where the biggest time loss is. This could be done like so:

Dim dttProcedureStartTime As Date
dttProcedureStartTime = Now()

Afterwards you can track the time with code-lines like these:

Debug.Print Now() - dttProcedureStartTime

Maybe like this you can identify the biggest "time loosers".

Sign up to request clarification or add additional context in comments.

1 Comment

Thank you very much for your time in answering my question. I'm going to try your suggestions tonight/tomorrow and i will let you know how it turned out. I really appreciate your efforts!
0

I think the actual slowness of OP code is due to useless loops

here's a code with same results as OP's one but looping through cells only when necessary

Option Explicit

Sub SubMine()
Dim lastRowUsedPtLog As Long, lastRowUsedReport As Long
Dim lastRowUsedControlVisitNoDate As Long, lastRowUsedControlDateNoMatch As Long

Dim ptLogDdateVisit As Long
Dim reportPatientNr As Long, reportNrVisit As Long, reportDateVisit As Long

Dim reportSht As Worksheet, ptLogSht As Worksheet, controlSht As Worksheet

Dim ptLogPatientNrs As Range, ptLogPatientNrCells As Range, ptLogPatientNrCell As Range
Dim ptLogVisitNrs As Range, ptLogNrVisitCell As Range, ptLogDateVisitCell As Range
Dim reportPatientNrs As Range, reportPatientNrCell As Range
Dim ptLogCellsToMark As Range


Application.ScreenUpdating = False
Application.EnableEvents = False
Application.Calculation = xlCalculationManual

Set reportSht = Sheets("Report")
Set ptLogSht = Sheets("PtLog")
Set controlSht = Sheets("CONTROL")

' to avoid first "Union()" method call to fail, I set a dummy ptLogCellsToMark
With ptLogSht
    Set ptLogCellsToMark = .Cells(1, .Columns.Count)
End With

lastRowUsedPtLog = GetLastRow(ptLogSht, 5)
lastRowUsedReport = GetLastRow(reportSht, 2)
lastRowUsedControlVisitNoDate = GetLastRow(controlSht, 2)
lastRowUsedControlDateNoMatch = GetLastRow(controlSht, 9)

Set ptLogPatientNrs = ptLogSht.Cells(2, 5).Resize(lastRowUsedPtLog) 'list of PatientNr in "PtLog" sheet
Set ptLogVisitNrs = ptLogSht.Range("K1:T1") 'list of VisitNr in "PtLog" sheet
Set reportPatientNrs = reportSht.Cells(2, 2).Resize(lastRowUsedReport) 'list of PatientNr in "Report" sheet

For Each reportPatientNrCell In reportPatientNrs 'loop through PatientNr of "Report" Sheet

    reportPatientNr = reportPatientNrCell.Value ' track patientNr value from "Report" sheet
    Set ptLogPatientNrCells = FindValues(reportPatientNr, ptLogPatientNrs) ' find ALL occurencies of that patientNr value in "PtLog" sheet
    If Not ptLogPatientNrCells Is Nothing Then ' if there's an occurrence of that patientNr in "PtLog" sheet

        reportNrVisit = reportPatientNrCell.Offset(, 2) ' now it makes sense to track nrVisit value from "Report" sheet
        Set ptLogNrVisitCell = ptLogVisitNrs.Find(reportNrVisit) ' search for that nrVisit value in "PtLog" sheet
        If Not ptLogNrVisitCell Is Nothing Then ' if there's an occurrence of that nrVisit value in "PtLog" sheet

            reportDateVisit = reportPatientNrCell.Offset(, 4) ' now it makes sense to track dateVisit value from "Report" sheet

            For Each ptLogPatientNrCell In ptLogPatientNrCells 'loop through ALL occurencies of report patientNr of "PtLog" Sheet

                Set ptLogDateVisitCell = ptLogSht.Cells(ptLogPatientNrCell.Row, ptLogNrVisitCell.column) 'set the "PtLog" sheet cell with the date corresponding to patientNr and nrVisit from "report" sheet
                ptLogDdateVisit = ptLogDateVisitCell.Value

                Select Case True
                    Case ptLogDdateVisit > 0 And reportDateVisit = 0
                        lastRowUsedControlVisitNoDate = lastRowUsedControlVisitNoDate + 1
                        controlSht.Cells(lastRowUsedControlVisitNoDate, 2).Resize(, 3) = Array(reportPatientNr, reportNrVisit, ptLogDdateVisit) ' write in "CONTROL" sheet . NOTE: I added "ptLogDdateVisit" to keep track of what was date was not peresent in "Report" sheet

                    Case ptLogDdateVisit = 0 And reportDateVisit > 0
                        With ptLogDateVisitCell
                            .Value = reportDateVisit 'correct the "PtLog" sheet date value with the "Report" sheet one
                            Set ptLogCellsToMark = Union(ptLogCellsToMark, .Cells(1, 1)) ' add this cell to those that will be formatted at the end
                        End With

                    Case Else
                        lastRowUsedControlDateNoMatch = lastRowUsedControlDateNoMatch + 1
                        controlSht.Cells(lastRowUsedControlDateNoMatch, 9).Resize(, 4) = Array(reportPatientNr, reportNrVisit, reportDateVisit, ptLogDdateVisit) ' write in "CONTROL" sheet
                End Select

            Next ptLogPatientNrCell

        Else

            ' here code to handle what to do when a nrVist in "Report" sheet is not present in "PtLog" sheet

        End If


    Else

        ' here code to handle what to do when a patientNr in "Report" sheet is not present in "PtLog" sheet

    End If

Next reportPatientNrCell

With ptLogCellsToMark.Font
    .Color = -1003520
    .TintAndShade = 0
End With


Application.ScreenUpdating = True
Application.EnableEvents = True
Application.Calculation = xlCalculationAutomatic

End Sub


Function FindValues(valueToFind As Variant, rngToSearchIn As Range) As Range
Dim cell As Range, unionRng As Range
Dim firstAddress As String

With rngToSearchIn
    Set cell = .Find(What:=valueToFind, LookAt:=xlWhole)
    If Not cell Is Nothing Then
        firstAddress = cell.Address
        Set unionRng = cell
        Do
            Set unionRng = Union(unionRng, cell)

            Set cell = .FindNext(cell)
        Loop While Not cell Is Nothing And cell.Address <> firstAddress
        Set FindValues = unionRng
    End If
End With

End Function


Function GetLastRow(sht As Worksheet, column As Long) As Long
With sht
    GetLastRow = .Cells(.Rows.Count, column).End(xlUp).Row
End With
End Function

3 Comments

Thank you very much for your time in answering my question. I'm going to try your suggestions tonight/tomorrow and i will let you know how it turned out. I really appreciate your efforts!
Your code is crazy good! It did the task in under 3 seconds! i'm going to study your code because i don't understand everything it does for the moment. Maybe i'll ask you some more questions if can't find the answer myself, if that's okay with you ofcourse... :) Thx!
Glad to know it helped. And to know you'll not just use the code but want to understand it. No problem with questions from you but I can't garantee quick answers. Finally, if I really fulfilled your first need you may want to upvote my answer and/or give some rep.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.