Overall, pretty good code. There are a couple things I would personally change though.
1 - I'd start by qualifying the references to Cells
and Range
even if it's assumed that everything is done with the same sheet - I'd grab a reference to ActiveSheet
right at the start and use that throughout. Using a Worksheet
reference inside a With
block is much faster than using the global objects. Outside of the performance, it's generally a good habit to get into as it avoids using an implicit reference.
2 - The call to calculate the size of the array is a bit of overkill - there really isn't a need for a function call with:
size = WorksheetFunction.Count(Range(Cells(2, 2), Cells(lastRow, lastColumn)))
You already have all the information you need to calculate it with a simple multiplication:
size = (lastRow - 1) * (lastColumn - 1)
Excel is almost certainly doing more work to count it, because a Range
object doesn't have to be rectangular.
3 - You write your array by setting the .Value
of the target Range
to the array, but your reads are still done inside of a loop. Not only that, but you read the same value (Cells(i, j)
) from the worksheet twice here:
If Not IsEmpty(Cells(i, j)) Then
arrayOfBids(counter, 1) = Cells(i, j)
Remember, Cells(r, c)
is a function call (2 actually - see #1) so you should be caching the result if the return value isn't going to change between the calls. I'd read the source Range
into an array also and then iterate over that.
4 - While I'm on that section of code, the block enclosed by...
If Not IsEmpty(Cells(i, j)) Then
'...
End If
...should be indented another level.
5 - IsEmpty
probably isn't the best choice (see this review) - you really just need to test that the cell isn't equal to vbNullString
. Or if a zero bid is invalid, test that the cell isn't equal to 0.
6 - temporaryArray
is basically being used as a tuple to store the swap values in your BubbleSort
implementation. However, when you store them in an array you carry a bunch of SafeArray overhead along with it, especially since it's curiously a two dimensional array with the first dimension fixed at 1 element. A one dimensional array would be better (the runtime performs a multiply internally every time you address a cell in a 2D array). Even better would be to create a simple type to store them in:
Private Type Tuple
ItemOne As Variant
ItemTwo As Variant
End Type
7 - One small advantage that a bubble sort has is that it sorts the array in place. But..., your implementation takes the array ByVal
and then just returns a copy of the sorted array when it's done. This is a huge performance hit that negates the advantage of an in-place sort. Think of it in terms of memory copies. The call sortedBids = BubbleSort(arrayOfBids)
creates a copy of sortedBids
in memory and then passes it to BubbleSort
. The BubbleSort
function then sorts the copy in place. But the copy is just a locally scoped variable, so finally you call BubbleSort = arrayOfBids
which copies it again and puts it on the stack as the return value. Take advantage of possible the sole benefit of your algorithm and convert BubbleSort
to a Sub
that takes the array ByRef
.
8 - Your counter i
should be declared as a Long
in BubbleSort
. I'd actually consider this a bug, because the variables you use to size the array you pass it are Long
. Speaking of which, you also need overflow protection in GetBids()
. WorksheetFunction.Count
returns a Double
which it has to because the total number of possible cells in a Range
is 17,179,869,184 (the maximum possible return value in your code is 17,178,804,225). This will easily overflow a Long
. Granted, you won't be able to write the results if it overflows (and I shudder to think of the execution time for a bubble sort with 1,048,575 elements), but a graceful exit would be nicer than an unhandled exception.
9 - The return value of UBound(arrayOfBids)
will never change inside your Do
loop in BubbleSort
. This value should also be cached outside of the loop - there isn't a need for repeated function calls to determine what is essentially a static value.
10 - You should swap the order of your data collection loops. Based on the sample data, the values on each row should always be increasing from left to right. This indicates that the final placement of an element in the sorted output is more strongly correlated by which column it's in instead of which row it's in. But when you're building the arrayOfBids
that is eventually going to be sorted, you're building it in row order then column order. While this doesn't make as much of a difference with a bubble sort as it would with other sort algorithms, you are basically ensuring that your input to the sort will be closer to the worst case than it will to the best case. If you switch the loop order to this...
For j = 2 To lastColumn
For i = 2 To lastRow
Next
Next
...you are using this characteristic of your typical data set to make sure your sort input is closer to best case.