Delegates to save actions : bad idea ?

Hello, I tried something that I never did before, but I finally came to think that could be a bad idea.

Let’s have an example :
My characters can attack, defend others etc. These are basic actions for characters but I wanted to be able to modify them easily. So I decided to make a delegate for each of them instead of a method.
I have a delegate attack, a delegate defense, and others. I write them in my character’s constructor and I can modify them later.
It worked fine in my tests, and all of them (around 20) were positive, but I ran into errors in play (like NullPointerException on this.attribute within the delegate method).
I searched for hours but didn’t found anything, or even why it worked well sometimes and sometimes not.

Is my way of doing wrong ? I’d like to correct my errors without rewrite all of my code, but if that’s not possible, how can I have easily alterable actions without using overriding ?

(if I made mistakes, that’s because I’m french :stuck_out_tongue: baguette)

Can you show code or an example of what you’re doing to create these errors?

I use Actions (delegates) pretty frequently with no problem.

My uses of these delegates are pretty complex, and I didn’t really found the logic behind the bug.
But I use this kind of code for my characters :

//Yes, that's the constructor
public Constructor () {

    //other code

    this.defense = new System.Predicate<Tile> ((obj) => this.card.tile.Before (obj) && this.card.tile.IsAdjacentTo (obj));
}

public System.Predicate<Tile> defense { get; set; }

public bool Defends (CharacterData other) {

    return this.card != null && this.card.onBoard && other.card != null && other.card.onBoard && other.card != this.card && this.defense (other.card.tile) && !other.defense (this.card.tile);
}

this examples uses Predicates and not custom delegates but that’s the same.
In my tests, all goes well.
But in play, when I call the method Defends() and this.defense (other.card.tile) is called, this.card is null in the predicate and I have a NullPointerException. But I checked it was not in the method Defends(). Does this means something else in delegates ?

OK, so first off I want to clean up some language here.

A delegate is the container of a method pointer. There is the delegate type (System.Predicate for example), and the delegate object (the actual object that houses the methods pointed to).

System.Action act = SomeMethod;

the object ‘act’ is the delegate, it will call the ‘named method’ ‘SomeMethod’.

System.Action act = () => {
    Debug.Log("TEST");
};

In this scenario act is a delegate of type System.Action, it is pointing at an ‘anonymous method’ as opposed to a ‘named method’. When using the ‘() =>’ syntax, these are also called ‘lamdas’, which are just a style of ‘anonymous method’.

When searching for stuff on how anonymous methods/lamdas act, you will want to use the appropriate vocabulary.

With that said.

How anonymous methods actually work is 1 of 2 things happen. Either:

  1. if it requires no state information to act, a named method is declared on the class/struct that you’ve declared the anonymous method in, and its given a very bizarre name like ‘__1_0’. The method is also flagged as not being included as part of the class/structs signature. But really… it’s just a named method that you don’t know the name of.

In this scenario, ‘this’ means the same thing as if you had declared the named method yourself.

  1. if state information is required (example, you access a variable scoped to the method inwhich the anon method was declared… state information will be needed to house that variable for later use since it’ll cease to exist when the method finishes and popped off the operating stack). In this case a nested class with a weird name is created with the anon method declared, and the fields needed to maintain that state. If ‘this’ is accessed, this will include a field for ‘this’. Basically… ‘this’ becomes an actual variable on the class created to represent this method.

In this scenario, ‘this’ is effectively the same thing as if you had declared a named method in the same scope. You’re really accessing a variable named ‘this’, but it works out the same (and actually the variable named is jarbled a little, but you don’t see this… syntax sugar and all).

So, when you call ‘this’ in the method, it will refer to the object within which the scope ‘this’ is meaningful. If you declare the anonymous method in the scope of a script, in a instance member method, ‘this’ will be that instance.

public class zTest01 : MonoBehaviour
{
  
    private System.Action _test;

    private void Start()
    {
        _test = () =>
        {
            Debug.Log(this.name);
        };
    }
  
}

this refers to the instance of zTest01 that has had Start called on it.

public static class SomeStaticClass
{

    public static System.Action GetAction()
    {
        return () =>
        {
            Debug.Log(this.name);
        };
    }

}

this will not work, since this does not make sense in this scope.

And even more interestingly, due to the limitations of how it all works under the hood. This:

public struct SomeStruct
{

    public string name;
    public System.Action Action;

    public SomeStruct(string nm)
    {
        this.name = nm;
        this.Action = () =>
        {
            Debug.Log(this.name);
        };
    }

}

Should result in a compiler error saying that lamda expressions can not access ‘this’ of a struct.

If you want a break down of the actual IL… open the spoiler:

Give you an idea… here is an example where it just becomes a named method:

    public class SomeClass
    {

        public string name;

        public System.Action GetFoo()
        {
            return () =>
            {
                Console.WriteLine(this.name);
            };
        }

    }

Becomes this IL:

.class public auto ansi beforefieldinit
  Console01.SomeClass
    extends [mscorlib]System.Object
{

  .field public string name

  .method public hidebysig instance class [mscorlib]System.Action
    GetFoo() cil managed
  {
    .maxstack 2
    .locals init (
      [0] class [mscorlib]System.Action V_0
    )

    // [15 9 - 15 10]
    IL_0000: nop         

    // [16 13 - 19 15]
    IL_0001: ldarg.0      // this
    IL_0002: ldftn        instance void Console01.SomeClass::'<GetFoo>b__1_0'()
    IL_0008: newobj       instance void [mscorlib]System.Action::.ctor(object, native int)
    IL_000d: stloc.0      // V_0
    IL_000e: br.s         IL_0010

    // [20 9 - 20 10]
    IL_0010: ldloc.0      // V_0
    IL_0011: ret         

  } // end of method SomeClass::GetFoo

  .method public hidebysig specialname rtspecialname instance void
    .ctor() cil managed
  {
    .maxstack 8

    IL_0000: ldarg.0      // this
    IL_0001: call         instance void [mscorlib]System.Object::.ctor()
    IL_0006: nop         
    IL_0007: ret         

  } // end of method SomeClass::.ctor

  .method private hidebysig instance void
    '<GetFoo>b__1_0'() cil managed
  {
    .custom instance void [mscorlib]System.Runtime.CompilerServices.CompilerGeneratedAttribute::.ctor()
      = (01 00 00 00 )
    .maxstack 8

    // [17 13 - 17 14]
    IL_0000: nop         

    // [18 17 - 18 46]
    IL_0001: ldarg.0      // this
    IL_0002: ldfld        string Console01.SomeClass::name
    IL_0007: call         void [mscorlib]System.Console::WriteLine(string)
    IL_000c: nop         

    // [19 13 - 19 14]
    IL_000d: ret         

  } // end of method SomeClass::'<GetFoo>b__1_0'
} // end of class Console01.SomeClass

Note how we just get a named method called ‘b__1_0’. That’s all that really happens.

BUT, if we access stateful information, then we need more. For example here we access a variable local to the GetFoo method called extra:

    public class SomeOtherClass
    {

        public string name;

        public System.Action GetFoo(string extra)
        {
            return () =>
            {
                Console.WriteLine(this.name + extra);
            };
        }
    }

The result is a nested class in the IL:

.class public auto ansi beforefieldinit
  Console01.SomeOtherClass
    extends [mscorlib]System.Object
{

  .class nested private sealed auto ansi beforefieldinit
    '<>c__DisplayClass1_0'
      extends [mscorlib]System.Object
  {
    .custom instance void [mscorlib]System.Runtime.CompilerServices.CompilerGeneratedAttribute::.ctor()
      = (01 00 00 00 )

    .field public string extra

    .field public class Console01.SomeOtherClass '<>4__this'

    .method public hidebysig specialname rtspecialname instance void
      .ctor() cil managed
    {
      .maxstack 8

      IL_0000: ldarg.0      // this
      IL_0001: call         instance void [mscorlib]System.Object::.ctor()
      IL_0006: nop         
      IL_0007: ret         

    } // end of method '<>c__DisplayClass1_0'::.ctor

    .method assembly hidebysig instance void
      '<GetFoo>b__0'() cil managed
    {
      .maxstack 8

      // [32 13 - 32 14]
      IL_0000: nop         

      // [33 17 - 33 54]
      IL_0001: ldarg.0      // this
      IL_0002: ldfld        class Console01.SomeOtherClass Console01.SomeOtherClass/'<>c__DisplayClass1_0'::'<>4__this'
      IL_0007: ldfld        string Console01.SomeOtherClass::name
      IL_000c: ldarg.0      // this
      IL_000d: ldfld        string Console01.SomeOtherClass/'<>c__DisplayClass1_0'::extra
      IL_0012: call         string [mscorlib]System.String::Concat(string, string)
      IL_0017: call         void [mscorlib]System.Console::WriteLine(string)
      IL_001c: nop         

      // [34 13 - 34 14]
      IL_001d: ret         

    } // end of method '<>c__DisplayClass1_0'::'<GetFoo>b__0'
  } // end of class '<>c__DisplayClass1_0'

  .field public string name

  .method public hidebysig instance class [mscorlib]System.Action
    GetFoo(
      string extra
    ) cil managed
  {
    .maxstack 2
    .locals init (
      [0] class Console01.SomeOtherClass/'<>c__DisplayClass1_0' 'CS$<>8__locals0',
      [1] class [mscorlib]System.Action V_1
    )

    IL_0000: newobj       instance void Console01.SomeOtherClass/'<>c__DisplayClass1_0'::.ctor()
    IL_0005: stloc.0      // 'CS$<>8__locals0'
    IL_0006: ldloc.0      // 'CS$<>8__locals0'
    IL_0007: ldarg.0      // this
    IL_0008: stfld        class Console01.SomeOtherClass Console01.SomeOtherClass/'<>c__DisplayClass1_0'::'<>4__this'
    IL_000d: ldloc.0      // 'CS$<>8__locals0'
    IL_000e: ldarg.1      // extra
    IL_000f: stfld        string Console01.SomeOtherClass/'<>c__DisplayClass1_0'::extra

    // [30 9 - 30 10]
    IL_0014: nop         

    // [31 13 - 34 15]
    IL_0015: ldloc.0      // 'CS$<>8__locals0'
    IL_0016: ldftn        instance void Console01.SomeOtherClass/'<>c__DisplayClass1_0'::'<GetFoo>b__0'()
    IL_001c: newobj       instance void [mscorlib]System.Action::.ctor(object, native int)
    IL_0021: stloc.1      // V_1
    IL_0022: br.s         IL_0024

    // [35 9 - 35 10]
    IL_0024: ldloc.1      // V_1
    IL_0025: ret         

  } // end of method SomeOtherClass::GetFoo

  .method public hidebysig specialname rtspecialname instance void
    .ctor() cil managed
  {
    .maxstack 8

    IL_0000: ldarg.0      // this
    IL_0001: call         instance void [mscorlib]System.Object::.ctor()
    IL_0006: nop         
    IL_0007: ret         

  } // end of method SomeOtherClass::.ctor
} // end of class Console01.SomeOtherClass

Note the nested class immediately defined after declaring ‘SomeOtherClass’. It too is given the weird name: ‘<>c__DisplayClass1_0’. And it has the field ‘<>4__this’ defined, which is our reference for ‘this’.

Now… with all that said, this effectively acts the same as if it were a named method.

So… this is pointing at specifically what we assume it should.

I’m not sure why your card field is coming up null. It might very well be null.

I do notice that your ‘defense’ property is a public getter/setter. Which makes sense, you may very well want to change the behaviour of ‘defense’ for a given instance of this object. Thing is, if you defined the anonymous method that gets set to this property in the scope of another object… ‘this’ is referring to that OTHER object, not the object on which you’re setting it.

This can be confusing say you do this:

    public class FooClass
    {

        public string Name;
        public System.Action SomeAction
        {
            get;
            set;
        }

        public void DoStuff(FooClass other)
        {
            other.SomeAction = () =>
            {
                Console.WriteLine(this.Name);
            };
        }

    }

in this scenario ‘this’ refers to the FooClass that had DoStuff called on it, NOT the FooClass passed in as ‘other’.

2 Likes

While I was typing this post, I see lordofduct has made a comprehensive post, so I’ll just leave this StackOverflow link which has several people discussing a similar issue :slight_smile:

(This may or may not be relevant to what you’re seeing, but be very careful with scoping issues and constructors)

1 Like

Ok, I see. That’s a big novel to say that but at least I understood very well. I’m very thankful, lordofduck, chiefly that you already helped me some days ago. :smile:

I’m pretty glad that I don’t have to rewrite all of my code, I’ll do my best to correct that. But the most I use them, the most I think delegates aren’t for clean code.

Now I found why it failed ingame but not in the tests.

Problem was in Clone(). I cloned the delegates too, referencing the bad character.
Is there any way to clone these actions ?

How was the original action created in the first place? Was it done via the constructor as mentioned above? In that case, wouldn’t the cloned object’s constructor also have constructed the delegate referencing to itself?

The actions are created in the constructor but could me modified after that.
That’s why I wanted to copy them in Clone(), elsewhise my copied object would only have the basic action as depicted in the constructor, even if the copied object hasn’t.
By setting these lines in comments in my Clone(), my game runs well, but I’m not able to modify these actions and then clone the object.

I’ve read that copying delegates by changing the this reference was impossible. How could I avoid this problem, please ?
I’ve thought of custom delegates with one extra parameter referencing to the current object instead of directly using this, but that sounds a bit weird to me…

Yeah, the “this” pointer is pretty much captured at that point, and even if it was possible to change the semantics of what “this” refers to, I’m not sure you really want to do that because you end up looking at code and being unsure what you’re looking at.

I think it’d be better for sanity’s sake to be more explicit, and pass in both the attacker and defender in the arguments. That way, there’s no room for doubt when reading the code in, say, 6 months from now.