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