Help me improve some old code

[EirikDaude]

In one of my modules I had this short function:

Function makeList(startCell As Range) As Range
  Dim helper As Range, i As Integer
  
  With startCell
    Set helper = Range(.Offset(0, 0), .End(xlDown))
    Do While ((helper.Item(helper.Count + i).Value = 0) And (helper.Count + i > 0))
      i = i - 1
    Loop
    Set helper = Nothing
    Set makeList = Range(.Offset(0, 0), .End(xlDown).Offset(i, 0))
  End With
End Function

As far as I can recall (and glean from the code) it is supposed to take a single cell as input and return the all the cells with values below it in the column. The loop in the middle is to prevent it from including the entire column if there are no cells with value below the input-cell. However, I suspect that looping through the entire column is somewhat inefficient, so I wondered if any of you people had some suggestions on how I can improve the function?

[Bob Phillips]

Function makeList(startCell As Range) As Range
    Set makeList = Range(startCell, startCell.End(xlDown))
End Function

[EirikDaude]

Won't that give me the entire column if startcell is the bottommost entry?

[snb]

You can test the code to answer your question.

Function makeList(startCell As Range) As Range 
    Set makeList = Range(startCell, cells(rows.count,startCell.column).End(xlup)) 
End Function

[Bob Phillips]

Won't that give me the entire column if startcell is the bottommost entry?

Might well do, but as you don't know what the code is supposed to do, it is hard for us to guess it.

[EirikDaude]

You can test the code to answer your question.

And he could have read the OP:

The loop in the middle is to prevent it from including the entire column if there are no cells with value below the input-cell.

Function makeList(startCell As Range) As Range 
    Set makeList = Range(startCell, cells(rows.count,startCell.column).End(xlup)) 
End Function

This is how I would usually do it, but since the cell could happen to be empty, and I don't want to include possible cells above the input either, I managed to do it was this finagling in the end:

Function makeList(startCell As Range) As Range
    
    Set makeList = Range(startCell, Cells(Rows.Count, startCell.Column).End(xlUp))
    
    If makeList.Row < startCell.Row Then
      Set makeList = startCell
    End If
    
End Function

I guess I should have specified that more explicitly in the OP, instead of just implying it

But thanks a lot for both of your inputs. And sorry for wasting your time :P I guess I should have spent a bit more time googling and trying before posting here.

[SamT]

The loop performs no actions

       Set helper = Range(.Offset(0, 0), .End(xlDown)) 
        Do While ((helper.Item(helper.Count + i).Value = 0) And (helper.Count + i > 0)) 
            i = i - 1 
        Loop 
        Set helper = Nothing

The loop will only be entered if the bottom cell of the list is a formula that returns zero

helper.Item(helper.Count - 0.)Value = 0

OR the the cell below startCell is empty, thereby setting helper and makeList to the range from startCell to the bottom of the sheet.

Without the second While clause, if the loop was ever entered (quite possible) it would throw a Subscript Out Of Range error as soon as Abs(i) > Helper.Count.

I think it is a signature.

[EirikDaude]

Hmm. That is kinda interersting. I thought what it would do was to start iterating over the column from the bottom upwards, if the cell below the startercell is empty, and then exit the loop if it reaches the startercell? Or am I misunderstanding you? I certainly can't see what the second while loop is, at least

As far as I can tell it hasn't caused any errors in the sheet, but I am replacing it with the function I posted in my previous post, as that is far less convoluted and I am also more confident that it will do what is intended.

Thanks for your reply!

[Bob Phillips]

As far as I can tell it hasn't caused any errors in the sheet, but I am replacing it with the function I posted in my previous post, as that is far less convoluted and I am also more confident that it will do what is intended.

How does that test in your code ever get to be true? If you work upwards from the bottom of the column that startcell is in, you can never arrive at a row less than startcell's row can you?

[EirikDaude]

Startcell doesn't necessarily have to be at the top of the column. And if startcell is empty, range.end(xlup) will choose either the first cell with a value above it, or the topmost cell of the column.

- edit - I created a sample workbook: New Microsoft Excel Worksheet.xlsm

It seems that either I misunderstood how the original function is supposed to work, or there is something funky going on inside that while-loop =/

[SamT]

I certainly can't see what the second while loop is, at least

Sorry, not enough coffee. Brain still in "granny low" gear.

"Without the second While Condition, (And (helper.Count + i > 0,) if the loop was ever entered (quite possible) it would throw a Subscript Out Of Range error as soon as Abs(i) > Helper.Count."

You could use

Function makeList(startCell As Range) As Range 
     'At this point makeList = Nothing     
    If Not startCell = "" Then Set makeList = Range(startCell, Cells(Rows.Count, startCell.Column).End(xlUp)) 
End Function

I would use

Function makeList(startCell As Range) As Range 
'Covers all possibilities except startCell not being in the correct range or being a multi-celled range.
     'At this point in code, makeList = Nothing
    If startCell = "" Then Goto ErrHandler
    If startCell.Offset(0, 1) = "" Then 
       Set makeList = startCell
    Else
        Set makeList = Range(startCell, Cells(Rows.Count, startCell.Column).End(xlUp)) 
   End If
Exit Function
ErrHandler:
MsgBox "Improper use of Function makeList in module XYZ"
End Function

[Paul_Hossler]

If there's empty cells within the top/bottom range, I'd guess that they were not to be included??

Option Explicit
Sub test()
    With ActiveSheet
        .Range("C:C").ClearContents
        
        .Range("C3:C20").Value = 1234
        .Range("C33:C45").Formula = "=12*12*12"
'        .Range("C33:C45").Value = 5678
        .Range("C55:C70").Value = 9012
        MsgBox makeList(.Range("C1")).Address
        
    End With
End Sub
 
Function makeList(rStart As Range) As Range
    Dim rTop As Range, rBottom As Range, rData As Range, rConst As Range, rForm As Range
    Set makeList = Nothing
    
    Set rTop = rStart.Cells(1, 1).End(xlDown)
    If IsEmpty(rTop) Then Exit Function
    
    Set rBottom = rTop.Parent.Cells(rTop.Parent.Rows.Count, rTop.Column).End(xlUp)
        
    If rTop.Address = rBottom.Address Then
        Set makeList = rTop
        Exit Function
    End If
    
    
    On Error Resume Next
    Set rConst = Range(rTop, rBottom).SpecialCells(xlCellTypeConstants)
    Set rForm = Range(rTop, rBottom).SpecialCells(xlCellTypeFormulas)
    On Error GoTo 0
    
    If Not rConst Is Nothing And Not rForm Is Nothing Then
        Set makeList = Union(rConst, rForm)
    ElseIf Not rConst Is Nothing And rForm Is Nothing Then
        Set makeList = rConst
    ElseIf rConst Is Nothing And Not rForm Is Nothing Then
        Set makeList = rForm
    Else
        Set makeList = Nothing
    End If
End Function

Paul

[snb]

Function makeListS(startCell As Range) As String
    makeListS = startCell.Address
    If Cells(Rows.Count, startCell.Column).End(xlUp).Row > startCell.Row Then makeListS = Range(startCell, Cells(Rows.Count, startCell.Column).End(xlUp)).Address
End Function

[EirikDaude]

Thanks for all of your replies! It was interesting to see your different takes on it!

If there's empty cells within the top/bottom range, I'd guess that they were not to be included??

Paul

Well, ideally that would be the case, but (as I didn't tell you) the function is used to generate validation lists based on the name of the sheet, the actual generation of the lists look something like:

With changedCell
    Set listStartsInCell = listStartsInCell.Find(sheetname, MatchCase:=False).Offset(2, 1)
    .Offset(0, 1).Validation.Add Type:=xlValidateList, AlertStyle:=xlValidAlertStop, Operator:= _
        xlBetween, Formula1:=("=" + makeList(listStartsInCell.Offset(0, 1)).Address(external:=True))
    .Offset(0, 2).Validation.Add Type:=xlValidateList, AlertStyle:=xlValidAlertStop, Operator:= _
        xlBetween, Formula1:=("=" + makeList(listStartsInCell.Offset(0, 2)).Address(external:=True))
End With

I don't think that works with a union of cells, I guess I could add in some checking and editing of the possible lists, but as the users hopefully won't tamper with them I think I'll just try to go with the current version of the formula for now.

And again, thanks a ton for all of your input, it helps my own thinking too to be able to air some of my ideas here