Question

J'ai un protocole qui nécessite un champ de longueur jusqu'à 32 bits, et il doit être généré au moment de l'exécution pour décrire le nombre d'octets dans un paquet donné.

Le code ci-dessous est un peu moche, mais je me demande si cela peut être refactorisé pour être légèrement plus efficace ou facilement compréhensible.Le problème est que le code ne générera que suffisamment d'octets pour décrire la longueur du paquet, donc moins de 255 octets = 1 octet de longueur, moins de 65535 = 2 octets de longueur, etc.

{
    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++;
    }
}
Était-ce utile?

La solution

En réalité, vous ne faites que quatre calculs, donc la lisibilité semble bien plus importante ici que l'efficacité.Mon approche pour rendre quelque chose comme ça plus lisible est de

  1. Extraire le code commun à une fonction
  2. Réunissez des calculs similaires pour rendre les modèles plus évidents
  3. Débarrassez-vous de la variable intermédiaire print_zeroes et soyez explicite sur les cas dans lesquels vous produisez des octets même s'ils sont nuls (c'est-à-direl'octet précédent était différent de zéro)

J'ai transformé le bloc de code aléatoire en fonction et modifié quelques variables (les traits de soulignement me posent problème dans l'écran d'aperçu des démarques).J'ai aussi supposé que octets est transmis, et que celui qui le transmet nous transmettra un pointeur afin que nous puissions le modifier.

Voici le code :

/* 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);
 }

Un peu plus efficace, et peut être plus facile à comprendre serait cette modification des quatre dernières instructions 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);

Je conviens cependant que la compression de ce champ rend la machine à états de réception trop compliquée.Mais si vous ne pouvez pas modifier le protocole, ce code est beaucoup plus facile à lire.

Autres conseils

Vous devriez vraiment utiliser un champ de largeur fixe pour votre longueur.

  • Lorsque le programme du destinataire doit lire le champ de longueur de votre paquet, comment sait-il où s'arrête la longueur ?
  • Si la longueur d'un paquet peut potentiellement atteindre 4 Go, une surcharge de 1 à 3 octets est-elle vraiment importante ?
  • Voyez-vous à quel point votre code est déjà devenu complexe ?

Essayez cette boucle :

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

    }

}

Je ne suis pas sûr de comprendre votre question.Qu'essayez-vous de compter exactement ?Si je comprends bien, vous essayez de trouver l'octet le plus significatif non nul.
Il est probablement préférable d'utiliser une boucle comme celle-ci :

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;
    }  
}
Licencié sous: CC-BY-SA avec attribution
Non affilié à StackOverflow
scroll top