C# code - SNMP packet processing

Question

I have the following code, which does the following:

Reads from the packet from UDP (SNMP) into an array, check if this number is > 127, if yes then '&' this with 127, now pad it with leading zero's to make it 7 bits in total. Combine it with result. Now check the next one in the packet and repeat the above. It works for if 3 numbers > 127. Can this code be optimized check for unlimited throughout the length of the array

Please do let me know if you require any further information.

Thank you.

 int[] obj = new int[Objectlength];
 string result = string.Empty;
 for (int cnt = 0; cnt < Objectlength; cnt++)
 {
     obj[cnt] = Convert.ToInt16(packet[objectstart]);
     objectstart++;

     if (obj[cnt] >127)
     {

         int brush = 127;
         int bin = obj[cnt] & brush;
         string binR1 = Convert.ToString(bin, 2).PadLeft(7, '0');
         result = result + binR1;
         cnt++;

         if ((packet[objectstart] > 127) | (packet[objectstart]  < 127))
         {

             Console.WriteLine(packet[objectstart]);
             int bin1 = packet[objectstart] & brush ;
             binR1 = Convert.ToString(bin1, 2).PadLeft(7, '0');
             result = result + binR1;

             obj[cnt] = Convert.ToInt16(packet[objectstart]);
             cnt++;
             objectstart++;

             if ((packet[objectstart] > 127) | (packet[objectstart] < 127))
             {

                 Console.WriteLine(packet[objectstart]);
                 int bin2 = packet[objectstart] & brush;
                 binR1 = Convert.ToString(bin2, 2).PadLeft(7, '0');
                 result = result + binR1;

                 obj[cnt] = Convert.ToInt16(packet[objectstart]);
                 objectstart++;
             }

         }

     }

}

Jeff Vanzella · Accepted Answer · 2013-07-02 18:11:22Z

First things first. Your variable names are not very well thought out. Even after I decode it, cnt or count?? does not describe what it does. I think a better name would be currentIndex.

Second, C# standards say to use camelCasing for variable names. I would also read up on var, it is a very useful keyword for cleaning up code.

Third, brush should be a constant, if you follow my suggestion on the change to recursion, it should be made a class constant.

I also don't like that cnt is incremented outside of the loop.

Now onto the main question.

Just seeing if ((packet[objectstart] > 127) | (packet[objectstart] < 127)) twice in the same structure (one inside the other one) is a huge code smell to me, especially when it is the same code inside the second, as it is the first. This looks like a good case for recursion, a method calling itself.

It would go something like this:

private string ProcessSegment(int[] obj, int currentIndex, int objectStart, dynamic[] packet)
{
     if ((packet[objectstart] != 127) || currentIndex > Objectlength)
     {
         return string.Empty;
     }

     Console.WriteLine(packet[objectstart]);

     var bin2 = packet[objectstart] & brush;
     binR1 = Convert.ToString(bin2, 2).PadLeft(7, '0');

     obj[currentIndex] = Convert.ToInt16(packet[objectstart]);

     return binR1 + ProcessSegment(obj, currentIndex++, objectStart++, packet);
}

Your calling code will look something like:

obj[cnt] = Convert.ToInt16(packet[objectstart]);
objectstart++;

if (obj[cnt] >127)
{
    result += ProcessSegment(obj, cnt++, objectstart++, packet);
}

I have no way of testing this, but I think it should work. Also note, please change the dynamic in the ProcessSegment call to be whatever the packet variable is

Good comments overall. Tail recursion is usually fun, but in this case you end up concatenating the strings, which is somewhat slow. I would yield substrings / characters using a while / for loop instead. Looks like IEnumerable and recursion can be combined, but the result is slow. stackoverflow.com/questions/2055927/… — Leonid
– Leonid, Commented Jul 3, 2013 at 0:22
@Leonid that is true, and there are ways to get around it. I was trying to keep it relevant to the question so the OP could see what was done. — Jeff Vanzella
– Jeff Vanzella, Commented Jul 3, 2013 at 1:24
@JeffVanzella: Yes the I didn't like the recursive if statements either so I sat down yesterday and did the method thing. I haven't tested your's so I'm not sure about this one, but it's worth a try. I take other comments of yours like naming etc. This is a great answer. Hands up for that. Thank you. — user75464
– user75464, Commented Jul 3, 2013 at 3:52

Stack Exchange Network

C# code - SNMP packet processing

1 Answer 1

You must log in to answer this question.

Hot Network Questions

C# code - SNMP packet processing

1 Answer 1

You must log in to answer this question.

Related

Hot Network Questions