6
\$\begingroup\$

I am trying to learn events and delegates in C#. To do that I am trying to create a hypothetical console project where when some user submits the orders in his/her shopping cart, I need the Billing department and mailing department to handle that event. Can someone please critique this code for doing the same?

using System;
using System.Collections.Generic;

namespace LearnEvents
{
    class Program
    { 
        static void Main(string[] args)
        {
            var shoppingCart = new ShoppingCart(10);//shopping cart for user number 10
            var bd = new BillingDepartment(shoppingCart);
            var md = new MailingDepartment(shoppingCart);
            shoppingCart.Add(75458);
            shoppingCart.Add(54693);
            shoppingCart.Add(52145);
            shoppingCart.Submit();

        } 

    }

    public class ShoppingCart
    {
        private int _userId ;
        private List<int> _orders=new List<int>();
        public delegate void OrderSubmitted(OrderDetails orderDetails);
        public  event OrderSubmitted OrderSubmittedEvent;


        public ShoppingCart(int userId)
        {
            _userId = userId;
        }
        public void Add(int itemNumber)
        {
            _orders.Add(itemNumber);
        }

        public void Submit()
        {
            OrderSubmittedEvent.Invoke(new OrderDetails{ItemCodes = _orders,UserId = _userId});
        }

    }

    public class OrderDetails
    {
        public List<int> ItemCodes { get; set; }
        public int UserId { get; set; } 
    }


    public class BillingDepartment
    {
        public  BillingDepartment(ShoppingCart sc)
        {
             sc.OrderSubmittedEvent+=OrderSubmittedHandler;
        }

        public void OrderSubmittedHandler(OrderDetails orderDetails)
        {
          foreach (var item in orderDetails.ItemCodes)
            {
                Console.WriteLine("Billing user "+orderDetails.UserId+" for the order "+item.ToString());
            }
        }

    }

    public class MailingDepartment
    {

        public MailingDepartment(ShoppingCart sc)
        {
            sc.OrderSubmittedEvent += OrderSubmittedHandler;
        }
        public void OrderSubmittedHandler(OrderDetails orderDetails)
        {
            foreach (var item in orderDetails.ItemCodes)
            {
                Console.WriteLine("Mailing user " + orderDetails.UserId + " the order " + item.ToString());
            }
        }

    }


}

I would also like to know, why do I need an event? Why can't I just use multicast delegates instead as shown below?

using System;
using System.Collections.Generic;

namespace LearnEvents
{
    class Program
    { 
        static void Main(string[] args)
        {
            var shoppingCart = new ShoppingCart(10);//shopping cart for user number 10
            var bd = new BillingDepartment(shoppingCart);
            var md = new MailingDepartment(shoppingCart);
            shoppingCart.Add(75458);
            shoppingCart.Add(54693);
            shoppingCart.Add(52145);
            shoppingCart.Submit();

        } 

    }

    public class ShoppingCart
    {
        private int _userId ;
        private List<int> _orders=new List<int>();
        public delegate void OrderSubmitted(OrderDetails orderDetails);

        public OrderSubmitted os;


        public ShoppingCart(int userId)
        {
            _userId = userId;
        }
        public void Add(int itemNumber)
        {
            _orders.Add(itemNumber);
        }

        public void Submit()
        {
            os.Invoke(new OrderDetails { ItemCodes = _orders, UserId = _userId });
        }

    }

    public class OrderDetails
    {
        public List<int> ItemCodes { get; set; }
        public int UserId { get; set; } 
    }


    public class BillingDepartment
    {
        public  BillingDepartment(ShoppingCart sc)
        {
            sc.os+=OrderSubmittedHandler;  
        }

        public void OrderSubmittedHandler(OrderDetails orderDetails)
        {
          foreach (var item in orderDetails.ItemCodes)
            {
                Console.WriteLine("Billing user "+orderDetails.UserId+" for the order "+item.ToString());
            }
        }

    }

    public class MailingDepartment
    {

        public MailingDepartment(ShoppingCart sc)
        {
            sc.os += OrderSubmittedHandler; 
        }
        public void OrderSubmittedHandler(OrderDetails orderDetails)
        {
            foreach (var item in orderDetails.ItemCodes)
            {
                Console.WriteLine("Mailing user " + orderDetails.UserId + " the order " + item.ToString());
            }
        }

    }


}
\$\endgroup\$
7
\$\begingroup\$

The standard signature for an event-handler delegate is

void HandlerName(object sender, HandlerArgs args)

Where HandlerArgs inherits from System.EventArgs

So to follow this convention, you should create a new class:

 class OrderSubmittedEventArgs : EventArgs
 {
       OrderDetails { get; set; }
 }

And then you could theoretically define the event as:

 //... 
 public delegate void OrderSubmittedHandler(object sender, OrderSubmittedEventArgs eventArgs);
 public  event OrderSubmittedHandler OrderSubmitted;

But that's more code than we need, now that we're following the convention. .NET provides a helper class:

 //
 public event EventHandler<OrderSubmittedEventArgs> OrderSubmitted;


You also ask another question - why use an event rather than a multicast delegate?

You can think of an event as a multicast delegate, but with restrictions.

  • Only the object that owns the event can invoke it.
  • Another object can add or remove a listener, but cannot modify it in any other way

For example:

class TestClass
{
    public EventHandler ThisDelegate;
    public event EventHandler ThisEvent;

    private void TryThis()
    {
        if (this.ThisEvent != null)
        {
            // I can fire my own event
            this.ThisEvent(this, EventArgs.Empty);
        }

        // I can clear my own event
        this.ThisEvent = null;
    }
}


class OtherClass
{
    void Test()
    {
        var test = new TestClass();
        // I can invoke test's delegate!
        test.ThisDelegate(this, EventArgs.Empty);
        // I can clear test's delegate!
        test.ThisDelegate = null;

        // But I can't do that to its event
        test.ThisEvent(this, EventArgs.Empty); // Compiler error
        test.ThisEvent = null;  // Compiler error
    }
}

So, by making the member an Event rather than a Multicast Delegate, you give it extra semantic meaning that is enforced by the compiler. You do this for similar reasons that you mark members as private.

\$\endgroup\$
  • \$\begingroup\$ @radarbob That doesn't make any sense, you seem to mixing up declaring a delegate type and invoking an event. \$\endgroup\$ – svick Sep 9 '13 at 0:16
  • \$\begingroup\$ why, yes. yes I am. SO.. what I'm trying to say is, one may forego subclassing EventArgs and then pass EventArgs.Empty. If of course one does not need to pass data in the object. I'd argue that's conventional. \$\endgroup\$ – radarbob Sep 9 '13 at 0:20
0
\$\begingroup\$

I think you need another entity to delegate events, it makes no sense to pass shopping cart to your "departments" constructors.

class Shop
{
    public ShoppingCart GetCart(int id)
    {
        var cart = new ShoppingCart(id);
        shoppingCart.OrderSubmittedEvent += OnOrderSubmitted;
        return cart;
    }

    private BillingDepartment _billingDepartment = new BillingDepartment(shoppingCart);
    private MailingDepartment _mailingDepartment = new MailingDepartment(shoppingCart);

    private void OnOrderSubmitted(OrderDetails orderDetails)
    {
        _billingDepartment.Bill(orderDetails);
        _mailingDepartment.Submit(orderDetails);

        //notice unsubsription, its a good style to do that 
        //even if you dont really need to
        //your current event signature does not allow access to shopping cart tho.
        //it probably should

        //shoppingCart.OrderSubmittedEvent -= OnOrderSubmitted;
    }
}

Usage:

var shop = new Shop();
var cart = shop.GetCart(10);
cart.Add(75458);
cart.Add(54693);
cart.Add(52145);
cart.Submit();

Also this:

public void Submit()
{
    OrderSubmittedEvent.Invoke(new OrderDetails { ItemCodes = _orders, UserId = _userId });
}

is bad code. The proper way do invoke events is:

public void Submit()
{
    //copy to local variable to avoid racing i multithreading environment
    var ev = OrderSubmittedEvent;
    //check for null
    if (ev != null)
    {
        //only then invoke
        ev(new OrderDetails { ItemCodes = _orders, UserId = _userId });
    }
}

Andrew also makes valid point about common conventions when it comes to events, check his answer. In my inner modules, i would probably break it and use Action<T> as event type though, but in code availible to others you should use EventHandler<T>.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.