Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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?

share|improve this question
add comment

2 Answers

up vote 10 down vote accepted

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).

share|improve this answer
    
Well, exposing it isn't a problem per se; I want to disallow changing the size, and not make it a public variable. But as the article link I provided indicates, properties and arrays shouldn't mix, either. I'm disinclined to use a List<> since the size is mutable, and IEnumerable types don't allow the values to be changed. The values of the colors should be changeable at any time, and the constructor would just add clutter to that (although it could simply make a default Color.White as a placeholder until something better comes along). Based on this, I think using properties is the best way. –  Kyle Baran Jan 15 at 1:45
3  
Array also implements IList<T>, so I don't see any reason to use List<T> here. –  svick Jan 15 at 3:05
add comment

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]);
share|improve this answer
1  
Why is it better? EmitterColor is similar to (but more verbose than) an array; it also doesn't support some possibly-useful interfaces, e.g. IEnumerable. Also, declaring that EmitterColor is sealed doesn't make a difference, as far as I can see: because the type-cast would still be valid even if _emitterColor were a subclass of EmitterColor, yet sealed doesn't ensure that EmitterColor is the only implementation of IEmitterColor. –  ChrisW Jan 15 at 14:11
    
@ChrisW: You can add IEnumerable implementation to the EmitterColor class, it is trivial. I do not get a point about type casting, please provide an example, in case you want to prevent type casting to EmitterColor class, you can declare it private. Cheers –  Artur Mustafin Jan 15 at 14:57
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.