De-serializing and serializing objects to be sent over the network

Question

I am serializing and de-serializing a class object within the class which is then sent over the network like this:

class data
  public name as string
  public cmd as Commands
  public  message  as String

 Public Sub New(ByVal byteData As Byte())
        Try
            ' convert byte array to memory stream
            Using _ms As New MemoryStream(byteData)
                ' create new BinaryFormatter
                Dim _bf As New BinaryFormatter()
                ' set memory stream position to starting point
                _ms.Position = 0
                ' Deserializes a stream into an object graph and return as a object.
                dim oData as data =_bf.Deserialize(_ms)
                name = oData.name 
                message = oData.Message
                cmd = oData.cmd
            End Using

        Catch ex As Exception
              Throw
        End Try

    End Sub

'Converts the Data structure into an array of bytes
    Public Function ToByte() As Byte()

        Dim bf As New BinaryFormatter()
        Try
            Using ms As New MemoryStream()
                bf.Serialize(ms, Me)
                Return ms.ToArray()
            End Using
        Catch ex As SerializationException
            Return Nothing
        Finally
            bf = Nothing
        End Try
    End Function
end class

Is this proper and efficient?

Answer 1

IMHO you are doing to much inside of the constructor (Sub New()). The constructor should be used to construct the object. It should be avoided that exceptions are thrown from inside of a constructor.

A better approach would be to add a Shared method which takes care of the deserializing and make the constructor private. This shared method would then return the fully constructed data object.

---

The exception handling in the constructor is senseless in its current state. You don't gain any advantage of using a Try..Catch because you are throwing the exception anyway. You can simply omit it.

---

Instead of calling ms.ToArray() in the ToByte() method you sould call ms.GetBuffer() which returns the content of the stream without creating a new byte array.

---

There is no need to set the Position property of the newly created MemoryStream. It is automatically set to the beginning. So skip _ms.Position = 0.

---

While we are at _ms, one shouldn't use underscore prefix unless it is a class level variable where it is accteptable.

---

  public name as string
  public cmd as Commands
  public  message  as String  

These fields shouldn't be public avaible. For encapsulation you should use properties.

---

Comments should be used to explain why something is done in the way it is done. Let the code speak for itself about what is done by using meaningful and descriptive names for classes, methods and variables.
Comments like ' create new BinaryFormatter only add noise to the code which should be removed for readability.

---

In the ToByte() method the setting of bf = Nothing is not needed and therefor you should declare the BinaryFormatter as near to its usage as possible (inside the using).

The Catch ex As SerializationException would need a comment why you handle the exception like you do.

---

This 'Converts the Data structure into an array of bytes should be put inside a XML documentation block.

---

Dim oData as data  

Please don't use hungarian notation like you are using it. By having intellisense you can easily hoover over the variable and see it is a class. There is no need to prefix it with o for object.

---

Naming a class data does not add any value. One should name a class in a way that it is obvious what purpose the class serves.

You also should be consistent at your naming style. Sometimese you are using an underscore prefix (like already stated) and sometimes you don't. If you have choosen a style you should stick to it.

Answer 2

I get the distinct feeling that this should be a set of extension methods instead of a constructor or regular method. It has that code smell about it.

MSDN Reference of Extension Methods

Also, you should use more proper naming conventions. (Name instead of name, etc.) And you don't need those Try/Catch/Finally blocks. Lastly, you seem to be missing the <Serializable> attribute.

Take your initial code

class data
    public name as string
    public cmd as Commands
    public  message  as String

    Public Sub New(ByVal byteData As Byte())
        Try
            ' convert byte array to memory stream
            Using _ms As New MemoryStream(byteData)
                ' create new BinaryFormatter
                Dim _bf As New BinaryFormatter()
                ' set memory stream position to starting point
                _ms.Position = 0
                ' Deserializes a stream into an object graph and return as a object.
                dim oData as data =_bf.Deserialize(_ms)
                name = oData.name 
                message = oData.Message
                cmd = oData.cmd
            End Using
        Catch ex As Exception
              Throw
        End Try
    End Sub

    'Converts the Data structure into an array of bytes
    Public Function ToByte() As Byte()
        Dim bf As New BinaryFormatter()
        Try
            Using ms As New MemoryStream()
                bf.Serialize(ms, Me)
                Return ms.ToArray()
            End Using
        Catch ex As SerializationException
            Return Nothing
        Finally
            bf = Nothing
        End Try
    End Function
end class

And let's rewrite it to be more effective.

<Serializable>
Class Data
    Public Name As String
    Public Cmd As Commands
    Public Message As String
End Class

<Extension>
Public Sub FromByteArray(ByVal data As Data, ByVal byteArray As Byte())
    Using _ms As New MemoryStream(byteArray)
        ' create new BinaryFormatter
        Dim _bf As New BinaryFormatter()
        ' set memory stream position to starting point
        _ms.Position = 0
        ' Deserializes a stream into an object graph and return as a object.
        Dim oData As Data = _bf.Deserialize(_ms)
        data.Name = oData.Name
        data.Message = oData.Message
        data.Cmd = oData.Cmd
    End Using
End Sub

<Extension>
Public Function ToByteArray(ByVal data As Data)
    Dim bf As New BinaryFormatter()
    Using ms As New MemoryStream()
        bf.Serialize(ms, data)
        Return ms.ToArray()
    End Using
End Function

Do note: Extension methods can only be declared at the module level in VB.NET.

This technique very gracefully separates the responsibility of the class and it's serialization. The class should be responsible for handling it's work, whereas the Serialization/Deserialization can be done in an extension method, which makes it appear to be part of the class, but it doesn't need to be.

This, and the modifications by Heslacher, should get you much more desirable code. (I did not include any modifications he proposed, except the obvious code cleaning.)

Sample usage:

Sub Main()
    Dim data As New Data()
    data.Name = "Test"
    Dim ByteArray As Byte() = data.ToByteArray()
    data.FromByteArray(ByteArray)
    Console.WriteLine(data.Name)
    Console.ReadLine()
End Sub

That will print Test, not only proving that this method works, but also proving, I think, your original design was flawed. How do you create an empty Data object if the only constructor for it requires a Byte-Array?