adding elements to a byte array: simple question

[zachattack05]

I haven't had enough coffee this morning yet and for some reason this doesn't look right.

I have two objects that I need to convert into one byte array. One object is already a byte array, and the other is a Guid.

Given that sockId is a Guid and commandData is a byte array, is this valid?

//finally we need to add the socket's Guid to the commandData so that the interpreter
            //knows where to send the response and also remove the data length that was originally
            //contained in the byte array at position 0.
            int TotalBytes = dataLength - 4 + 16;
            int i = 0;
            byte[] NewCommandData = new byte[TotalBytes];

            while (i <= TotalBytes)
            {
                //add the Guid byte array
                while (i < 16)
                {
                    NewCommandData[i] = sockId.ToByteArray()[i];
                    i++;
                }

                //add the commandData byte array
                NewCommandData[i] = commandData[i - 12];
                i++;
            }

or is there a better (preferrably faster, more elegant) way of doing this?

[gusano79]

is this valid? or is there a better (preferrably faster, more elegant) way of doing this?

Does it work the way you expect it to? That's the first test. =)

Here's a few things to improve:

1. The loop to add the GUID data shouldn't be inside the loop that adds the rest of the data. You'll end up with the GUID repeatedly copied into NewCommandData , which isn't what you want. It should be separate:

int TotalBytes = dataLength - 4 + 16;
int i = 0;

byte[] NewCommandData = new byte[TotalBytes];

//add the Guid byte array
while (i < 16)
{
    NewCommandData[i] = sockId.ToByteArray()[i];
    i++;
}
    
while (i <= TotalBytes)
{

    //add the commandData byte array
    NewCommandData[i] = commandData[i - 12];
    i++;
}

2. You're calling sockId.ToByteArray() multiple times inside a loop, but sockId doesn't ever change, so it's more efficient to do this conversion once, outside of the loop:

int TotalBytes = dataLength - 4 + 16;
int i = 0;

byte[] NewCommandData = new byte[TotalBytes];
byte[] sockIdBytes = sockId.ToByteArray();

//add the Guid byte array
while (i < 16)
{
    NewCommandData[i] = sockIdBytes[i];
    i++;
}
    
while (i <= TotalBytes)
{
    //add the commandData byte array
    NewCommandData[i] = commandData[i - 12];
    i++;
}

3. You don't need to copy each byte by hand; that's what Array.Copy is for:

int TotalBytes = dataLength - 4 + 16;

byte[] NewCommandData = new byte[TotalBytes];
byte[] sockIdBytes = sockId.ToByteArray();

//add the Guid byte array
Array.Copy(sockIdBytes, NewCommandData, sockIdBytes.Length);

//add the commandData byte array
Array.Copy(commandData, 4, NewCommandData, 16, commandData.Length - 4);

Does that all make sense?

There are usually multiple ways to get the same result. Here's another way to create NewCommandData that isn't necessarily better than the above code, just different:

MemoryStream ms = new MemoryStream();

//add the Guid byte array
ms.Write(sockId.ToByteArray(), 0, 16);

//add the commandData byte array
ms.Write(commandData, 4, commandData.Length - 4);

byte[] NewCommandData = ms.ToArray();

I'm just offering that as an example of a different perspective on the problem; it's good to recognize alternatives, even if you don't end up using them.

[zachattack05]

Excellent!

Thanks for that. I knew it didn't look right. Also, that Array.Copy trick is kinda nifty :)

[zachattack05]

As a hint.

Instead of creating a new array for the Guid, I just did this:

Array.Copy(sockId.ToByteArray(), NewCommandData, sockId.ToByteArray().Length);
            Array.Copy(commandData, 4, NewCommandData, 16, commandData.Length - 4);

[gusano79]

As a hint.

Instead of creating a new array for the Guid, I just did this:

Array.Copy(sockId.ToByteArray(), NewCommandData, sockId.ToByteArray().Length);
            Array.Copy(commandData, 4, NewCommandData, 16, commandData.Length - 4);

Ah, but now you're calling sockId.ToByteArray() twice again when it's not necessary. If you're going to reuse the result of a method and don't expect the returned value to change, it's better to squirrel the result away in a local variable. This makes the code clearer, and if it's something that's going to be repeated a lot, you'll see efficiency improve.

[zachattack05]

Hmm...good point.