Event Arguments: Is this wrong?

[seth350]

Good afternoon all,

I have a question regarding event arguments and their correct use.

I have an app that I have created several different EventArgs for different events. Each passing the needed information to the event sub.

The class this sub is located in is declared once from the MainForm. In this sub, I am using StatusArgs as my event argument.
My question is, is it necessary for me to be calling "New StatusArgs" every time I raise an event? Would calling "New StatusArgs" be creating a new instance that eventually uses up resources?

I suppose the real question is, after calling "New StatusArgs" and setting the information in it, does the instance hang out forever in memory?

Would the better option be to create one instance of StatusArgs for the class, and then just set the arguments every time I raise the event? Would doing this essentially overwrite the variables that are stored in the instance?

Code: [Select]

    Public Sub VerifyPart(ByVal scan As String)

        Dim partFound As Boolean = False

        For Each item As ListViewItem In _list.Items
            If scan = item.SubItems.Item(1).Text Then
                If CheckOffItem(item) = True Then
                    RaiseEvent OnPartProcessed(Me, New StatusArgs(0, String.Format("Part# {0} is valid! Please scan the next part.", scan), Color.Green, Color.White))
                    _audio.ValidScan()
                Else
                    RaiseEvent OnPartProcessed(Me, New StatusArgs(2, String.Format("Part# {0} previously scanned!" & vbNewLine & "Verify your quanity and try again.", scan), Color.Red, Color.White))
                    _audio.InvalidScan()
                End If
                partFound = True
            End If
        Next item

        If partFound = False Then
            If scan = "NoRead" Then
                RaiseEvent OnPartProcessed(Me, New StatusArgs(2, "Scanner did not find a bar code. Try again.", Color.Red, Color.White))
                RaiseEvent InvalidLog(Me, New StatusArgs(scan, "Scanner did not find a bar code. No Read."))
            Else
                RaiseEvent OnPartProcessed(Me, New StatusArgs(2, String.Format("Part# {0} is invalid. Please verify your parts.", scan), Color.Red, Color.White))
                RaiseEvent InvalidLog(Me, New StatusArgs(scan, String.Format("Part# {0} was not found in the current parts list.", scan)))
                _audio.InvalidScan()
            End If

        End If

    End Sub


Code: [Select]

''' <summary>
''' Station Status Arguments Class For Passing Information To The Status Panel.
''' </summary>
Public Class StatusArgs
    Inherits EventArgs

    Private _code As Integer
    Private _msg As String
    Private _scan As String
    Private _backclr As Color
    Private _foreclr As Color

    ''' <summary>
    ''' Creates a new instance to pass status arguments to the status panel.
    ''' </summary>
    ''' <param name="code">Determines what the graphic and color is set to. 0=No Change To Image 1=Scanning Complete 2=Scan Invalid 3=General Alarm 4=Ready</param>
    ''' <param name="msg">Text to set the status to.</param>
    ''' <param name="backclr">Specify what color to set backcolor on status panel.</param>
    ''' <param name="foreclr">Specify what color to set the text to.</param>
    Public Sub New(ByVal code As Integer, ByVal msg As String, ByVal backclr As Color, ByVal foreclr As Color)

        _code = code
        _msg = msg
        _backclr = backclr
        _foreclr = foreclr

    End Sub

    ''' <summary>
    ''' Creates new instance of StatusArgs to pass scanned code and message.
    ''' </summary>
    ''' <param name="scan">The scanned bar code to pass.</param>
    ''' <param name="msg">The message to attach.</param>
    Public Sub New(ByVal scan As String, ByVal msg As String)
        _scan = scan
        _msg = msg
    End Sub

    ''' <summary>
    ''' Indicates what status image/color should be displayed.
    ''' </summary>
    ''' <returns>Integer</returns>
    Public ReadOnly Property Code As Integer
        Get
            Return _code
        End Get
    End Property

    ''' <summary>
    ''' Gets the status message.
    ''' </summary>
    ''' <returns>String</returns>
    Public ReadOnly Property Message As String
        Get
            Return _msg
        End Get
    End Property

    ''' <summary>
    ''' Gets the scanned code associated with this event.
    ''' </summary>
    ''' <returns>String</returns>
    Public ReadOnly Property Scan As String
        Get
            Return _scan
        End Get
    End Property

    ''' <summary>
    ''' Gets the image that the status image should be set to.
    ''' </summary>
    ''' <returns>Image</returns>
    Public ReadOnly Property Image As Image
        Get
            Select Case _code
                Case 0
                    Return Nothing
                Case 1 'scanning complete
                    Return My.Resources.greenCheck
                Case 2 'invalid scan
                    Return My.Resources.traffic_light_red
                Case 3
                    Return My.Resources.important
                Case 4 'station ready
                    Return My.Resources.traffic_light_green
                Case Else
                    Return My.Resources.gear
            End Select
        End Get
    End Property

    ''' <summary>
    ''' Gets the back color of the status image and message.
    ''' </summary>
    ''' <returns>Color</returns>
    Public ReadOnly Property BackColor As Color
        Get
            Return _backclr
        End Get
    End Property

    ''' <summary>
    ''' Gets the fore ground color for the status message.
    ''' </summary>
    ''' <returns>Color</returns>
    Public ReadOnly Property ForeColor As Color
        Get
            Return _foreclr
        End Get
    End Property

End Class

 

[Archie]

The best practice is to create a new instance of your event args for each event. The reason is to be safer in multi-threading. Think of the scenario that an event fires and part way through the event handler, another thread fires the event again. If you used the same event args instance, halfway through the event handler for first firing of the event, your arguments would suddenly change and potentially creating havoc not to mention making it extremely difficult to troubleshoot.

As for memory usage, once the instance has no more references pointing to it, the garbage collector will get rid of it.

Two other notes on your code....

1) According to .NET patterns and practices, event arg classes should be named with "EventArgs" suffix. So your class name should be StatusEventArgs

2) Raising events should be always done in a subroutine named On<name of event>. In your case, your event should be named PartProcessed. You should then have a subroutine declared like this:

Protected Overridable OnPartProcessed(byval e as StatusEventArgs)
     RaiseEvent PartProcessed(Me,e)
End Sub

Then instead of raising the event directly in your code, you call that sub.

This pattern allows inheriting classes to either override the event or insert code prior to the event being fired.

[seth350]

Thank you Archie, I see very well what you mean.
In my mind, I was seeing a new StatusArgs created every time the app verified a scan with potentially the exact same parameters as the last one that was created.

This particular event updates a picture box on the main form and sets the label for it.

 
This is just one of several things that I have looked at when trying to trim some fat in the app and optimize the code. My main concern is to try to resolve a high cpu usage on a Raspberry Pi 3.
Other areas were corrected that were constantly accessing properties of other classes, like inside a For Loop. Instead, I dim’d a local variable to first go grab the property, then use that variable in the For Loop.

It was also mentioned that the PictureBox control was frowned upon in regards to performance. An Image control was preferred unless features of the PictureBox were necessary.

[Archie]

If you are looking to optimize your code, Visual Studio has some very good tools. The Performance Analyzer in the Analyze menu can give some very good information, but can also be difficult to decipher. Essentially you run your program for a time period with the performance analyzer, then it can break down what code consumed the most CPU and memory.

[seth350]

If you are looking to optimize your code, Visual Studio has some very good tools. The Performance Analyzer in the Analyze menu can give some very good information, but can also be difficult to decipher. Essentially you run your program for a time period with the performance analyzer, then it can break down what code consumed the most CPU and memory.

Difficult to decipher. Yes. lol

Taking your advice, I ran the profiler and found that this bit of code is using the most memory, or 15%. Seems like using Resource Manager is quite taxing. I will try using direct file paths instead.

Code: [Select]

''' <summary>
    ''' Gets the image that the status image should be set to.
    ''' </summary>
    ''' <returns>Image</returns>
    Public ReadOnly Property Image As Image
        Get
            Select Case _code
                Case 0
                    Return Nothing
                Case 1 'scanning complete
                    Return My.Resources.greenCheck
                Case 2 'invalid scan
                    Return My.Resources.traffic_light_red
                Case 3
                    Return My.Resources.important
                Case 4 'station ready
                    Return My.Resources.traffic_light_green
                Case Else
                    Return My.Resources.gear
            End Select
        End Get
    End Property