C#, ArrayList Property to List field

Question

Hello, I have the following Field:

private IList<Modulo> modulos;

and the following Property:

    public virtual ArrayList XmlModulos
    {
        get
        {
            if (modulos == null) return new ArrayList();
            var aux = new ArrayList();
            foreach (Modulo m in modulos)
                aux.Add(m);
            return aux;
        }
        set
        {
            modulos = new List<Modulo>();
            foreach (object o in value)
                modulos.Add((Modulo)o);
        }

    }

The get works perfectly, as I have another direct property that is used for different purposes. But the set is not working, as when I use this property it doesn't update anything. The debug says that the Property is an empty ArrayList (count 0) and the field stays in null. Any Idea why this is not working? any help will be highly appreciated.

Answer 1

I cannot reproduce your issue; I copied your code and then executed this line:

XmlModulos = new ArrayList { new Modulo() };

After this, both the debugger and Console.WriteLine report both modulos.Count and XmlModulos.Count as 1.

Therefore, the error lies in some other code that you haven’t shown us.

Answer 2

To understand what's going on here, you must realize that .NET properties are simply a convenient way to represent two separate methods, one for getting and another for setting.

That is, they basically map these operations:

ArrayList m = obj.XmlModulos;

obj.XmlModulos = new ArrayList();

to code that really more closely resembles this:

ArrayList m = obj.get_XmlModulos();

obj.set_XmlModulos(new ArrayList());

OK, so with this in mind, let's consider what you're trying to do (this is based on a comment to Timwi's answer):

obj.XmlModulos.Add(new Modulo());

What this is really doing is:

obj.get_XmlModulos().Add(new Modulo());

Considering your get accessor creates a new ArrayList, you're never going to be able to add a Modulo to your collection in this way. The only way you could do it would be this:

ArrayList m = obj.XmlModulos;

m.Add(new Modulo());

obj.XmlModulos = m;

HOWEVER, this is a very inefficient way to implement your get and set accessors. Creating a new ArrayList on every get and populating it with the current members of modulos is costly. So is enumerating over any value passed to set and copying each to modulos.

If anything, this class design is backwards (really, most developers would advise steering clear of the ArrayList type altogether these days; but if you insist, that's your call). You are storing an IList<Modulo> (which provides type safety) internally but exposing an ArrayList (which does not) externally; what this means is that someone could easily try to add some random object of non-Modulo type to your list and you would get an InvalidCastException. Code that tries to do this really ought not to compile at all; that's the whole point of generics.

OK, I've said enough. What do I suggest? Have just one collection belonging to this class. Decide what functionality you actually want to provide, and expose just that functionality. Don't do this expensive copying back and forth between two collections.

For example, if you truly need an ArrayList, but you only need to enable client code to add and remove elements to/from it, then implement it like this:

private ArrayList modulos;

public void AddModulo(Modulo m)
{
    modulos.Add(m);
}

public bool RemoveModulo(Modulo m)
{
    return modulos.Remove(m);
}

This way, even though you're using a non-generic ArrayList internally (if you have good reasons for this), you're still getting the effective type safety of an IList<T> by only allowing client code to add and remove items of the Modulo type.

This could be very different from the solution that actually makes sense for you. Let it hopefully steer you in the right direction, at least.