Have i cornered my self using events? I feel forced to write repeating code but maybe its ok.

Hello devs, let me paint the picture for you. I have an Input script which on key(0-9)press invokes of an event Action(so every key has its own unique event Action). I then have a PlayerAbilites script that subscribes to these events.

So here is the issue, the code that i am writing dose not feel like the correct way to go. The code is repeating and i have been thought its a bad sign.

Code (CSharp):

1. public AbilityIconUI[] AbilityIcons;
2. public class PlayerAbility : MonoBehaviour
3. {  
4.     void Start()
5.     {
6.         GameManager.Instance.PlayerInputManager.OnAbilityKey1Down += OnKeypress1;
7.         GameManager.Instance.PlayerInputManager.OnAbilityKey2Down += OnKeypress2;
8.        //Etc ...
9.     }
10.     void Update()
11.     {
12.         AbilityIcons[0].AbilityOnIcon?.CoolDownImgEffect();
13.         AbilityIcons[1].AbilityOnIcon?.CoolDownImgEffect();
14.         // all the way down to 9 ....
15.     }
16.     private void CastAbility(Ability ability)
17.     {
18.        if(ability != null)
19.        {//Do whatever}        
20.     }
21.     void OnKeypress1()
22.     {
23.         CastAbility(AbilityIcons[0].AbilityOnIcon);
24.     }
25.     void OnKeypress2()
26.     {
27.         CastAbility(AbilityIcons[1].AbilityOnIcon);
28.     }
29.     // all the way down to 9 Keys ....
30. }
31.  
32. public class PlayerInputManager : MonoBehaviour
33. {
34.     public event Action OnAbilityKey1Down;
35.     // all the way down to 9 Keys ....
36.     void Update()
37.     {
38.       abilityInputs(ability1KeyCode, OnAbilityKey1Down);//Press key -> On keycode -> Invoke Action
39.       // Here i dont have an issue with 9 calls because of parameters make this fine IMO
40.     }
41. }
42.  
43.  

I though i had enough basic understating of events and delegates, I was working under certain assumptions when it came to parameters and Evenst(thinking i could pass them the same way i did in the playerInput) and now cornered my self. I feel i am missing something very basic, the solutions(not specifically to my problem) seem to be a little to complicated/hacks/overkill for this.

 

Is the code easy enough for you and your team to understand, maintain, extend? Then it doesnt matter if its the "right" or "wrong" way.

Best thing to do is profile it , if it is running like **** then optimise, otherwise as long as its readable etc its fine

 

I suppose you could use anonymous methods for this if you want.

Code (csharp):

1. void Start()
2. {
3.    // Instead of having different events, introduce an array of events?
4.    for(int i = 0; i < 10; i++)
5.       GameManager.Instance.PlayerInputManager.OnAbilityKeyDown[i] += () => CastAbility(AbilityIcons[i].AbilityOnIcon);
6. }
7.  
8. void Update()
9. {
10.    for(int i = 0; i < 10; i++)
11.       AbilityIcons[i].AbilityOnIcon?.CoolDownImgEffect();
12. }
13.  
14. // No need for each keypress method anymore.

 

I guess it is easy enough to understand, but if i where to do something(audio/effects) to this in the future i would have to do it times 9 . But i guess i could put it in the ability itself.

 

Very cool, i have never looked into anonymous methods. It allows for the parameters to be used, that is was the wall i hit with Actions. Very cool @WallaceT_MFM

 

@WallaceT_MFM real savings was just using the array, which you could have done already. Anytime you find yourself writing repeating code, you just need to look at what you are doing and why. Collections and loops tend to (not always) be a big solution to repeating code, especially like what you were doing.

 

I completely agree, the problem for me was i couldn't find a way to loop the Events. With the solution @WallaceT_MFM
provided the loop would be a natural next step. As for the Update() Cdeffect-> rookie mistake by me but i had a reason for doing it but i cant remember what it was hmm.

 

Be VERY careful using anonymous methods as event handlers.

Since they're anonymous, there's no-way to unscubscribe from the event. You don't have a handle to the event listener method... since it's anonymous.

The only way to remove the listener is for event owner to set its event null.

 

Thank you for that warning. So something like

Code (CSharp):

1. OnDisable()
2. {
3. for(int i = 0; i < 10; i++)
4.       GameManager.Instance.PlayerInputManager.OnAbilityKeyDown[i] = null;
5. }

Or can i store this[ () => CastAbility(AbilityIcons.AbilityOnIcon) ] into a delegate then sub/unsub that ?

 

Well, repeating code is a good way to reduce the maintenance ability. Changes are also often not applied the same to all copies, which can lead to bugs that can be fairly hard to find.

The big problem with removing the repeating code is that it sometimes requires more complicated constructions, which can reduce the understanding part.

You can also do it in a way that is not anonymous by the way:

Code (csharp):

1.  
2. private CastAbilityIndex[] castAbilityIndex;
3.  
4. private void Start()
5. {
6.     castAbilityIndex = new CastAbilityIndex[10];
7.     for(int i = 0; i < 10; i++)
8.     {
9.         castAbilityIndex[i] = new CastAbilityIndex(this, i);
10.         GameManager.Instance.PlayerInputManager.OnAbilityKeyDown[i] += castAbilityIndex[i].Activate;
11.     }
12. }
13.  
14. private void OnDisable()
15. {
16.     for(int i = 0; i < 10; i++)
17.     {
18.         GameManager.Instance.PlayerInputManager.OnAbilityKeyDown[i] -= castAbilityIndex[i].Activate;
19.     }
20. }
21.  
22. internal void Activate(int index)
23. {
24.     CastAbility(AbilityIcons[index].AbilityOnIcon);
25. }
26.  
27. private class CastAbilityIndex
28. {
29.     private PlayerAbility parent;
30.     private int index;
31.  
32.     public CastAbilityIndex(PlayerAbility parent, int index)
33.     {
34.         this.parent = parent;
35.         this.index = index;
36.     }
37.  
38.     public void Activate()
39.     {
40.         parent.Activate(index);
41.     }
42. }
43.  

You could of course also just integrate CastAbilityIndex with AbilityIconUI.

This way is not anonymous, so the delegates can also be removed again. Less repetition, but it might also be less understandable.

 

This is the thing i wanted to avoid, because the way things are set up i dont want to add more complexity to the system. But with warning from lordofduct, i am leaning more towards this solution. It looks like a nice and simple addition. very cool @jvo3dc

 

Of course, but proper naming and comments go a long way here to help others (and your future self) understand how things are set up. I used "CastAbilityIndex" here, but you can probably think of a better name.

I personally always prefer to remove as much code repetition as possible. It improves maintenance and allows you to make changes more quickly.

 

I share your attitude but my inexperience with events and delegates made me think i was doing something wrong with them, that there was some very simple thing that i just didn't understand but i should have looked more to restructure like you did @jvo3dc . By using the concept you provided i moved a method into the AbilityIconUI and it works like a charm.

Thank you everyone for your contribution. I am going to take a closer look at lambda expressions on top of Events and delegates, as the solution my @WallaceT_MFM was very smooth but possibly danegrus as @lordofduct mentions .I just wouldn't have gotten over this hump if it was not for this thread.