Chart_GetColor can be improved
RaymondWise opened this issue · 2 comments
In module chart_helpers, the function chart_getcolor currently passes an unrestricted index by reference.
Index should be passed byval.
Index can only be between 1 and 10, but that is not error checked
colors() is created every time the function is run while it's essentially a constant
It might be better to create a public enum with an Else
for handling out of bounds index. I haven't figured out why index is a variant though.
I agree that this function leaves something to be desired.
Switching to ByVal is good.
There should be some error checking on the index. If nothing else, limiting the value if the index is out of bounds or throwing an error. I think it could go out of bounds as written with no error checking.
I agree that colors()
could (should?) be defined as a global variable and then only created once. It's a waste to create the thing every time a color is needed, but at the same time, it's a negligible waste given how infrequently the colors are used. I have always avoided doing global variables, but this array might be a good spot to do it.
We could also create a new class (butlConstants or such) and then a single global instance of that class. This avoids having several global variables if there are more of these things and also contains all the globals in one clean spot. If we introduce settings or anything else that might need a global scope, it would be good to catch all these things at once.
This is not included in #61
An enum changes the argument too much for me and it seems this helper is called in 2 or 3 different places. I came up with this
Public Function Chart_GetColor(ByVal index As Variant) As Long
'---------------------------------------------------------------------------------------
' Procedure : Chart_GetColor
' Author : @byronwall
' Date : 2015 07 24
' Purpose : Returns a list of colors for styling chart series
'---------------------------------------------------------------------------------------
'
Select Case index
Case 1
Chart_GetColor = RGB(31, 120, 180)
Case 2
Chart_GetColor = RGB(227, 26, 28)
Case 3
Chart_GetColor = RGB(51, 160, 44)
Case 4
Chart_GetColor = RGB(255, 127, 0)
Case 5
Chart_GetColor = RGB(106, 61, 154)
Case 6
Chart_GetColor = RGB(166, 206, 227)
Case 7
Chart_GetColor = RGB(178, 223, 138)
Case 8
Chart_GetColor = RGB(251, 154, 153)
Case 9
Chart_GetColor = RGB(253, 191, 111)
Case 10
Chart_GetColor = RGB(202, 178, 214)
Case Else
MsgBox "Invalid color index input"
Chart_GetColor = 0
End Select
End Function
This gets rid of the overhead of creating the array and populating the array. But because the RGB
function is a function, we can't use constants. Now I think Case Else
will give us black, so maybe that's not the best Else
argument, but there would need to be error checking everywhere this function is called to handle that.