Problem
This feels like a simple question, but I’m wondering what the best way would be to manage the elements of a private, fixed-size array.
Context: I’m making a simple particle engine, and my Emitter class will smoothly interpolate between up to 8 colors per Particle, depending on the Particle’s Life value. I have that part implemented and it works great. However, my question is more concerning the Color[] array itself.
Suppose I have the following code. What I would like to do is be able to alter the 8 values in a simple and logical way:
public sealed class Emitter
{
private readonly Color[] _colors = new Color[8];
}
One option would be to return the whole array, but according to CA1819, a property should not return an array, since it warns the array is mutable unless a copy is made. However, this is the behavior I want, so it might make sense to do it:
public Color[] Colors
{
get { return _colors; }
}
Another option would be to do a simple get/set and pass a desired index as a parameter; this has the benefit of checking for success, but is more cumbersome to use because of the out parameter. In addition, there’s nothing that explicitly states the min/max values of the color array, which means I’d have to have a const byte MaxColors = 8; or the like:
public bool GetColor(int id, out Color result)
{
if (id >= 0 && id < 8)
{
// Valid index, return the color at that position
result = _colors[id];
return true;
}
// Invalid index, return the default color (R:0, G:0, B:0, A:0)
result = default(Color);
return false;
}
public void SetColor(int id, Color value)
{
if (id >= 0 && id < 8)
{
_colors[id] = value;
}
}
public void SetColor(param Color[] values)
{
// Set all 8 colors, using default values if not enough arguments are supplied
if(values != null)
for(int i = 0; i < 8; i++)
if (i > values.Length)
_colors[i] = default(Color);
else
_colors[i] = values[i];
}
Finally, I could do the brute force approach and make a property for each color value, and I probably don’t need to explain why that’s a bad idea:
public Color Color1
{
get { return _colors[0]; }
set { _colors[0] = value; }
}
...
public Color Color8
{
get { return _colors[7]; }
set { _colors[7] = value; }
}
So… yeah. Based on my needs, which approach seems to make the most sense? I’m leaning towards the getter/setter pattern. Is there another way I haven’t considered that would be desireable in this situation?
Solution
If you expose it via a public property then it’s not a “private” array any more:it’s part of the public interface.
If resetting its elements is something that you want clients to do, I’d choose the first option (because it’s simplest).
Or, these …
public void SetColor(int id, Color value) { ... }
public bool GetColor(int id, out Color result) { ... }
… could perhaps be done using indexers …
public Color this[int id]
{
get { ... }
set { ... }
}
… but that’s not very different from exposing the array (and there can be only one indexer of a given signature, which doesn’t work if there’s more than one array to be exposed).
Here’s a blog post on the topic: http://blogs.msdn.com/b/ericlippert/archive/2008/09/22/arrays-considered-somewhat-harmful.aspx
Another possibility (I’ve just thought of it, haven’t tested/researched this idea) might be to pass the array into the constructor:
public sealed class Emitter
{
private readonly Color[] _colors;
public Emitter(Color[] colors) { _colors = colors; }
}
The reason excuse for that is that so that whoever constructs Emitter has the array, can keep that array, and can (evilly) modify that array after it has used it to construct Emitter.
Color[] colors = new Color[8];
Emitter emitter = new Emitter(colors);
colors[0] = Color.Red; // changes constents of array inside the Emitter
The official recommendation is to use a List<Color> instead of a Color[] array, and return IList<Color> … conceptually similar IMO, but you won’t get the compiler and FxCop warnings.
public sealed class Emitter
{
private readonly List<Color> _colors;
public IList<Color> Colors { get { return _colors; } }
}
As @svick noted in a comment below, because arrays implement the IList<T> interface, you can also do this:
public sealed class Emitter
{
private readonly Color[] _colors;
public IList<Color> Colors { get { return _colors; } }
}
IList<T> supports the API you want (i.e. an indexer property). Using IList<T> supresses the compiler warning. It may be less intuitive to the client/user though (who may think, incorrectly, that the IList<T> would allow them to change its size).
There is a very good alternative
using System;
using System.Collections.Generic;
using System.Drawing;
using System.Linq;
using System.Net;
using System.Net.Sockets;
using System.Text;
using System.Threading.Tasks;
namespace Server
{
internal class Program
{
private static void Main(string[] args)
{
var emitter = new Emitter();
emitter.Colors[Emitter.Color.Color1] = Color.FromArgb(255, 255, 0, 0);
Console.WriteLine(emitter.Colors[Emitter.Color.Color1]);
//Color[] color = ((Emitter.EmitterColor) emitter.Colors).Colors;
//Console.WriteLine(color[0]);
}
}
public class Emitter
{
public enum Color : int
{
Color1,
Color2,
Color3,
Color4,
Color5,
Color6,
Color7,
Color8
}
readonly EmitterColor _emitterColor;
public Emitter()
{
_emitterColor = new EmitterColor(new System.Drawing.Color[Enum.GetValues(typeof(Color)).Length]);
}
public IEmitterColor Colors
{
get { return _emitterColor; }
}
sealed class EmitterColor : IEmitterColor
{
private readonly System.Drawing.Color[] _colors;
public EmitterColor(System.Drawing.Color[] colors)
{
_colors = colors;
}
public System.Drawing.Color this[Emitter.Color color]
{
get { return _colors[(int)color]; }
set { _colors[(int)color] = value; }
}
public System.Drawing.Color[] Colors
{
get { return _colors; }
}
}
}
public interface IEmitterColor
{
Color this[Emitter.Color colorIndex] { get; set; }
}
}
as a bonus, by changin accessor to internal
internal sealed class EmitterColor : IEmitterColor
{
...
}
you can always cast as follows, to access internals:
Color[] color = ((Emitter.EmitterColor) emitter.Colors).Colors;
Console.WriteLine(color[0]);