Chart_GetColor can be improved

[RaymondWise]

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.

[byronwall]

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.

[RaymondWise]

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.