6
\$\begingroup\$

For a project I created a more detailed last row finder which looks at more than one column.

Could you provide any feed back on the code below.

Function lColCount(ws As Worksheet, Optional iWhichRow As Integer = 1) As Long
Dim sMaxCol As String
sMaxCol = Cells(iWhichRow, ws.Columns.Count).Address
lColCount = ws.Range(sMaxCol).End(xlToLeft).Column
Function`

Function lRowCount(ws As Worksheet, Optional iWhichCol As Integer = 1) As Long
Dim sMaxRow As String
sMaxRow = Cells(ws.Rows.Count, iWhichCol).Address
lRowCount = ws.Range(sMaxRow).End(xlUp).Row
End Function

Function Col_Letter(lngCol As Long) As String
Dim vArr
vArr = Split(Cells(1, lngCol).Address(True, False), "$")
Col_Letter = vArr(0)
End Function`

Sub findLast()
Dim LC As Long
Dim LR As Long
Dim LCa As String
Dim ws As Worksheet
Dim i As Integer
Dim arr() As Variant
'Assigns ws = sheet1
Set ws = Sheet1
'Sets LC = to the last column number of default row 1
LC = lColCount(ws)
'I counter = 1
i = 1
'Defines array as 1 to the last column number
ReDim arr(1 To LC)
'Loops untill I = Last column counter + 1
Do While i <> LC + 1
'Sets Lr to last row of the looping column i
LR = lRowCount(ws, i)
'Sets last row to arr(I) value loop
arr(i) = LR
'Adds 1 to I ready for next loop
i = i + 1
Loop
'Finds maximum value in array and uses that as last row
LR = Application.WorksheetFunction.Max(arr())
'Use to check the array value in the Immediate window
Dim Res As Variant
For Each Res In arr()
Debug.Print Res
Next Res
'Col_letter function finds the letter of the column number
LCa = Col_Letter(LC)
'copies range A1 sheet1 and all data to sheet2
ws.Range("A1:" & LCa & LR).Copy Sheet2.Range("A1")
End Sub
\$\endgroup\$
4
\$\begingroup\$

You end two functions like this -

End Function`

Which might be a typo, but it causes errors.


Readability

Indentation

Indenting your code makes it a lot easier to follow and maintain

Sub findLast2()
    '<snip>
    Dim i As Integer
    Dim arr() As Variant
    Set ws = Sheet1
    LC = lColCount(ws)
    i = 1
    ReDim arr(1 To LC)
    Do While i <> LC + 1
        LR = lRowCount(ws, i)
        arr(i) = LR
        i = i + 1
    Loop
    LR = Application.WorksheetFunction.Max(arr())
    Dim Res As Variant
    For Each Res In arr()
        Debug.Print Res
    Next Res
    '<snip>
End Sub

Comments

"code tell you how, comments tell you why". The code should speak for itself, if it needs a comment, it might need to be made more clear. If not, the comment should describe why you're doing something rather than how you're doing it. Here are a few reasons to avoid comments all together.

Naming

Variable names - give your variables meaningful names.

What is LC? lastColumn. LR? lastRow What is vArr() an array of?

Additionally, Standard VBA naming conventions have camelCase for local variables and PascalCase for other variables and names.

Structure

If possible, you should pass argument ByVal rather than ByRef.

You also declare all of your subs and functions as Public by default, so call them public or private to ensure it's clear

Public Sub FindLastRow()
Private Function ReturnColumnLetter(ByVal columnNumber As Long) As String

and so on.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I had a nightmare copying the into the web browser which is why ` was included. (Could not work out how to surround my code). I will take note of the other suggestions. \$\endgroup\$ – Rhys Williams Jan 20 '17 at 13:07
  • \$\begingroup\$ I figured that's what happened, which is why I mentioned it. If it doesn't compile, usually the question is closed, but in this case it was simply a typo. \$\endgroup\$ – Raystafarian Jan 20 '17 at 14:11
  • \$\begingroup\$ Also valid to point out that, when working with rows and columns, it is always advisable to use Long variables instead of Integer and there are some not-fully-qualified cells in both Cells(iWhichRow, ws.Columns.Count).Address and Cells(ws.Rows.Count, iWhichCol).Address, which might cause some trouble in future \$\endgroup\$ – Victor Moraes Jan 20 '17 at 16:50
  • \$\begingroup\$ @VictorMoraes Yes to both. Integers - integers are obsolete. According to msdn VBA silently converts all integers to long. I just didn't address those things. \$\endgroup\$ – Raystafarian Jan 20 '17 at 17:00
  • \$\begingroup\$ I have amended the code- I have changed it so that I can pass the WS variable and which row, so that I can call this section of code from another sub. However I can't return the Lastrow and Lastcolumn value to the Sub that is calling the above. I have a work around where I add a sheet assign the results of last row and Column to a cell and use these in the new Sub, Is their a better way to do this? should I start a new thread? \$\endgroup\$ – Rhys Williams Jan 24 '17 at 16:02

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.