2
\$\begingroup\$

I'm recieving UDP messages as byte arrays and depending on details in some of the first byte, I need to convert remaining bytes into a multitude of different possible arrangements of other data types.

These data types are primarily 'uint', 'ushort' and just 'byte' but can be in any order, and there can be any number of them. Each of these items will have a named variable.

I've tried using quite a few different options and am getting close, but feel that the methods I've created are not the best they could be. In places I've wanted to use 'sizeof' but don't want to mark the code as 'unsafe'. I've tried to use the 'params' keyword on the input, but this cannot be in conjunction with 'ref'. I've wanted to pass them with a maximum number of T1, T2, etc. using generics, but realised I can't enumerate these. I'm now passing an array of variables in conjunction with the 'ref' keyword, which means creating the array in memory but only the array gets updated with the changed and not the original variables.

byte[] message = new byte[] { }; //some byte array
ushort item1 = default(ushort);
byte item2 = default(byte);
var argumentArray = new object[] { item1, item2 };
ConvertArray(response, ref argumentArray);
private void ConvertArray(byte[] response, ref object[] items)
{
    int index = 0;
    for (int i = 0; i < items.Length; i++)
    {
        var item = items[i];
        var itemType = item.GetType();
        var itemSize = SizeOf(item.GetType());

        if (itemSize == 0)
        {
            continue;
        }
        else if (itemSize == 1)
        {
            items[i] = response[index];
        }
        else
        {
            var method = typeof(BitConverter).GetMethod($"To{itemType.Name}");
            var returned = method.Invoke(null, new object[] { response, index });
            items[i] = Convert.ChangeType(returned, itemType);
        }

        index = index + itemSize;
     }
}
private int SizeOf(Type type)
{
   switch (type.Name)
   {
       case nameof(UInt16):
           return 2;

       case "Byte":
           return 1;

       default:
           return 0;
   }
}

So this is partially working in that 'argumentArray' is updating with the values from the 'ConvertArray' method, but I'm sure there is a neater way to do this using Types.

Ideally I wouldn't need to create the 'argumentArray' and just pass the items (e.g. Item1 and Item2) directly as arguments to the method.

\$\endgroup\$

closed as off-topic by t3chb0t, yuri, pacmaninbw, esote, Toby Speight May 24 at 8:30

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Code not implemented or not working as intended: Code Review is a community where programmers peer-review your working code to address issues such as security, maintainability, performance, and scalability. We require that the code be working correctly, to the best of the author's knowledge, before proceeding with a review." – t3chb0t, yuri, pacmaninbw, esote, Toby Speight
If this question can be reworded to fit the rules in the help center, please edit the question.

  • 1
    \$\begingroup\$ Question number two, why do you need ref there? You're not re-assigning items it... \$\endgroup\$ – t3chb0t May 23 at 15:15
  • 3
    \$\begingroup\$ Firstly, you must not change the code after answers have been posted so I reverted the edit... secondly, the edit still doesn't make sense and does not use ref. I don't think you know how ref works... \$\endgroup\$ – t3chb0t May 23 at 15:30
  • 4
    \$\begingroup\$ I have rolled back your last edit. Please see What should I do when someone answers my question?: Do not add an improved version of the code after receiving an answer. Including revised versions of the code makes the question confusing, especially if someone later reviews the newer code. Please refrain from further code edits. \$\endgroup\$ – t3chb0t May 23 at 16:10
  • 2
    \$\begingroup\$ FWIW everyone can see the edits \$\endgroup\$ – Mathieu Guindon May 23 at 16:19
  • 2
    \$\begingroup\$ @sebpinski can you clarify why you need ref? As t3chb0t, I'm inclined to believe you have misunderstood how it works; my first suggestion would be to remove it, as it may create confusion. Your code will behave exactly the same without it (assuming the caller does not change items concurrently, which is presumably not a supported use-case). \$\endgroup\$ – VisualMelon May 23 at 16:37
5
\$\begingroup\$
       case nameof(UInt16):
           return 2;

       case "Byte":
           return 1;

Be consistent. nameof is clearly preferable.


private void ConvertArray(byte[] response, ref object[] items)
{
    int index = 0;
    for (int i = 0; i < items.Length; i++)
    {
        var item = items[i];
        var itemType = item.GetType();
        var itemSize = SizeOf(item.GetType());
  1. The last line should use the variable itemType.
  2. The sole point of items seems to be to extract types. So why is the parameter not params Type[] types?

        if (itemSize == 0)
        {
            continue;
        }

Would it be better to throw an exception?


        else if (itemSize == 1)
        {
            items[i] = response[index];
        }

That's a bug waiting to happen in the future when you add support for sbyte.


        {
            var method = typeof(BitConverter).GetMethod($"To{itemType.Name}");
            var returned = method.Invoke(null, new object[] { response, index });
            items[i] = Convert.ChangeType(returned, itemType);
        }

Ugh. I would much rather use T4 to generate the entire method and get rid of the reflection. That would probably also eliminate the need for Sizeof.


If you're using a recent .Net (e.g. .Net Core 2.1) then I think some combination of Span<T> and Memory<T> will do what you want. Over the past year or so, Microsoft has done a lot of work on C# features for byte-level memory manipulation. You can probably create a struct for the contents of the packet and coerce the byte[] into your struct with one or two lines of code.

\$\endgroup\$
  • \$\begingroup\$ @sebpinsky type.TypeCode can be used as an alternative for nameof(Type) \$\endgroup\$ – dfhwze May 23 at 15:07
  • \$\begingroup\$ @dfhwze, (a) I don't think sebpinsky will get that notification; (b) nameof(Type) gives "Type"; (c) reviews of OP's code belong in an answer, not in comments on someone else's answer. \$\endgroup\$ – Peter Taylor May 23 at 15:21
  • \$\begingroup\$ (b) was meant as pseudo-code, but could be confusing nevertheless (c) I have no idea how I ended up commenting the wrong section (brainfart I guess) + I will formulate an answer rather than commenting the next time \$\endgroup\$ – dfhwze May 23 at 15:25
  • \$\begingroup\$ 1. I'd like to be consistent. I'd tried doing 'nameof(byte)' and 'nameof(Int8)' (which doesn't exist. Just realised it'll be 'nameof(Byte)'. 2. I'd like to use 'params Type[]' as the argument, but just suggesting this without a full call signature and any suggestion of how to implement the method is not helpful. 3. Agreed that exceptions and an exhaustive list of types would be better but as StackExchange ask, I've added a shortened version of the code. 4. Any suggestions of how to actually use T4 here again, or are you going to just keep me guessing? \$\endgroup\$ – sebpinski May 23 at 15:45
  • 1
    \$\begingroup\$ @reggaeguitar, I see it more as using the right tool for the job. Reflection should be a last resort, not (just) because it's slow but because it moves errors from compile-time to runtime. That proposal also eliminates a large class of potential bugs in Sizeof while allowing easy extension to the full range of fixed width types. (Of course, it's important to note that the last paragraph renders the rest of the answer academic: I expect that all of the previous points will be things to take into account in the future rather than direct changes to make to the code). \$\endgroup\$ – Peter Taylor May 24 at 8:52
3
\$\begingroup\$

If you determine the valid types to handle in code as it seems from your method, I would do it more rigidly than by using Reflection. There is no meaning in determining the byte length by "hand" and then afterwards call a method by Reflection relying on consistency in names (not to mention possible types with the same byte length).

Why not try something plain and simple like:

private void ConvertArray(byte[] response, ref object[] items)
{
  using (MemoryStream stream = new MemoryStream(response))
  using (BinaryReader reader = new BinaryReader(stream))
  {
    int index = 0;
    for (int i = 0; i < items.Length; i++)
    {
      var itemType = items[i].GetType();
      int itemSize = 0;

      switch (itemType.Name)
      {
        case nameof(Byte):
          items[i] = reader.ReadByte();
          itemSize = sizeof(byte);
          break;
        case nameof(UInt16):
          items[i] = reader.ReadUInt16();
          itemSize = sizeof(ushort);
          break;
        case nameof(UInt32):
          items[i] = reader.ReadUInt32();
          itemSize = sizeof(uint);
          break;
      }

      index += itemSize;
    }
  }
}

As dfhwze comments using the type code for primitives is even more stringent:

private void ConvertArray(byte[] response, ref object[] items)
{
  using (MemoryStream stream = new MemoryStream(response))
  using (BinaryReader reader = new BinaryReader(stream))
  {
    for (int i = 0; i < items.Length; i++)
    {
      var itemType = items[i].GetType();

      switch (Type.GetTypeCode(itemType))
      {
        case TypeCode.Byte:
          items[i] = reader.ReadByte();
          break;
        case TypeCode.UInt16:
          items[i] = reader.ReadUInt16();
          break;
        case TypeCode.UInt32:
          items[i] = reader.ReadUInt32();
          break;
      }
    }
  }
}

As sebpinski points out in the comment the index and itemSize is unecessary in this approach.

\$\endgroup\$
  • \$\begingroup\$ Consider using itemType.TypeCode for primitive types. And +1 because I would also go the streamreader direction. \$\endgroup\$ – dfhwze May 23 at 15:03
  • \$\begingroup\$ @dfhwze: good idea! - I'll update \$\endgroup\$ – Henrik Hansen May 23 at 15:08
  • \$\begingroup\$ @HenrikHansen I really like your suggestion here. The only problem now is the reassigning back to the original items. I don't need the' index' and 'itemSize' anymore with this implementation. \$\endgroup\$ – sebpinski May 23 at 15:40
  • 1
    \$\begingroup\$ @sebpinski: OK, but to be strict in respect to the rules on this site, you're not allowed to change the original question when answers have been posted. But you can always post a self-answer. \$\endgroup\$ – Henrik Hansen May 23 at 15:59
  • 1
    \$\begingroup\$ @HenrikHansen Apologies, I simply realised that I'd forgotten to include part of the original code, so added it. Frankly the original question wouldn't have worked without it. I won't update the question again now, as I didn't realise this was a rule in this community. \$\endgroup\$ – sebpinski May 23 at 16:04

Not the answer you're looking for? Browse other questions tagged or ask your own question.