A question of proper programming technique

[Rees]

Can someone help me clarify if this is the best practice, or just extra programming.

To me it seems best to place use a variable to hold the value from a worksheet that is going to be picked up multiple times during run time.

Ie. If I have this code which I created that will tell me the widest column width for a certain column range:

endRange = Range("D6").End(xlToRight).Cells.Column
For x = 4 To endRange
holdWidth = Cells(7, x).ColumnWidth
If holdWidth > largestWidth Then
For y = 4 To endRange
If holdWidth < Cells(7, y).ColumnWidth Then
holdWidth = Cells(7, y).ColumnWidth
largestWidth = Cells(7, y).ColumnWidth
End If
Next y
End If
Next x

I can then use largestWidth for the rest of my procedure


My question is: Do you think it is best practice to assign "Range("D6").End(xlToRight).Cells.Column" to the variable "endRange" and then use that variable in my For loops, or is that just overkill...it seems to me like that should be faster overall.

Any thoughts would be appreciated.

Thanks,

Rees

 

[HalfAce]

Hi Rees,
For what it's worth I believe using endRange would be faster. (Suppose I could be wrong though. Seems to me that actually happened once... )
Dan

 

[SIXTH SENSE]

Can someone help me clarify if this is the best practice, or just extra programming.

To me it seems best to place use a variable to hold the value from a worksheet that is going to be picked up multiple times during run time.

Ie. If I have this code which I created that will tell me the widest column width for a certain column range:

endRange = Range("D6").End(xlToRight).Cells.Column
For x = 4 To endRange
holdWidth = Cells(7, x).ColumnWidth
If holdWidth > largestWidth Then
For y = 4 To endRange
If holdWidth < Cells(7, y).ColumnWidth Then
holdWidth = Cells(7, y).ColumnWidth
largestWidth = Cells(7, y).ColumnWidth
End If
Next y
End If
Next x

I can then use largestWidth for the rest of my procedure


My question is: Do you think it is best practice to assign "Range("D6").End(xlToRight).Cells.Column" to the variable "endRange" and then use that variable in my For loops, or is that just overkill...it seems to me like that should be faster overall.

Any thoughts would be appreciated.

Thanks,

Rees

Hi!
For me, personally, i like that style, to assign that value to
a variable since it will become a "name" to that range. Its much
easier to remember if you name it

EndOfList1

to reffer to the last cell's row number than having it

Range("A65536").End(xlup).Row

it makes more sense.

 

[dk]

Hi,

If you're going to be referring to property values multiple times then assigning them to a variable can have more than one advantage:-

1. Your code will become smaller and easier to read.
2. The code will often run more quickly

To demonstrate my second point try running these two pieces of code:-

Sub PropertyValuesNotCached()
    Dim a As Long
    Dim sngTimer As Single, lngLoop As Long

    'Approx 20 seconds on my PC

    sngTimer = Timer

    For lngLoop = 1 To 1000000
        a = Range("D6").End(xlToRight).Cells.Column
    Next lngLoop

    MsgBox Timer - sngTimer

End Sub

Sub PropertyValuesCached()
    Dim a As Long, b As Long
    Dim sngTimer As Single, lngLoop As Long

    'Approx 0.03 seconds on my PC

    b = Range("D6").End(xlToRight).Cells.Column

    sngTimer = Timer

    For lngLoop = 1 To 1000000
        a = b
    Next lngLoop

    MsgBox Timer - sngTimer

End Sub

You should see that caching the propery value first has definite performance advantages. Ken Getz (a VBA demi-god) also reports performance improvements in this article.

 

[tusharm]

I would worry less about efficiency and extra programming and more about getting the logic right and understanding the XL object model better.

I reformatted your code for readability:

    endRange = Range("D6").End(xlToRight).Cells.Column
    For x = 4 To endRange
        holdWidth = Cells(7, x).ColumnWidth
        If holdWidth > largestWidth Then
            For y = 4 To endRange
                If holdWidth < Cells(7, y).ColumnWidth Then
                    holdWidth = Cells(7, y).ColumnWidth
                    largestWidth = Cells(7, y).ColumnWidth
                    End If
                Next y
            End If
        Next x

One of the two loops is essentially redundant! Just step through the code.
The first time through the X loop, largestWidth is uninitialized. Assuming it is declared as some kind of a number, the default value is zero. So, unless the column is hidden, holdWidth will be greater than largestWidth.

The Y loop goes through the entire list of cells-of-interest and sets both holdWidth and largestWidth to the same value -- the largest column width.

Now, when the code returns to the next iteration of the X loop, holdWidth can never be > largestWidth. So, the entire remainder of the X loop is cycled through without any meaningful processing being done!

Also, you are setting the limit of the check with row 6 and then working with row 7. Until and unless XL allows individual rows in the same column to have different widths, there's no need to do that.

Finally, I would definitely object to the name endRange. To me it implies a range object, but it actually refers to a column number. Use something like EndColID or EndColNbr.

So, as a first pass, the code can be simplified to:

Sub testit()
    Dim aCol  As Range, largestWidth As Single
    For Each aCol In Range(Range("D6"), _
            Range("d6").End(xlToRight))
        largestWidth = Application.WorksheetFunction.Max( _
            largestWidth, aCol.ColumnWidth)
        Next aCol
    MsgBox largestWidth
    End Sub

This still has two references to Range("D6"). If someone was to ever maintain this code, there is a small probability of introducing an error. The next version has a single reference to the starting point:

Sub testit2()
    Dim aCol  As Range, largestWidth As Single
    With Range("D6")
    For Each aCol In Range(.Cells(1), .End(xlToRight))
        largestWidth = Application.WorksheetFunction.Max( _
            largestWidth, aCol.ColumnWidth)
        Next aCol
        End With
    MsgBox largestWidth
    End Sub

This is still not very general purpose. All the work and all it does is address a single hardcoded range. Make it a function and its utility will surge:

Function MaxColWidth(aRng As Range) As Single
    Dim aCol As Range
    For Each aCol In aRng.Columns
        MaxColWidth = Application.WorksheetFunction.Max(MaxColWidth, aCol.ColumnWidth)
        Next aCol
    End Function
Sub testMaxColWidth()
    With Range("d6")
    MsgBox MaxColWidth(Range(.Cells(1, 1), .End(xlToRight)).Offset(1, 0))
        End With
End Sub

I would focus on this kind of 'best practice' first and then worry about other stuff.

Can someone help me clarify if this is the best practice, or just extra programming.

To me it seems best to place use a variable to hold the value from a worksheet that is going to be picked up multiple times during run time.

Ie. If I have this code which I created that will tell me the widest column width for a certain column range:

endRange = Range("D6").End(xlToRight).Cells.Column
For x = 4 To endRange
holdWidth = Cells(7, x).ColumnWidth
If holdWidth > largestWidth Then
For y = 4 To endRange
If holdWidth < Cells(7, y).ColumnWidth Then
holdWidth = Cells(7, y).ColumnWidth
largestWidth = Cells(7, y).ColumnWidth
End If
Next y
End If
Next x

I can then use largestWidth for the rest of my procedure


My question is: Do you think it is best practice to assign "Range("D6").End(xlToRight).Cells.Column" to the variable "endRange" and then use that variable in my For loops, or is that just overkill...it seems to me like that should be faster overall.

Any thoughts would be appreciated.

Thanks,

Rees

 

[Rees]

Thanks for all the input everyone, and especially, Tushar for taking the time to show me what I could improve on. Thanks again, I am still trying to hack my way through this stuff and appreciate the help that all put in here

Rees