質問
最大32ビットの長さフィールドを必要とするプロトコルがあり、特定のパケットにあるバイトの数を説明するために実行時に生成する必要があります。
以下のコードは醜いですが、これをリファクタリングして、少し効率的であるか、簡単に理解できるようにすることができるかどうか疑問に思っています。問題は、コードがパケットの長さを記述するのに十分なバイトのみを生成するため、255バイト未満の長さ、65535 = 2バイトの長さなど...
{
extern char byte_stream[];
int bytes = offset_in_packet;
int n = length_of_packet;
/* Under 4 billion, so this can be represented in 32 bits. */
int t;
/* 32-bit number used for temporary storage. */
/* These are the bytes we will break up n into. */
unsigned char first, second, third, fourth;
t = n & 0xFF000000;
/* We have used AND to "mask out" the first byte of the number. */
/* The only bits which can be on in t are the first 8 bits. */
first = t >> 24;
if (t) {
printf("byte 1: 0x%02x\n",first );
byte_stream[bytes] = first; bytes++;
write_zeros = 1;
}
/* Now we shift t so that it is between 0 and 255. This is the first, highest byte of n. */
t = n & 0x00FF0000;
second = t >> 16;
if (t || write_zeros) {
printf("byte 2: 0x%02x\n", second );
byte_stream[bytes] = second; bytes++;
write_zeros = 1;
}
t = n & 0x0000FF00;
third = t >> 8;
if ( t || write_zeros) {
printf("byte 3: 0x%02x\n", third );
byte_stream[bytes] = third; bytes++;
write_zeros = 1;
}
t = n & 0x000000FF;
fourth = t;
if (t || write_zeros) {
printf("byte 4: 0x%02x\n", fourth);
byte_stream[bytes] = fourth; bytes++;
}
}
解決
実際には 4 つの計算しか行っていないので、 読みやすさの方がはるかに重要なようです ここでは効率よりも。このようなものを読みやすくするための私のアプローチは、
- 関数に共通するコードを抽出する
- 同様の計算を組み合わせて、パターンをより明確にします。
- 中間変数 print_zeroes を削除し、バイトがゼロであっても出力する場合を明示的にします (つまり、前のバイトはゼロ以外でした)
ランダムなコードブロックを関数に変更し、いくつかの変数を変更しました(マークダウンのプレビュー画面でアンダースコアが問題を引き起こしています)。私もそれを想定していました バイト が渡されており、それを渡している人がポインタを渡して、それを変更できるようにします。
コードは次のとおりです。
/* append byte b to stream, increment index */
/* really needs to check length of stream before appending */
void output( int i, unsigned char b, char stream[], int *index )
{
printf("byte %d: 0x%02x\n", i, b);
stream[(*index)++] = b;
}
void answer( char bytestream[], unsigned int *bytes, unsigned int n)
{
/* mask out four bytes from word n */
first = (n & 0xFF000000) >> 24;
second = (n & 0x00FF0000) >> 16;
third = (n & 0x0000FF00) >> 8;
fourth = (n & 0x000000FF) >> 0;
/* conditionally output each byte starting with the */
/* first non-zero byte */
if (first)
output( 1, first, bytestream, bytes);
if (first || second)
output( 2, second, bytestream, bytes);
if (first || second || third)
output( 3, third, bytestream, bytes);
if (first || second || third || fourth)
output( 4, fourth, bytestream, bytes);
}
これまでよりわずかに効率が向上し、 多分 最後の 4 つの if ステートメントを次のように変更すると理解しやすくなります。
if (n>0x00FFFFFF)
output( 1, first, bytestream, bytes);
if (n>0x0000FFFF)
output( 2, second, bytestream, bytes);
if (n>0x000000FF)
output( 3, third, bytestream, bytes);
if (1)
output( 4, fourth, bytestream, bytes);
ただし、このフィールドを圧縮すると受信ステート マシンが過度に複雑になるという点には私も同意します。ただし、プロトコルを変更できない場合は、このコードの方がはるかに読みやすいです。
他のヒント
長さには固定幅フィールドを使用する必要があります。
- 受信側のプログラムがパケットの長さフィールドを読み取る必要がある場合、長さがどこで終わるかをどうやって知るのでしょうか?
- パケットの長さが 4 GB に達する可能性がある場合、1 ~ 3 バイトのオーバーヘッドは本当に重要でしょうか?
- コードがすでにどれほど複雑になっているかわかりますか?
このループを試してください。
{
extern char byte_stream[];
int bytes = offset_in_packet;
int n = length_of_packet; /* Under 4 billion, so this can be represented in 32 bits. */
int t; /* 32-bit number used for temporary storage. */
int i;
unsigned char curByte;
for (i = 0; i < 4; i++) {
t = n & (0xFF000000 >> (i * 16));
curByte = t >> (24 - (i * 8));
if (t || write_zeros) {
printf("byte %d: 0x%02x\n", i, curByte );
byte_stream[bytes] = curByte;
bytes++;
write_zeros = 1;
}
}
}
あなたの質問を理解できるかわかりません。いったい何を数えようとしているのでしょうか?私の理解が正しければ、ゼロ以外の最上位バイトを見つけようとしているということになります。
おそらく、次のようなループを使用する方がよいでしょう。
int i;
int write_zeros = 0;
for (i = 3; i >=0 ; --i) {
t = (n >> (8 * i)) & 0xff;
if (t || write_zeros) {
write_zeros = 1;
printf ("byte %d : 0x%02x\n", 4-i, t);
byte_stream[bytes++] = t;
}
}