Removing bytes from a byte array

Question

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<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;
  }
}

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<byte>();
  int i;
  for (i = 0; i <= input.Length - pattern.Length; i++)
  {
    var foundMatch = !pattern.Where((t, j) => 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 <= 0 || pattern.Length <= 0 || start > 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;
    }
  return foundIndex;
}

I think the remove bytes method is fine, but the way I search for the wilcard pattern and copy the array seems a bit clunky. Credit to the author of the wildcard search code which can be found here.

Answer 1

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<int> containing the start positions of the possible patterns will improve readability like so

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;
        }
    }
}  

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 <= 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;

}  

---

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<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;
  }
}

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

• if (w <= 0) continue; the continue should better be a break. There is no need to evaluate again the while condition. In addition this condition should be (w < 0) if the pattern should be found at the start too.

• 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) { 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; }
    }
}

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)
{
    byte[] pattern = new byte[] { 1, 0, 131 };
    bool[] wild = new bool[] { false, true, false };
    byte[] responsePattern = new byte[] { 130, 119, 126, 118, 48, 49, 12, 19, 14 };
    int patternLength = pattern.Length;

    while (true)
    {
        var readBuffer = new byte[4096];
        var asyncReader = await stream.ReadAsync(readBuffer, 0, readBuffer.Length);

        var result = RemovePattern(readBuffer, responsePattern);
        result = RemovePattern(result, pattern, wild);

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

Answer 2

Heslacher touched this part of the code, but I have a change that I think would make this a little more readable.

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);
}

I think that you should make this into a for loop without an iterator in the declaration (you probably could put it into the for loop declaration but it would look messy I think)

like this

for (int i = 1; i > 0)
{
    i = FindPattern(result, new byte[] { 1, 0, 131 }, new[] { false, true, false });
    l = result.ToList();
    l.RemoveAt(i);
    l.RemoveAt(i);
    l.RemoveAt(i);
    Buffer.BlockCopy(l.ToArray(), 0, result, 0, result.Length - 3);
}

If you were going to use that variable (w) somewhere outside of the loop, then I would say that you could assign the "initializer" outside of the for loop declaration. Your for loop would look like this

int i = 1;
for (; i > 0; )
{
    //...
}

According to MSDN, any part of the for loop declaration is optional

All of the expressions that define a for statement are optional. For example, the following statement creates an infinite loop.

for (; ; )
{
    // ...
}

---

I turned the variable into a int because the FindPattern is going to return an int. really it will work either way, but I like to be clear, really a personal preference here. It just feels right to me to declare that it is an integer.

---

I also looked at your RemoveBytes method and thought that it could be better written.

public static byte[] RemoveBytes(byte[] input, byte[] pattern)
{
    if (pattern.Length == 0) return input;
    var result = new List<byte>();
    int i;
    for (i = 0; i <= input.Length - pattern.Length; i++)
    {
        var foundMatch = !pattern.Where((t, j) => 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();
}

The first thing that bothered me was the iteration variable and the fact that you were using it in 2 for loops without re-initializing. I saw right away that you had a valid reason for this, but I had some ideas to wiggle around the way you had written this code.

I created one for loop where I had an initializer, a condition, and an iterator. then I used a single if/else statement to decide what to do with everything.

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