[RESOLVED] What am I missing in my Custom Event?

Hello, I use custom events all the time but with the following piece of code I am not sure what is wrong. I am not able to Add an event handler to an instance of this class.

Code:

---

using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Data;
using System.Drawing;
using System.Text;
using System.Windows.Forms;

namespace CustomNotepad2
{
    public partial class frmGoTo : Form
    {
        public frmEditor FormEditor = new frmEditor();
        public delegate void GoToEventHandler(object sender, GoToEventArgs e);
        public GoToEventHandler GoToEvent;
        public frmGoTo()
        {
            InitializeComponent();
        }

        private void btnGoTo_Click(object sender, EventArgs e)
        {
            OnGoTo(new GoToEventArgs(int.Parse(txtLineNumber.Text.Trim())));
        }

        private void btnCancel_Click(object sender, EventArgs e)
        {
            this.Close();
        }
        protected virtual void OnGoTo(GoToEventArgs e)
        {
            if (GoToEvent != null)
            {
                GoToEvent(this, e);
            }
        }
    }
 
    public class GoToEventArgs : EventArgs
    {
        int _line = 0;
        public GoToEventArgs(int line)
        {
            _line = line;
        }
        public int GoToLine
        {
            get
            {
                return _line;
            }
        }
    }
}

---

you didn't set "GoToEvent" as event
replce this:

Code:

---

  public event GoToEventHandler GoToEvent;

---

with this

Code:

---

  public event GoToEventHandler GoToEvent;

---

also it's consider bad practice to set your delegate as public, when you wish to create an event
use the built in EventHandler delegate, which will save you a lot of work protecting your delegates

Code:

---

public event EventHandler<GoToEventArgs> GoToEvent;

---

Just out of curiosity, what is the issue with having public delegates?

What motil is suggesting should work and might be the best way to go, but the problem in your code is that you have declared the delegate inside the class.
That way it isn't accessible when hooking the event from the outside.
The suggestion by motil is that instead of a delegate, you use a generic EventHandler as event handler that is publicly accessible already.

I think the OP was right declaring the event inside the class since the purpose of this event is to raise events only related to this class.

and the reason you shouldn't declare delegate as public is because it's break the rule of encapsulation and even worse once you declared your delegate as public it gives the caller the power of invoking this delegate whenever he please, in some application it could be huge security hole.

So this line:

Code:

---

public event EventHandler<GoToEventArgs> GoToEvent;

---

Would replace both these lines?

Code:

---

public delegate void GoToEventHandler(object sender, GoToEventArgs e);
public GoToEventHandler GoToEvent;

---

Or is that just for the Event?

Both lines

Thanks Resolved :check:

Quote:

---

Originally Posted by motil

even worse once you declared your delegate as public it gives the caller the power of invoking this delegate whenever he please, in some application it could be huge security hole.

---

Er, no. The delegate declaration simply defines the signature. You need an instance of the delegate to actually execute. frmGoTo.GoToEventHandler isn't anything that you can execute.

I made a very simple example for what i meant, let's say you have an AirCraft class with two events:
1) AirCraftCreatedDelegate which you declare as public.
2) AirCraftCrashedDelegate which you create by using the generic EventHandler event

because you declared AirCraftCrashedDelegate as public the caller can invoke it however he likes.. look

AirCraft Class

Code:

---

class AirCraft
    {
        public delegate void AirCraftCreatedDelegate(string message);
        public AirCraftCreatedDelegate AirCraftCreated;

        public event EventHandler<AirCraftCrashedEventArgs> AirCraftCrashedDelegate;


        public void StartEngines()
        {
            if (this.AirCraftCreated != null)
                this.AirCraftCreated.Invoke("Air craft created!");
           
        }

        public void AirCraftCrashed()
        {
            if (this.AirCraftCrashedDelegate != null)
                this.AirCraftCrashedDelegate(this, new AirCraftCrashedEventArgs("Boom"));
        }


    }

    public class AirCraftCrashedEventArgs : EventArgs
    {
        public string message { set; get; }

        public AirCraftCrashedEventArgs(string message)
        {
            this.message = message;
        }
    }

---

Code:

---

namespace WindowsFormsApplication3
{
    public partial class Form1 : Form
    {
        public Form1()
        {
            InitializeComponent();
        }

        private void Form1_Load(object sender, EventArgs e)
        {
            AirCraft airCraft = new AirCraft();
            airCraft.AirCraftCreated += bla;

         
            airCraft.AirCraftCrashedDelegate += new EventHandler<AirCraftCrashedEventArgs>(airCraft_AirCraftCrashedDelegate);

            // using it like you should
            airCraft.StartEngines();

            // AirCraftCrashedEventArgs is public so we can send whatever messages we want..
            AirCraft.AirCraftCreatedDelegate doWhatIeverIWant = new AirCraft.AirCraftCreatedDelegate(bla);
            doWhatIeverIWant.Invoke("I'm not even an air craft! I'm a monkey!");

            // here is the only way you can invoke AirCraftCrashedEventArgs.
            airCraft.AirCraftCrashed();

        }
       

        private void bla(string message)
        {
            Console.WriteLine(message);
        }
        void airCraft_AirCraftCrashedDelegate(object sender, AirCraftCrashedEventArgs e)
        {
            Console.WriteLine(e.message);
        }
    }
}

---

now i'm sure you don't want your aircraft class reveal its most saved secret that it not really an aircraft but it actually a monkey in disguise :)

Um, let me get this straight.

Aircraft defines a delegate.

Form1 defines a method bla that has the same signature as the delegate and that takes a string and outputs it to the console.
Form1 defines a string literal "I'm not even an air craft! I'm a monkey!".
Form1 defines an instance of the delegate as the method it previously defined.
Form1 executes the instance (that it defined) that invokes the method (that it defined) with the string parameter (that it defined), and yet somehow that's the fault of the Aircraft class? Or some fault with it?

It is not invoking anything in your Aircraft instances. Anyone registered to the events on your Aircraft instances will not be invoked.

You seem to have completely missed the difference between a delegate declaration (analogy: type) and a delegate instance (analogy: object).

Er, Um, TaDam :down:
I don't have the time or the need to proof to you what I already know... do some reading you might learn something.

I said what I had to say take it or leave it.

Wait up, I think I may see where you're coming from. It's not the delegate being public that's the problem, but the instance of that delegate. Is that what you're saying?

In which case, your example shows the problem in the AirCraftCreated member not having the event keyword. This allows calling code to get the instances attached to the "event" and invoke them, like this:

csharp Code:

AirCraft airCraft = new AirCraft();
 
airCraft.AirCraftCreated += message => Console.WriteLine("AirCraft Created: {0}", message);
airCraft.AirCraftCrashedDelegate +=
    (sender, eventargs) => Console.WriteLine("AirCraft Crashed: {0}", eventargs.message);
 
Console.WriteLine("Starting Engines:");
airCraft.StartEngines();
 
Console.WriteLine("Crashing Aircraft:");
airCraft.AirCraftCrashed();
 
Console.WriteLine("Doing naughty things with Aircraft internals");
airCraft.AirCraftCreated("I'm not even an air craft! I'm a monkey!");

But that is, as I said, caused by not declaring the "event" as an event. It's not a problem caused by the delegate being public, since the delegate is only the signature of the method. Think about it: EventHandler<TEventArgs> is public, so what's the difference?

I fear we're talking about completely different things when we use the term "delegate"

Code:

---

EventHandler<TEventArgs> is public

---

something gotta be public to use the event, when you create an event the compiler actually create two more private methods in the background that do the work of hiding and protecting the delegate (instance).

anyhow I might wasn't so clear since my english is not the best but i'm glad you got me.