2

Code Reviewで以前にいくつかの推奨事項を実装しました。また、ポインターを使用してコードを改善しました。しかし、以下のアドレスインクリメントの部分はどこが悪いのsqueezed_str++でしょうか? アドレスがインクリメントされていないようです。お知らせ下さい。

PS。substring() 関数は機能しています。:)

char *squeeze (char *str, int start_index, int end_index, char *ref_str) {
     char *substr;
     substr = malloc (sizeof (*substr));
     if (substr == NULL) {
          printf ("Unable to allocate memory.\n");
          exit (EXIT_FAILURE);
     }

     char *squeezed_str;
     squeezed_str = malloc (sizeof (*squeezed_str));
     if (squeezed_str == NULL) {
          printf ("Unable to allocate memory!\n");
          exit (EXIT_FAILURE);
     }

     substr = substring (str, start_index, end_index);
     int substr_len = strlen (substr);
     int refstr_len = strlen (ref_str);

     char chr1, chr2; chr1 = chr2 = '\0';

     for (int i = 0; i < substr_len; i++) {
          chr1 = *(substr+i);
          for (int j = 0; j < refstr_len; j++) {
               chr2 = *(ref_str + j);
               if (chr1 == chr2) {
                    break;
               }
          }
          if (chr1 != chr2) {
               *squeezed_str = *(substr+i);
               squeezed_str++;
          }     
     }

     return squeezed_str;
 } /* end of squeeze() */
4

2 に答える 2

2

テストされていないインラインアドバイスによるいくつかの改善：

char *squeeze (char *str, int start_index, int end_index, char *ref_str) {
     char *substr = substring (str, start_index, end_index);
         //do not malloc here!, you are doing an assignment later on, so memory leak
         //infact might as well move assignment right here

     char *squeezed_str;
     squeezed_str = malloc (strlen(str)+1); //+ 1 one for null terminator
     if (squeezed_str == NULL) {
          printf ("Unable to allocate memory!\n");
          exit (EXIT_FAILURE);
     }

     int substr_len = strlen (substr);
     int refstr_len = strlen (ref_str);
     int squeezeStrIdx = 0;

     for (int i = 0; i < substr_len; i++) {
          char chr1 = substr[i]; //using index accessing is nicer
                                 //moved the scope of the variable in
          char chr2 = '\0'; //reduced variable scope, benefit is now resets each loop
          for (int j = 0; j < refstr_len; j++) {
               chr2 = ref_str[j];
               if (chr1 == chr2) {
                    break;
               }
          }
          if (chr1 != chr2) {
               squeezed_str[squeezeStrIdx] = chr1;
               squeezeStrIdx++; //if you modify squeezed_str,
                                //how do you expect to return it at end of method?
          }     
     }

     //this is to null terminate the squeezed_str
     squeezed_str[squeezeStrIdx] = '\0';

     return squeezed_str;
 } /* end of squeeze() */
于 2012-11-03T09:16:28.833 に答える
1

完全に欠陥のあることの 1 つは、まったく変更することsqueezed_strです。これは によって返されるアドレスmallocです。free変更すると、後でそのメモリ領域にアクセスできなくなります。メモリリークが発生したり、さらに悪いことに、最後に呼び出したときにクラッシュが発生したりfreeしました。

編集：

 char *squeeze (const char *str, size_t start_index, size_t end_index, const char *ref_str) 
 /* You don't modify `str` and `ref_str` so it is a good idea to make it visible in the signature of the function, whence the const qualifiers */
 {
 char *substr;   
 /* Snip the memory leak */

 char *squeezed_str = malloc (sizeof (*squeezed_str));
 if (!squeezed_str) {
      fprintf (stderr, "Unable to allocate memory!\n");  /* Error are to be written on stderr */
      exit (EXIT_FAILURE);     /* Exitting the app in a function is not good style */
 }

 substr = substring (str, start_index, end_index);
 size_t substr_len = strlen (substr);       /* strlen returns size_t not int */
 size_t refstr_len = strlen (ref_str);

 char chr1 = 0, chr2 = 0;
 char *squeezed_copy = squeezed_str;       /* We use a second pointer to not lose the malloc'ed area */
 for (size_t i = 0; i < substr_len; i++) {
      chr1 = substr[i];                     /* This syntax is simpler (you'll see when you have nested structures */
      for (size_t j = 0; j < refstr_len; j++) {
           chr2 = ref_str[j];
           if (chr1 == chr2) {
                break;
           }
      }
      if (chr1 != chr2)
           *squeezed_copy++ = substr[i];
 }
 /* Add the NUL sentinel */
 *squeezed_copy++ = 0;

 return squeezed_str;
} /* end of squeeze() */

これは、アルゴリズムが意図したとおりに機能するかどうかを確認していないことを示しています。直感strstr的には、を使用する方が良いと思います。

于 2012-11-03T09:17:46.130 に答える