С кодом - нужно уточнить эффективность - PullRequest
0 голосов
/ 09 октября 2009

Привет, я написал код, основанный на требовании.

(field1_6) (field2_30) (field3_16) (field4_16) (field5_1) (field6_6) (field7_2) (field8_1) ..... это одно ведро (8 полей) данных. мы получим 20 ведер за раз, то есть всего 160 полей. мне нужно принять значения field3, field7 & fields8 на основе предопределенного условия. если входным аргументом является N, тогда возьмите три поля из первого сегмента, и если это Y, мне нужно взять три поля из любого другого ведра, кроме 1-го. если 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;
}

Ответы [ 2 ]

1 голос
/ 09 октября 2009

Мне кажется, что этот код очень хрупкий. Это может хорошо работать, если дать хороший ввод (я не предлагаю проверять это для вас), но если дать некоторые неверные данные, он либо рухнет и сгорит, либо даст неверные результаты.

Проверяли ли вы неожиданные данные? Например:

  • Предположим, что i_balance_info равно нулю?
  • Предположим, что i_balance_info равен ""?
  • Предположим, что во входной строке содержится менее 8 элементов, что будет делать эта строка кода?

    memcpy(o_balance_sign,str[7],1);
    
  • Предположим, что элемент в строке [3] имеет длину менее 16 символов, что будет делать эта строка кода?

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

Мой подход к написанию такого кода состоял бы в защите от всех подобных ситуаций. По крайней мере, я бы добавил операторы ASSERT (), я обычно писал бы явную проверку ввода и возвращал ошибки, когда это плохо. Проблема здесь заключается в том, что интерфейс, по-видимому, не допускает вероятность неправильного ввода.

0 голосов
/ 09 октября 2009

Мне было трудно читать ваш код, но я добавил несколько комментариев, 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 )

(или что вы пытаетесь сделать)

...