0

I'm trying to convert numbers that have been stored in text to numbers on multiple worksheets. My issue is that the code I've cobbled together seems to be taking an inordinate amount of time. I'm using a For Each statement that loops through the necessary worksheets and ranges. It doesn't crash Excel, it just keeps running seemingly forever.

Sub ConvertTextToNumber()
    Application.ScreenUpdating = False
    Dim WshtNames As Variant
    Dim WshtNameCrnt As Variant
    Dim r As Range

    WshtNames = Array("Financial Data", "Site Data ", "Org Data", "Program Data")

    For Each WshtNameCrnt In WshtNames
    On Error Resume Next
        For Each r In Worksheets(WshtNameCrnt).UsedRange.SpecialCells(xlCellTypeConstants)
            If IsNumeric(r) Then r.Value = Val(r.Value)
        Next
    Next
    Application.ScreenUpdating = False
End Sub

When I stop the script from running and click Debug, it seems to be getting caught up at the first Next statement. I think that the method I'm using to convert the values is simply more time intensive than necessary, and thus running it on multiple sheets is even worse.

I'm open to any and all suggestions to make this process run faster. Thanks in advance!

Will Clein
  • 79
  • 8
  • Look into finding the last row and column used,loading everything into an array, do your changes and then put the values back. It will speed it up. As it is you are cycling through every cell testing it and changing it. That is going to take a long time. Arrays woulds speed it up exponentially. – Scott Craner Mar 22 '17 at 17:01
  • This would definitely help I imagine. As I know my data won't go beyond a certain point, my update works for me. However, if I find time, I will update the code to simply find the last row and column and post it here. Thanks! – Will Clein Mar 22 '17 at 18:10
  • instead of putting it in the question, please put the new code as an answer, as it is such. – Scott Craner Mar 22 '17 at 18:11
  • Sorry about that! I'm pretty new and haven't had the opportunity to provide my own answer yet. Thanks for the heads up! – Will Clein Mar 23 '17 at 14:54

3 Answers3

1

Try the code below. I used an index number instead of trying to loop through the array using a variant. I could be wrong, but I think For Each only works on collections. Someone please correct me if I am mistaken. (EDIT: I am indeed mistaken. For Each works just fine here.)

In any event, index numbers on arrays is best practice.

I also removed your Resume Next and properly handled it. I highly advise not using Resume Next. I cant think of any event that Resume Next can't be replaced by good logic.

Sub ConvertTextToNumber()
    Application.ScreenUpdating = False

    ' These two statements should further improve processing time.
    ' The first prevents formulas from calculating. The second prevents
    ' any background events from firing (mostly for Event triggered macros).
    Application.Calculation = xlCalculationManual
    Application.EnableEvents = False
    Dim WshtNames As Variant
    Dim i as Long
    Dim r As Range

    WshtNames = Array("Financial Data", "Site Data ", "Org Data", "Program Data")

    ' When looping over an array use an index number.
    ' I this case, 'i' will go from the lowest range of the array
    ' all the way through to the highest range of the array.
    For i = LBound(WshtNames) to Ubound(WshtNames)
        'On Error Resume Next ' It is best to catch the errors, dont just skip them.
        If Not Worksheets(WshtNames(i)) Is Nothing Then
            For Each r In Worksheets(WshtNames(i)).UsedRange.SpecialCells(xlCellTypeConstants)
                ' No need to check for an empty string here since
                ' IsNumeric() will return false for non-numbers.
                If IsNumeric(r) Then r.Value = Val(r.Value)
            Next
        Else
            ' Put your error handling in here, or you can just skip it
            ' I tend to use debug.print just to keep track.
            Debug.Print WshtNames(i) & " doesn't exist."
        End If
    Next
    Application.ScreenUpdating = True

    Application.Calculation = xlCalculationAutomatic
    Application.EnableEvents = True
End Sub
Brandon Barney
  • 2,382
  • 1
  • 9
  • 18
  • 1
    I tried it without putting Calculation to manual and disabling events, and it took just as long, but with the addition of those two, it now runs in about 30 secs which is light years better. Thanks for your help! Also, I kept the Debug.Print option. I don't typically use it, but this is for an end-user so giving them dialogs about issues is great advice. – Will Clein Mar 22 '17 at 17:03
  • 1
    Glad to hear it! The calculation event is likely the main performance increase. Keep in mind though that your end user will likely never see the debug.print unless they are writing VBA as well. You can overcome this by using a messagebox, but your sheetnames are hard coded so this is less than ideal. – Brandon Barney Mar 22 '17 at 17:05
1

I initially used Brandon's answer, but upon his suggestion I looked into using an array to store the values and make changes in memory. Below is the updated code I am now using:

Sub ConvertTextToNumber()
    Application.ScreenUpdating = False
    Application.Calculation = xlCalculationManual
    Application.EnableEvents = False
    Dim WshtNames As Variant
    Dim DataRange As Variant
    Dim r As Range
    Dim i As Long
    Dim lrow As Long
    Dim lcol As Integer
    Dim MyVar

    WshtNames = Array("Financial Data", "Site Data ", "Org Data", "Program Data")
    DataRange = Range("A1:FZ6000").Formula

    For lrow = 1 To 6000
        For lcol = 1 To 156
        MyVar = DataRange(lrow, lcol)
        If IsNumeric(MyVar) Then
            MyVar = Val(MyVar)
            DataRange(lrow, lcol) = MyVar
        End If
        Next lcol
    Next lrow
    Range("A1:FZ6000").Formula = DataRange

    Application.ScreenUpdating = False
    Application.Calculation = xlCalculationAutomatic
    Application.EnableEvents = True
End Sub

This works for me because I know that my sheets will never get beyond the ranges I've selected based on the nature of my data. This has effectively reduced my calculation time to ~2 secs. Thanks for everyone's input and happy coding!

Will Clein
  • 79
  • 8
0

Well this is because your loop is literally going through every cell. Best to make some checks for blank values

 IF r.Value <> "" or r.value <> vbnullstring then
Doug Coats
  • 6,255
  • 9
  • 27
  • 49
  • "" and vbNullstring are the same, so it would be best to use 'If r.Value <> vbNullString then'. – Brandon Barney Mar 22 '17 at 16:04
  • @BrandonBarney actually they're not. Null values versus white space are two different things and get handled differently. This is especially true with excel. – Doug Coats Mar 22 '17 at 16:21
  • whitespace and "" are two different things. vbNullString is a constant expression for "". You can even use the immediate window to check for equality between the two (Debug.Print "" = vbNullString returns True). Now, a whitespace character is a whole different story, and certainly is handled differently, but "" is not whitespace. You may be interested in this: http://stackoverflow.com/questions/32435320/is-there-any-difference-between-vbnullstring-and. – Brandon Barney Mar 22 '17 at 16:23
  • @BrandonBarney please see http://stackoverflow.com/questions/32435320/is-there-any-difference-between-vbnullstring-and – Doug Coats Mar 22 '17 at 16:25
  • It seems we shared the same page with eachother. As that post explains though, they are treated the same but the memory usage is different. As noted before, checking for quality between vbNullString and "" returns true. As we can see from the MSDN page as well (https://msdn.microsoft.com/en-us/library/microsoft.visualbasic.constants.vbnullstring(v=vs.110).aspx) vbNullstring is literally a 0 length string (as is ""). – Brandon Barney Mar 22 '17 at 16:28
  • Also, for clarity I am not trying to argue and I am certainly not saying your advice was wrong. I am simply recommending that it is redundant due to the way the two are treated. It took me months before I discovered that "" and vbNullstring were the same, but that the latter was better for memory management and so now I do my best to point others in the right direction. The way I see it, there is no use in using a less effective alternative. – Brandon Barney Mar 22 '17 at 16:30
  • Okay lets assume they are different as Doug states, doing an or on a negative will result in it returning true. for example if the cell has `""` then the vbnullstring will return true and vice versa, I think you want AND if they are truly different – Scott Craner Mar 22 '17 at 16:31
  • No, you would want Or, assuming you want to skip either type. The whole thing is a useless argument as is given that SpecialCells(xlCellTypeConstants) wont return any null cells. It only returns cells with values. Additionally, any cells that did have data but were whitespace would also be skipped by the "IsNumeric()" statement. – Brandon Barney Mar 22 '17 at 16:33
  • @BrandonBarney think about it Or returns true if either is true. Lets change it to `A` and `B`, `If r.value<>"A" or r.value<>"B" then` would fire if the cell is A or B. If `r.value` = A then the If would resolve to `If False or True then` which would be `True`, if we want to not do the If block when it does not equal either then we need and `If r.value<>"A" And r.value<>"B" then` which would resolve to in the previous example `If False And True then` which would return `False`. But you are correct in this instance both will either resolve to true or false together so the Or/And is the same. – Scott Craner Mar 22 '17 at 16:42
  • This got lively in my short absence. We've put more effort into this than the OP has even bothered reviewing, which i find humorous. – Doug Coats Mar 22 '17 at 16:45
  • @ScottCraner I know the difference between Or and And, but in this case (assuming they are different) then AND would never evaluate to true. A cell value can't be two constant values. There are many applications for AND (of course), but in this case replacing the Or with AND would break the logic of the comparison (again, assuming that the two are different. In actuality, the comparison would work just fine since both statements would evaluate as true given that they both evaluate the same). – Brandon Barney Mar 22 '17 at 16:46
  • @DougCoats Agreed. Admittedly I only really spend the time on discussion in the comments in case anyone comes back to this thread in the future in hopes of solving a problem of their own. I know there were many times where I wouldn't have solved a particular task if someone hadnt provided the comment that ended up being the silver bullet. – Brandon Barney Mar 22 '17 at 16:47
  • @ScottCraner Scratch my statement about using AND versus OR. For some reason, in all the confusion, I was thinking of checking for inequality on both sides and not the reverse. You are correct in that AND should be used since, if they were evaluated differently, it could present an issue if one was used over the other. – Brandon Barney Mar 22 '17 at 16:54
  • 2
    @BrandonBarney man, I was just about to post a picture proving my point and you had to ruin my moment by taking the higher road. :) – Scott Craner Mar 22 '17 at 16:57
  • 1
    OP here, I have actually been reading this exchange as I've been testing both of the proposed solutions. The distinction between '""' and 'vbNullString' is certainly interesting. To my predicament though, it seems this solution does exactly what mine does without actually taking less time. I think I may just have to skip making this change. Thanks for trying! – Will Clein Mar 22 '17 at 16:57
  • @WillClein You can see the other solution to see if it helps at all. Unfortunately, any time you are modifying values on the sheet directly you will see a performance decrease. My usual advice is to use an array if possible. You can load all the data into memory, modify them in memory, then overwrite. There may be other events that can be disables as well, but the performance cost will increase with the size of the data. – Brandon Barney Mar 22 '17 at 17:02
  • @BrandonBarney Ultimately my ideal solution is to get our IT department to not have those fields be formatted as text when I'm pulling reports from one of our SQL servers, but that's a low priority issue for them right now so I'm having to take care of it myself in this manner. – Will Clein Mar 22 '17 at 17:06
  • @WillClein Makes perfect sense. That's how I got started in VBA as well. We use so many different reports and I was tired of having to do legwork to make the reports useful (since they were designed for hundreds or more people to use, and not tailored for my use). Hopefully you can still get them to do it even though it is automated now :). – Brandon Barney Mar 22 '17 at 17:16