Do not nest type warning (C#)

I have the following code in a class to setup a serial port even handler.

I get two warnings; CA1034 (Do not nest types), which tells me to make my delgate private (which will stop me being to setup the even handler in inherited classes), and CA1009 (declare the second parameter of the event as an EventArgs, or a instance of a type that extends EventArgs, named 'e'), which I don't understand.

My code is below

myPort.DataReceived += new SerialDataReceivedEventHandler(port_OnDataRecived); //Setup when port is opened

private void port_OnDataRecived(object sender, SerialDataReceivedEventArgs e)
{
        int lengthToRead = myPort.BytesToRead;
        byte[] rxBytes = new byte[lengthToRead];
        myPort.Read(rxBytes, 0, lengthToRead);
        PacketReceived(rxBytes, e);
}

public delegate void PacketReceivedEventHandler(object sender, byte[] packet);
public event PacketReceivedEventHandler OnPacketReceived;

public virtual void PacketReceived(byte[] packet, EventArgs e)
{
        if (OnPacketReceived != null)
        {
            OnPacketReceived(this, packet);                
        }
}

I have been looking at MSDN articles and some SO questions, but I can't relate the suggested fixes back to my own code. This answer sort of makes sense, but converting my code to look like it leads to

OnPacketReceived(this, packet);

being replaced with

handle(this, new PacketReceivedEventHandler();

which takes an argument (void (object, byte[]) target) (that's where I get stuck). As for trying to fix the CA1034 warning I don't even see how what I have written is a nested type and the MSDN article doesn't contain a example of how to fix the rule violation.

Jon Skeet
people
quotationmark

I don't even see how what I have written is a nested type

You've declared your delegate type (PacketReceivedEventHandler) within your class. That's one type nested inside another. Just move the declaration outside your existing class declaration.

For the other warning, you'd want to create a PacketEventArgs class deriving from EventArgs and containing the packet data as an extra property. At that point, you could use an EventHandler<PacketEventArgs> and not declare your own delegate at all.

Additionally:

  • Your current way of checking handler nullity is broken as it could change from being non-null to null between calls.
  • Normally the event would be called PacketReceived and your method would be OnPacketReceived (and protected), rather than the other way round.

With all this in place, you'd have:

public event EventHandler<PacketEventArgs> PacketReceived;

protected virtual void OnPacketReceived(byte[] packet)
{
    var handler = PacketReceived;
    if (handler != null)
    {
        handler.Invoke(this, new PacketEventArgs(packet));
    }
}

Or in C# 6, using the null conditional operator to make the implementation simpler:

public event EventHandler<PacketEventArgs> PacketReceived;

protected virtual void OnPacketReceived(byte[] packet)
{
    PacketReceived?.Invoke(this, new PacketEventArgs(packet));
}

people

See more on this question at Stackoverflow