PDA

View Full Version : [SOLVED] Help me improve some old code



EirikDaude
04-01-2014, 02:25 AM
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
04-01-2014, 02:51 AM
Function makeList(startCell As Range) As Range
Set makeList = Range(startCell, startCell.End(xlDown))
End Function

EirikDaude
04-01-2014, 03:02 AM
Won't that give me the entire column if startcell is the bottommost entry?

snb
04-01-2014, 03:25 AM
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
04-01-2014, 05:18 AM
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
04-01-2014, 05:51 AM
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
04-01-2014, 06:14 AM
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
04-01-2014, 06:28 AM
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
04-01-2014, 07:35 AM
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
04-01-2014, 07:42 AM
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: 11491

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
04-01-2014, 08:12 AM
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
04-01-2014, 08:52 AM
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
04-01-2014, 09:43 AM
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
04-03-2014, 05:57 AM
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 :)