رمز C - بحاجة إلى توضيح الفعالية

Question

مرحبًا، لقد كتبت رمزًا بناءً على المتطلبات.

(الحقل1_6)(الحقل2_30)(الحقل3_16)(الحقل4_16)(الحقل5_1)(الحقل6_6)(الحقل7_2)(الحقل8_1).....هذا عبارة عن مجموعة واحدة (8 حقول) من البيانات.سوف نتلقى 20 دلوًا في المرة الواحدة، مما يعني إجمالي 160 حقلاً.أحتاج إلى أخذ قيم field3 وfield7 وfield8 بناءً على شرط محدد مسبقًا.إذا كانت وسيطة الإدخال TEH هي n ، فاخذ الحقول الثلاثة من الجرافة الأولى وإذا كان الأمر كذلك ، فأنا بحاجة إلى أخذ الحقول الثلاثة من أي دلو آخر غير الحقول الأولى.إذا كان Argumnet y ، فأنا بحاجة إلى مسح جميع الدلاء 20 بعد غيرها والتحقق من الحقل الأول من الدلو لا يساوي 0 ، وإذا كان صحيحًا ، فأحضر الحقول الثلاثة لهذا الجرافة والخروج.لقد كتبت الكود وهو يعمل بشكل جيد أيضًا .. ولكن لست واثقًا من فعاليته.أخشى حدوث عطل في بعض الوقت. يرجى اقتراح الرمز أدناه.

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

وكان لدي صعوبة في قراءة التعليمات البرمجية ولكن FWIW لقد أضاف بعض التعليقات، 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;
}

وهناك تقنية مفيدة عندما تريد إزاحة الوصول في مجموعة هو خلق البنية التي تعين تخطيط الذاكرة. ثم هل يلقي المؤشر إلى مؤشر من البنية واستخدام أعضاء البنية لاستخراج المعلومات بدلا من الخاص بك مختلف memcpy في

وأود أن أقترح أيضا على إعادة النظر في المعايير الخاصة بك إلى وظيفة في عام، إذا قمت بوضع كل منهم في البنية لديك سيطرة أفضل ويجعل وظيفة أكثر قابلية للقراءة منها مثلا.

int foo( input* inbalance, output* outbalance )

و(أو كل ما هو كنت تحاول القيام به)

Answer 2

شعوري هو أن هذا الرمز هش للغاية.قد ينجح الأمر جيدًا عند تقديم مدخلات جيدة (لا أقترح أن تقوم بفحص الأمر نيابةً عنك) ولكن إذا تم إعطاؤه بعض المدخلات غير الصحيحة، فسوف يتعطل ويحترق أو يعطي نتائج مضللة.

هل قمت باختبار المدخلات غير المتوقعة؟على سبيل المثال:

• لنفترض أن i_balance_info فارغ؟
• لنفترض أن i_balance_info هو ""؟

• لنفترض أن هناك أقل من 8 عناصر في سلسلة الإدخال، فماذا سيفعل هذا السطر من التعليمات البرمجية؟

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

• لنفترض أن العنصر الموجود في str[3] يبلغ طوله أقل من 16 حرفًا، فماذا سيفعل هذا السطر من التعليمات البرمجية؟

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

إن أسلوبي في كتابة مثل هذا الكود هو الحماية من كل هذه الاحتمالات.على الأقل أود أن أضيف عبارات ASSERT()، وعادة ما أكتب التحقق من صحة الإدخال الصريح وأرجع الأخطاء عندما يكون الأمر سيئًا.المشكلة هنا هي أن الواجهة لا يبدو أنها تسمح بأي احتمال لوجود مدخلات سيئة.