Code C - besoin de clarifier l'efficacité

Question

Salut J'ai écrit un code basé sur une exigence.

(field1_6) (field2_30) (field3_16) (field4_16) (field5_1) (field6_6) (field7_2) (field8_1) ..... ceci est une benne (8 champs) de données. nous recevrons 20 seaux à la fois signifie totalement 160 champs. J'ai besoin de prendre les valeurs de field3, field7 & fields8 en fonction d'une condition prédéfinie. Si l'argument d'entrée est N alors prenez les trois champs du 1er seau et s'il est Y j'ai besoin de pour prendre les trois champs de tout autre seau autre que le premier. si argumnet est Y alors j'ai besoin de balayer tous les 20 seaux l'un après l'autre et de vérifier le premier champ du seau n'est pas égal à 0 et si elle est vraie puis récupérer les trois champs de ce seau et quitter. J'ai écrit le code et son fonctionne aussi bien ..mais pas si confiant que c'est effctive. J'ai peur d'un accident un peu de temps. S'il vous plaît suggérer ci-dessous est le code.

int CMI9_auxc_parse_balance_info(char *i_balance_info,char *i_use_balance_ind,char *o_balance,char *o_balance_change,char *o_balance_sign 
) 
{ 
    char *pch = NULL; 
    char *balance_id[MAX_BUCKETS] = {NULL}; 
    char balance_info[BALANCE_INFO_FIELD_MAX_LENTH] = {0}; 
    char *str[160] = {NULL}; 
    int i=0,j=0,b_id=0,b_ind=0,bc_ind=0,bs_ind=0,rc; 
    int total_bukets ; 
    memset(balance_info,' ',BALANCE_INFO_FIELD_MAX_LENTH); 
    memcpy(balance_info,i_balance_info,BALANCE_INFO_FIELD_MAX_LENTH); 
    //balance_info[BALANCE_INFO_FIELD_MAX_LENTH]='\0'; 
    pch = strtok (balance_info,"*"); 
    while (pch != NULL && i < 160) 
    { 
    str[i]=(char*)malloc(strlen(pch) + 1); 
    strcpy(str[i],pch); 
    pch = strtok (NULL, "*"); 
    i++; 
    } 
total_bukets = i/8 ; 
    for (j=0;str[b_id]!=NULL,j<total_bukets;j++) 
    { 
    balance_id[j]=str[b_id]; 
    b_id=b_id+8; 
    } 
    if (!memcmp(i_use_balance_ind,"Y",1)) 
    { 
    if (atoi(balance_id[0])==1) 
    { 
     memcpy(o_balance,str[2],16); 
     memcpy(o_balance_change,str[3],16); 
     memcpy(o_balance_sign,str[7],1); 
     for(i=0;i<160;i++) 
     free(str[i]); 
     return 1; 
    } 
    else 
    { 
     for(i=0;i<160;i++) 
     free(str[i]); 
     return 0; 
    } 
    } 
    else if (!memcmp(i_use_balance_ind,"N",1)) 
    { 
     for (j=1;balance_id[j]!=NULL,j<MAX_BUCKETS;j++) 
     { 
     b_ind=(j*8)+2; 
     bc_ind=(j*8)+3; 
     bs_ind=(j*8)+7; 
     if (atoi(balance_id[j])!=1 && atoi(str[bc_ind])!=0) 
     { 
     memcpy(o_balance,str[b_ind],16); 
     memcpy(o_balance_change,str[bc_ind],16); 
     memcpy(o_balance_sign,str[bs_ind],1); 
     for(i=0;i<160;i++) 
     free(str[i]); 
     return 1; 
     } 
     } 
    for(i=0;i<160;i++) 
    free(str[i]); 
    return 0; 
    } 
for(i=0;i<160;i++) 
free(str[i]); 
return 0; 
} 

Answer 1

J'ai eu du mal à lire votre code, mais FWIW j'ai ajouté quelques commentaires, HTH:

// do shorter functions, long functions are harder to follow and make errors harder to spot 
// document all your variables, at the very least your function parameters 
// also what the function is suppose to do and what it expects as input 
int CMI9_auxc_parse_balance_info 
(
    char *i_balance_info, 
    char *i_use_balance_ind, 
    char *o_balance, 
    char *o_balance_change, 
    char *o_balance_sign 
) 
{ 
    char *balance_id[MAX_BUCKETS] = {NULL}; 
    char balance_info[BALANCE_INFO_FIELD_MAX_LENTH] = {0}; 
    char *str[160] = {NULL}; 
    int i=0,j=0,b_id=0,b_ind=0,bc_ind=0,bs_ind=0,rc; 
    int total_bukets=0; // good practice to initialize all variables 

    // 
    // check for null pointers in your arguments, and do sanity checks for any 
    // calculations 
    // also move variable declarations to just before they are needed 
    // 

    memset(balance_info,' ',BALANCE_INFO_FIELD_MAX_LENTH); 
    memcpy(balance_info,i_balance_info,BALANCE_INFO_FIELD_MAX_LENTH); 
    //balance_info[BALANCE_INFO_FIELD_MAX_LENTH]='\0'; // should be BALANCE_INFO_FIELD_MAX_LENTH-1 

    char *pch = strtok (balance_info,"*"); // this will potentially crash since no ending \0 

    while (pch != NULL && i < 160) 
    { 
    str[i]=(char*)malloc(strlen(pch) + 1); 
    strcpy(str[i],pch); 
    pch = strtok (NULL, "*"); 
    i++; 
    } 
    total_bukets = i/8 ; 
    // you have declared char*str[160] check if enough b_id < 160 
    // asserts are helpful if nothing else assert(b_id < 160); 
    for (j=0;str[b_id]!=NULL,j<total_bukets;j++) 
    { 
    balance_id[j]=str[b_id]; 
    b_id=b_id+8; 
    } 
    // don't use memcmp, if ('y'==i_use_balance_ind[0]) is better 
    if (!memcmp(i_use_balance_ind,"Y",1)) 
    { 
    // atoi needs balance_id str to end with \0 has it? 
    if (atoi(balance_id[0])==1) 
    { 
     // length assumptions and memcpy when its only one byte 
     memcpy(o_balance,str[2],16); 
     memcpy(o_balance_change,str[3],16); 
     memcpy(o_balance_sign,str[7],1); 
     for(i=0;i<160;i++) 
     free(str[i]); 
     return 1; 
    } 
    else 
    { 
     for(i=0;i<160;i++) 
     free(str[i]); 
     return 0; 
    } 
    } 
    // if ('N'==i_use_balance_ind[0]) 
    else if (!memcmp(i_use_balance_ind,"N",1)) 
    { 
    // here I get a headache, this looks just at first glance risky. 
    for (j=1;balance_id[j]!=NULL,j<MAX_BUCKETS;j++) 
    { 
     b_ind=(j*8)+2; 
     bc_ind=(j*8)+3; 
     bs_ind=(j*8)+7; 
     if (atoi(balance_id[j])!=1 && atoi(str[bc_ind])!=0) 
     { 
     // length assumptions and memcpy when its only one byte 
     // here u assume strlen(str[b_ind])>15 including \0 
     memcpy(o_balance,str[b_ind],16); 
     // here u assume strlen(str[bc_ind])>15 including \0 
     memcpy(o_balance_change,str[bc_ind],16); 
     // here, besides length assumption you could use a simple assignment 
     // since its one byte 
     memcpy(o_balance_sign,str[bs_ind],1); 
     // a common practice is to set pointers that are freed to NULL. 
     // maybe not necessary here since u return 
     for(i=0;i<160;i++) 
      free(str[i]); 
     return 1; 
     } 
    } 
    // suggestion do one function that frees your pointers to avoid dupl 
    for(i=0;i<160;i++) 
     free(str[i]); 
    return 0; 
    } 
    for(i=0;i<160;i++) 
    free(str[i]); 
    return 0; 
} 

Une technique utile lorsque vous souhaitez accéder à des décalages dans un tableau est de créer une structure qui associe la disposition de la mémoire. Ensuite, vous jetez votre pointeur sur un pointeur de la structure et utilisez les membres de la structure pour extraire des informations au lieu de vos divers memcpy

Je vous suggère également de reconsidérer vos paramètres à la fonction en général, si vous placez chacun d'eux dans un struct vous avez un meilleur contrôle et rend la fonction plus lisible, par exemple

int foo(input* inbalance, output* outbalance) 

(ou quoi que ce soit que vous essayez de le faire)

Answer 2

Mon sentiment est que ce code est très fragile. Il peut très bien fonctionner quand on lui donne une bonne entrée (je ne propose pas de vérifier la chose à votre place), mais si on lui donne des entrées incorrectes, il risque de s'écraser et de brûler ou de donner des résultats trompeurs.

Avez-vous testé pour des entrées inattendues? Par exemple:

• Supposons que i_balance_info est null?
• Supposons que i_balance_info est ""?

• Supposons qu'il y ait moins de 8 éléments dans la chaîne d'entrée, que fera cette ligne de code?

  memcpy(o_balance_sign,str[7],1); 
  

• Supposons que que l'élément dans str [3] est inférieure à 16 caractères de long, ce que cette ligne de code va faire?

  memcpy(o_balance_change,str[3],16); 
  

Mon approche de l'écriture tel code serait de protéger contre toutes ces éventualités. À tout le moins, j'ajouterais des instructions ASSERT(), j'écrirais généralement une validation d'entrée explicite et retournerais des erreurs quand c'est mauvais. Le problème ici est que l'interface ne semble pas permettre une éventuelle mauvaise entrée.