Ayende @ Rahien

It's a girl

Explain this code: Answers

The reason this code is useful?

image

Because it allows you to write this sort of code:

class Program
{
    private static Collection<int> nums;

    static void Main(string[] args)
    {
        nums.Add(1);
        Console.WriteLine(nums.Count);
    }
}

I was aiming for that, and still this code strikes me as wrong.

Comments

Buildstarted
04/13/2012 09:07 AM by
Buildstarted

The fact that it's missing an initializer for nums?

Richard
04/13/2012 09:22 AM by
Richard

Structs don't require an initialiser as they always have a constructor that will initialise all its fields. The above code is an abuse of this mechanic.

Thomas Levesque
04/13/2012 09:33 AM by
Thomas Levesque

@Richard, it will initialize all fields to their default value, which is null in the case of IList, so you still have to initialize inner when needed (you can't do it in the constructor, since you can't define a default constructor for a struct)

Mads Topro
04/13/2012 09:36 AM by
Mads Topro

@Thomas: Maybe, Add method is initializing the inner field before use it.

Thomas Levesque
04/13/2012 09:39 AM by
Thomas Levesque

@Mads, yes, this is what I had in mind ;)

Daniel Grunwald
04/13/2012 11:00 AM by
Daniel Grunwald

Ouch, so if I copy the struct after it's initialized, the underlying list will be shared, but if I copy it before, then each copy will create its own underlying list?

Mutable structs are very bad, especially if they behave like reference types in some aspects, but not in others. To avoid this issue, the default value should be a read-only empty list - so the struct itself will be immutable, only the underlying list (if any) can be modified. That would make the struct act just like a non-nullable reference type on the outside. You'll get an exception when adding to an uninitialized struct, but it still solves the problem of null references for pure read access (which tend to be much more common than write access).

Daniel Grunwald
04/13/2012 11:05 AM by
Daniel Grunwald

To elaborate on why such a mutable struct is wrong:

Replace your private static field with a static automatic property, and your example program will print 0. (as it mutates the properties' return value, not the actual underlying field)

This is a massive pitfall! A NotSupportedException (uninitialized Collection is readonly) would be much better.

Dmitry
04/13/2012 11:30 AM by
Dmitry

Why is it a problem to call the default constructor?

Andres
04/13/2012 03:00 PM by
Andres

WTF!

Scooletz
04/13/2012 07:34 PM by
Scooletz

This code is just wrong. I can imagine a plenty of people reading it and then trying to figure out what's going on. After 15 minutes they would go to the summary of the Collection class, oops! struct and read "Because it allows you to write this sort of code:". Then a facepalm would occur.

Ayende Rahien
04/14/2012 12:16 AM by
Ayende Rahien

Daniel, Actually, no, it would still work. The internal list is mutated, not the actual struct.

Daniel Grunwald
04/14/2012 10:28 AM by
Daniel Grunwald

@Ayende Try it, I'm fairly sure your example program will break with an automatic property instead of a field. The actual struct is mutated when the internal list is created on the first Add() call. Your struct behaves like a value type before initialization, and like a reference type after. With an automatic property, there's no way to initialize the underlying field; only the properties' return value gets initialized - so in effect every access to the property returns a new collection.

Andres
04/14/2012 07:06 PM by
Andres

April fools?

SergeyT.
04/14/2012 08:01 PM by
SergeyT.

@Daniel, @Ayende: actually there is even more subtle way to break this code.

Try to add readonly modifier to the "nums" field and run this sample one more time.

Karep
04/14/2012 09:26 PM by
Karep

WTF? What i don't understand. What did you get by writing this class? That that you don't have to 'new' Collection?

Gerke Geurts
04/16/2012 06:54 AM by
Gerke Geurts

Why not be upfront about being immutable and use the following signature instead: public Collection Add(T other)?

Clay Ver Valen
04/18/2012 11:00 PM by
Clay Ver Valen

You'll have to ensure inner has been initialized before every public method and get_*. Not that you can't, but not very elegant either.

Also, make sure to NOT to initialize it to a new instance of "Collection" or you'll get a StackOverflowException. Instead fully qualify it (e.g. System.Collections.ObjectModel.Collection() ) or use List, etc.

Fabian Schmied
04/20/2012 07:00 AM by
Fabian Schmied

@Ayende: I don't understand the purpose, even after your explanation. Are you saying you did this because you needed to define a static field holding an initialized list, but couldn't use a field initializer?

If so, why couldn't you use a field initializer?

Ayende Rahien
04/21/2012 09:15 AM by
Ayende Rahien

Fabian, The idea is that you have a "never null" collection.

Don't think just about this scenario, think about how useful it is when you KNOW that a collection can never be null.

Neil Mosafi
04/22/2012 04:42 PM by
Neil Mosafi

But the inner list can be null (and will be) so you gain very little apart from a bit of added indirection and possibly a source of some bugs (every property and method will have to lazy initialise the inner list or something similar). Better to initialise a private readonly field if you want to guarantee non nullness.

Ayende Rahien
04/22/2012 05:28 PM by
Ayende Rahien

Neil, The inner prop will be null, sure. But I can do the lazy init in a single class, not in all places.

Neil Mosafi
04/22/2012 08:02 PM by
Neil Mosafi

Sure, but I don't think the point of this post is to discuss building a lazy wrapper for a list. I will wait and see cos I'm not sure what it's about now!

Comments have been closed on this topic.