HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpModerate

Removing bytes from a byte array

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
removingarraybytebytesfrom

Problem

I am reading from a Networkstream, and the response code is:

private async void GetResponse(NetworkStream stream)
{
  while (true)
  {
    var readBuffer = new byte[4096];
    var asyncReader = await stream.ReadAsync(readBuffer, 0, readBuffer.Length);
    var result = RemoveBytes(readBuffer, new byte[] { 130, 119, 126, 118, 48, 49, 12, 19, 14 });
    List l = null;
    var w = 1;
    while (w > 0)
    {
      w = FindPattern(result, new byte[] { 1, 0, 131 }, new[] { false, true, false });
      if (w <= 0) continue;
      l = result.ToList();
      l.RemoveAt(w);
      l.RemoveAt(w);
      l.RemoveAt(w);
      Buffer.BlockCopy(l.ToArray(), 0, result, 0, result.Length - 3);
    }
    if (l != null) txtBoxMessagesIn.Text += Encoding.ASCII.GetString(l.ToArray()) + Environment.NewLine;
    if (asyncReader <= 0) break;
  }
}


I initially remove a group of know bytes using:

public static byte[] RemoveBytes(byte[] input, byte[] pattern)
{
  if (pattern.Length == 0) return input;
  var result = new List();
  int i;
  for (i = 0; i  input[i + j] != t).Any();
    if (foundMatch) i += pattern.Length - 1;
    else result.Add(input[i]);
  }
  for (; i < input.Length; i++)
  {
    result.Add(input[i]);
  }
  return result.ToArray();
}


Then I have the following method to search for a pattern with a wilcard in a byte array:

```
public static int FindPattern(byte[] body, byte[] pattern, bool[] wild, int start = 0)
{
var foundIndex = -1;
if (body.Length body.Length - pattern.Length || pattern.Length > body.Length) return foundIndex;
for (var index = start; index <= body.Length - pattern.Length; index += 4)
if (wild[0] || (body[index] == pattern[0]))
{
var match = true;
for (var index2 = 1; index2 <= pattern.Length - 1; index2++)
{
if (wild[index2] || (body[index + index2] == pattern[index2])) continue;
match = false;
break;
}
if (!match) continue;
foundIndex = index;
break;
}

Solution

You aren't using braces {} for the outer loops body which is bad, bad, bad. You should always use them although they are optional it just will make your code less error prone.

If you decide not to use them, you should stick to your style. Right now you are mixing them.

Using a guard clause is a good idea, but if one needs to scroll to the right to see each condition its suboptimal.

Use some vertical space to group related code together makes your code much more readable and easier to maintain.

By adding a method which returns an IEnumerable containing the start positions of the possible patterns will improve readability like so

private static IEnumerable GetPatternStartings(byte[] body, byte[] pattern, bool[] wild, int start = 0)
{
    for (var index = start; index <= body.Length - pattern.Length; index += 1)
    { 
        if (wild[0] || (body[index] == pattern[0]))
        {
            yield return index;
        }
    }
}


The name of the method could/should be adjusted but I couldn't come up with a better one.

If we would now add a method to check if there really is a pattern like so

private static bool IsPattern(byte[] body, byte[] pattern, bool[] wild, int bodyIndex)
{
    for (var index = 1; index <= pattern.Length - 1; index++)
    {
        if (wild[index] || (body[bodyIndex + index] == pattern[index])) continue;

        return false;
    }

    return true;
}


we can then implement the mentioned points and change the former FindPattern() method to

public static int FindPattern(byte[] body, byte[] pattern, bool[] wild, int start = 0)
{
    var foundIndex = -1;

    if (body.Length  body.Length - pattern.Length
        || pattern.Length > body.Length)
    { return foundIndex; }

    foreach (int patternStart in GetPatternStartings(body, pattern, wild, start))
    {
        if (IsPattern(body, pattern, wild, patternStart)) { return patternStart; }
    }

    return foundIndex;

}


So let us take a look at this

private async void GetResponse(NetworkStream stream)
{
  while (true)
  {
    var readBuffer = new byte[4096];
    var asyncReader = await stream.ReadAsync(readBuffer, 0, readBuffer.Length);
    var result = RemoveBytes(readBuffer, new byte[] { 130, 119, 126, 118, 48, 49, 12, 19, 14 });
    List l = null;
    var w = 1;
    while (w > 0)
    {
      w = FindPattern(result, new byte[] { 1, 0, 131 }, new[] { false, true, false });
      if (w <= 0) continue;
      l = result.ToList();
      l.RemoveAt(w);
      l.RemoveAt(w);
      l.RemoveAt(w);
      Buffer.BlockCopy(l.ToArray(), 0, result, 0, result.Length - 3);
    }
    if (l != null) txtBoxMessagesIn.Text += Encoding.ASCII.GetString(l.ToArray()) + Environment.NewLine;
    if (asyncReader <= 0) break;
  }
}


  • The call to ToList() and 3 times RemoveAt() together with the Buffer.BlockCopy() will slow down the whole thing.



-
if (w

-
the
pattern and wild to be used in FindPattern() should be created at the start of the method. It won't change.

-
w is a poorly named variable, let us use patternIndex instead.

Applying this will lead to

private async void GetResponse(NetworkStream stream)
{
    byte[] pattern = new byte[] { 1, 0, 131 };
    bool[] wild = new bool[] { false, true, false };
    int patternLength = pattern.Length;

    while (true)
    {
        var readBuffer = new byte[4096];
        var asyncReader = await stream.ReadAsync(readBuffer, 0, readBuffer.Length);
        var result = RemoveBytes(readBuffer, new byte[] { 130, 119, 126, 118, 48, 49, 12, 19, 14 });

        var patternIndex = 1;
        while (patternIndex > 0)
        {
            patternIndex = FindPattern(result, pattern, wild);
            if (patternIndex  0) 
        {
            txtBoxMessagesIn.Text += Encoding.ASCII.GetString(l.ToArray()) + Environment.NewLine;
        }
        if (asyncReader <= 0) { break; }
    }
}


So now we are finished, aren't we ? NO !

Lets check this
RemoveBytes() method, because I see a pattern there.

After renaming the method to
RemovePattern we can have 2 overloaded ones like so

private static byte[] RemovePattern(byte[] input, byte[] pattern)
{
    return RemovePattern(input, pattern, new bool[pattern.Length]);
}  

private static byte[] RemovePattern(byte[] input, byte[] pattern, bool[] wild)
{
    int patternLength = pattern.Length;
    var patternIndex = -1;

    while ((patternIndex = FindPattern(input, pattern, wild)) >= 0)
    {
        byte[] current = new byte[input.Length - 3];
        Buffer.BlockCopy(input, 0, current, 0, patternIndex);
        Buffer.BlockCopy(input, patternIndex + patternLength, current, patternIndex, input.Length - patternIndex - patternLength);
        input = current;
    }
    return input;
}


You see, I have extracted the removal to a separate method. Now the former
GetResponse() method will become this

``
private async void GetResponse(NetworkStream stream)
{
by

Code Snippets

private static IEnumerable<int> GetPatternStartings(byte[] body, byte[] pattern, bool[] wild, int start = 0)
{
    for (var index = start; index <= body.Length - pattern.Length; index += 1)
    { 
        if (wild[0] || (body[index] == pattern[0]))
        {
            yield return index;
        }
    }
}
private static bool IsPattern(byte[] body, byte[] pattern, bool[] wild, int bodyIndex)
{
    for (var index = 1; index <= pattern.Length - 1; index++)
    {
        if (wild[index] || (body[bodyIndex + index] == pattern[index])) continue;

        return false;
    }

    return true;
}
public static int FindPattern(byte[] body, byte[] pattern, bool[] wild, int start = 0)
{
    var foundIndex = -1;

    if (body.Length <= 0
        || pattern.Length <= 0
        || start > body.Length - pattern.Length
        || pattern.Length > body.Length)
    { return foundIndex; }

    foreach (int patternStart in GetPatternStartings(body, pattern, wild, start))
    {
        if (IsPattern(body, pattern, wild, patternStart)) { return patternStart; }
    }

    return foundIndex;

}
private async void GetResponse(NetworkStream stream)
{
  while (true)
  {
    var readBuffer = new byte[4096];
    var asyncReader = await stream.ReadAsync(readBuffer, 0, readBuffer.Length);
    var result = RemoveBytes(readBuffer, new byte[] { 130, 119, 126, 118, 48, 49, 12, 19, 14 });
    List<byte> l = null;
    var w = 1;
    while (w > 0)
    {
      w = FindPattern(result, new byte[] { 1, 0, 131 }, new[] { false, true, false });
      if (w <= 0) continue;
      l = result.ToList();
      l.RemoveAt(w);
      l.RemoveAt(w);
      l.RemoveAt(w);
      Buffer.BlockCopy(l.ToArray(), 0, result, 0, result.Length - 3);
    }
    if (l != null) txtBoxMessagesIn.Text += Encoding.ASCII.GetString(l.ToArray()) + Environment.NewLine;
    if (asyncReader <= 0) break;
  }
}
private async void GetResponse(NetworkStream stream)
{
    byte[] pattern = new byte[] { 1, 0, 131 };
    bool[] wild = new bool[] { false, true, false };
    int patternLength = pattern.Length;

    while (true)
    {
        var readBuffer = new byte[4096];
        var asyncReader = await stream.ReadAsync(readBuffer, 0, readBuffer.Length);
        var result = RemoveBytes(readBuffer, new byte[] { 130, 119, 126, 118, 48, 49, 12, 19, 14 });

        var patternIndex = 1;
        while (patternIndex > 0)
        {
            patternIndex = FindPattern(result, pattern, wild);
            if (patternIndex < 0) { break; }

            byte[] current = new byte[result.Length - patternLength];
            Buffer.BlockCopy(result, 0, current, 0, patternIndex);
            Buffer.BlockCopy(result, patternIndex + patternLength, current, patternIndex, result.Length - patternIndex - patternLength);
            result = current;
        }

        if (result != null && result.Length > 0) 
        {
            txtBoxMessagesIn.Text += Encoding.ASCII.GetString(l.ToArray()) + Environment.NewLine;
        }
        if (asyncReader <= 0) { break; }
    }
}

Context

StackExchange Code Review Q#106118, answer score: 10

Revisions (0)

No revisions yet.