4
\$\begingroup\$

I'm writing in VB.NET, using VS Community 2013. I don't know much about this language. I have managed to make my code work, but I suspect it is possible to improve the code a lot.

This code extracts packets from some data I've received using TcpClient.

Private inbuf(10000) As Byte
Private inoff As Integer
Private Sub ProcessBuffer
 ' Buffer may contain multiple packets, delimited by a newline
 Do While 1
 Dim newline As Byte = 10
 Dim newline_index = System.Array.IndexOf(Of Byte)([inbuf], newline) ' I think this is zero-based index
 Dim Data As String = Nothing ' if I leave this initializer out then Data seems to retain its value for next loop iteration
 If (newline_index >= 0 AndAlso newline_index < inoff) Then ' packet found
 Data = Encoding.ASCII.GetString(inbuf, 0, newline_index)
 inbuf = inbuf.Skip(newline_index + 1).ToArray()
 ReDim Preserve inbuf(10000) ' the Skip shortened the array
 inoff = inoff - (newline_index + 1)
 If inoff < 0 Then inoff = 0 ' shouldn't happen but seems to happen sometimes?
 End If
 ' If we did not find a newline (or the input was empty string), it means we need to wait for more input
 If String.IsNullOrEmpty(Data) Then
 Return
 End If
 ' Process the response string
 handle_packet(Data)
 Loop
End Sub
asked Dec 2, 2015 at 23:15
\$\endgroup\$
7
  • \$\begingroup\$ Instead of the methods that use the IASyncResult pattern, is it an option for you to use the newer Async methods (ReadAsync, etc.)? .NET 4.5+ would be required. \$\endgroup\$ Commented Dec 2, 2015 at 23:22
  • \$\begingroup\$ Yes that would be an option \$\endgroup\$ Commented Dec 2, 2015 at 23:28
  • \$\begingroup\$ @moarboilerplate i'm not sure how I would use ReadAsync in this situation; I want my UI to remain responsive while waiting for data and could not find any code examples along those lines (all the examples show executing some other statements simultaneously with the async call, but then blocking for the async call to complete once those other statements are finished) \$\endgroup\$ Commented Dec 3, 2015 at 0:25
  • \$\begingroup\$ I don't have the luxury of being able to do a full write up tonight, but using await will not block, it will return control to the caller until the task is finished, and then subsequent lines will execute. So your loop should suspend until the task completes but not block the UI. Also if you want a pattern closer to the current callback pattern you can use .ContinueWith on the task. Hopefully that helps? \$\endgroup\$ Commented Dec 3, 2015 at 4:20
  • \$\begingroup\$ @moarboilerplate sort of... I'll try some things on that front. (Note that this doesn't affect the main point of my question, which is the logic for extracting a packet from the buffer) \$\endgroup\$ Commented Dec 3, 2015 at 4:21

1 Answer 1

1
\$\begingroup\$

Not a full review, but:

If String.IsNullOrEmpty(Data) Then is completely unnecessary. Because Data will be null if you didn't set it within the If portion. Just add an else to the previous if and have it return.

If (newline_index >= 0 AndAlso newline_index < inoff) Then ' packet found
 ...
else 
 Return
End If

Also your variable names could use some work. I actually don't know what inoff is supposed to stand for here.

answered Dec 4, 2015 at 14:50
\$\endgroup\$
1
  • \$\begingroup\$ Good catch, I had that around from an earlier iteration of the code that was more complicated \$\endgroup\$ Commented Dec 4, 2015 at 22:14

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.