C-Code - müssen die Wirksamkeit klären

Question

Hallo, ich habe einen Code geschrieben, basierend auf einer Anforderung.

(field1_6) (field2_30) (field3_16) (field4_16) (field5_1) (field6_6) (field7_2) (field8_1 ) ..... dies ist eine Schaufel (8 Felder) von Daten. wir werden 20 Eimer auf einmal Mittel insgesamt 160 Felder erhalten. Ich brauche die Werte von field3, Feld7 & fields8 auf vordefinierte Bedingung basiert zu nehmen. wenn teh Eingabeargument wird dann N die drei Felder vom 1. Eimer nehmen und wenn es Y i Notwendigkeit die drei Felder von einem anderen Schaufel andere als erstes ein zu nehmen. wenn argumnet Y dann muss ich alle die 20 Eimer eins nach dem anderen und die Prüfung scannen das erste Feld des Eimers ist nicht gleich 0 ist und, wenn es wahr ist, dann die drei Felder dieser Eimer und Austritt holen. Ich habe den Code geschrieben und es ist auch gut funktioniert ..aber nicht so zuversichtlich, dass es effctive ist. Ich habe Angst vor einem Absturz einige time.please unten vorschlagen der Code ist.

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

Ich hatte eine harte Zeit, Ihren Code zu lesen, aber FWIW Ich habe einige Kommentare hinzugefügt, 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;
}

Eine hilfreiche Technik, wenn Sie in einem Array Zugriff Offsets wollen, ist eine Struktur zu schaffen, die das Speicherlayout zuordnet. Dann werfen Sie Ihren Zeiger auf einen Zeiger der Struktur und verwenden Sie die Strukturkomponenten zu extrahieren Informationen anstelle Ihre verschiedenen Memcpy der

Ich würde auch vorschlagen, dass Sie Ihre Parameter an die Funktion im allgemeinen überdenken, wenn man jeden von ihnen in einer Struktur legen Sie eine bessere Kontrolle haben und macht die Funktion besser lesbar z.

int foo( input* inbalance, output* outbalance )

(oder was auch immer es ist, dass Sie versuchen, zu tun)

Answer 2

Mein Gefühl ist, dass dieser Code sehr spröde ist. Es kann gut funktionieren, wenn guten Eingang (I nicht vor, auf Schreibtisch für Sie die Sache überprüfen), aber wenn einige falschen Eingaben gegeben wird es entweder zum Absturz bringen und brennt oder gibt Ergebnisse irreführend.

Haben Sie für unerwartete Eingänge getestet? Zum Beispiel:

• Nehmen wir an i_balance_info null ist?
• Nehmen wir an i_balance_info ist ""?

• Angenommen, es gibt weniger als 8 Elemente in der Eingabezeichenfolge, was diese Zeile Code tun?

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

• , dass Angenommen, dass das Element in str [3] weniger als 16 Zeichen lang sein, was diese Zeile Code zu tun?

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

Mein Ansatz solchen Code zu schreiben wäre gegen alle solche Fälle zu schützen. Zumindest würde ich ASSERT () Anweisungen hinzufügen, würde ich schreiben in der Regel explizite Eingabevalidierung und Rückkehr Fehler, wenn es schlecht. Das Problem hierbei ist, dass die Schnittstelle nicht für jede Möglichkeit zu erlauben scheint, dass es möglicherweise schlecht eingegeben werden.