Отправлено: 04.10.18 17:07. Заголовок: Пример того, как не следует и как следует писать код на C

Данная тема создана мною, чтобы продемонстрировать, как не следует и как следует писать код на C, на примере вопроса начинающего программиста Can't figure out what's wrong with my code :\ (Array subtraction in C) на сайте Stackoverflow.

Как следует из вопроса, требуется написать функцию, которая заполняет массив на основе разницы значений элементов двух других массивов, пока в первом из исходных массивов не встретится значение -1, которое также должно быть записано в целевой массив. Пои этом если во-втором исходном массиве также встречается значение -1, то следует использовать в операции элементы этого массива, начиная снова с нулевого индекса.

Вот как выглядит код начинающего программиста, представленный в вопросе.

 
 void signalclear(int noise[64], int star[64], int clear[64]) { 
     int i = 0; 
  
     while (i < 63) { 
         i++; 
         if (star[ i ] == -1) { 
             continue; 
         } 
         if (noise[ i ] == -1) { 
            break; 
         } 
         clear[ i ] = noise[ i ] - star[ i ]; 
     } 
 } 
 

Очевидно, что код неверный, так как по, крайней мере, доступ к элементам массивов начинается с индекса 1, а не с 0, благодаря предложению в начале цикла

 
 i++; 
 

Также индексация массива star не начинается снова с 0, когда в массиве встретился элемент со значением -1.

К тому же функция не записывает значение -1 в результирующий массив clear, хотя, как следует из комментариев автора вопроса к своему вопросу, это значение также должно быть занесено в результирующий массив. То есть это граничное значение, определяющее число актуальных элементов в результирующем, а также в двух исходных массивах.

Делая рефакторинг кода, начнем с того, что в данном объявлении функции

 
 void signalclear(int noise[64], int star[64], int clear[64]); 
 

магическое число 64 не играет никакой роли и не имеет смысла. Во-первых, так как параметры массивов упорядочиваются компилятором к указателям на элементы массива. То есть, например, данные три объявления функции с одним и тем же именем объявляют одну и ту же функцию

 
 void signalclear( int noise[64], int star[64], int clear[64] ); 
 void signalclear( int noise[10], int star[20], int clear[39] ); 
 void signalclear( int noise[], int star[], int clear[] ); 
 

и эквивалентны следующему объявлению функции

 
 void signalclear( int *noise, int *star, int *clear ); 
 

Во-вторых, так как в массивах используется граничное значение -1, по которому определяется число актуальных элементов в массивах, то размерность самих массивов не имеет значения. Очевидно предполагается, что результирующий массив достаточно велик, чтобы включить в себя все генерируемые элементы на основе двух исходных массивов согласно алгоритму.

Так как массивы noise и star не изменяются в функции, то их следует объявить с квалификатором const.

Далее в C результирующий массив обычно объявляется первым параметром, а исходные массивы объявляются последующими параметрами. Смотрите. например, объявления строковых функций в <string.h>.

Кроме того, желательно, чтобы каждая функция предоставляла как можно больше полезной информации пользователю функции. В данном конкретном случае было бы разумно, чтобы функция возвращала число актуальных элементов, включая граничное значение, в результирующем массиве.

Итак, согласно вышеперечисленным замечаниям, данную функцию следовало бы объявить как

 
 size_t signalclear( int *clear, const int *noise, const int *star );

Относительно используемого в функции алгоритма следует сделать одно замечание. Массив star в общем случае может содержать лишь один актуальный элемент - само граничное значение, то есть -1. В этом случае только оно и следует быть записано в результирующий массив, так как бессмысленно каждый раз возвращаться к индексу 0 для массива star, так как других элементов в массиве нет, и мы всегда будет снова и снова попадать на значение -1 в массиве star.

С учетом сказанного функция может быть определена, как это показано в нижеприведенной демонстрационной программе.

 
 #include <stdio.h> 
  
 size_t signalclear( int *clear, const int *noise, const int *star ) 
 { 
     size_t n = 0; 
      
     if ( star[ 0 ] != -1 ) 
     { 
         for ( size_t i = 0; noise[ n ] != -1; i++, n++ ) 
         { 
             if ( star[ i ] == -1 ) i = 0; 
              
             clear[ n ] = noise[ n ] - star[ i ]; 
         } 
     } 
      
     clear[ n++ ] = -1; 
      
     return n; 
 } 
  
 int main( void ) 
 { 
     enum { N = 64 }; 
     int noise[N] = { 20, 20, 20, 20, 30, 30, 30,-1 }; 
     int star[N]  = {  0,  5, 10, 15, -1 }; 
     int clear[N]; 
      
     size_t n = signalclear( clear, noise, star ); 
      
     printf( "There are %zu elements in the array clear: ", n ); 
      
     size_t i = 0; 
     do 
     { 
         printf( "%d ", clear[ i ] ); 
     } while ( ++i != n ); 
      
     putchar( '\n' ); 
 } 
 

Вывод программы на консоль для указанных в программе тестовых массивов будет:

 
 There are 8 elements in the array clear: 20 15 10 5 30 25 20 -1

Как видно из вывода программы в массиве clear будет ровно столько элементов, сколько актуальных элементов содержится в массиве noise, включая элемент с граничным значением -1.

И последнее замечание к результирующей реализации функции signalclear.

Желательно для граничного значения ввести именованную константу. Например,

 
 size_t signalclear( int *clear, const int *noise, const int *star ) 
 { 
     const int SENTINEL = -1; 
      
     size_t n = 0; 
      
     if ( star[ 0 ] != SENTINEL ) 
     { 
         for ( size_t i = 0; noise[ n ] != SENTINEL; i++, n++ ) 
         { 
             if ( star[ i ] == SENTINEL ) i = 0; 
              
             clear[ n ] = noise[ n ] - star[ i ]; 
         } 
     } 
      
     clear[n++] = SENTINEL; 
      
     return n; 
 } 
 

 Отправлено: 07.10.18 15:22. Заголовок: Хотя следующее обсуж..

Хотя следующее обсуждение плохого и хорошего кода базируется на примере вопроса начинающего программиста, касающегося языка программирования C++ (смотрите вопрос C++ incorrect average results), тем не менее общие принципы написания хорошего кода не меняются.

Автор вопроса просит помочь ему найти ошибку в программе, которая вычисляет среднее значение последовательности целых чисел, введенных с клавиатуры, ограниченной введенным пользователем 0.

Автор вопроса приводит следующий фрагмент кода своей программы

 
 int value; 
 float sum = 0.0; 
 int counter = 0; 
 float average; 
  
 cout << "Enter value " << endl; 
 cin >> value; 
  
 while (value != 0) 
 { 
     cout << "Enter value" << endl; 
     cin >> value; 
     counter++; 
     sum += value; 
 } 
  
 average = sum / counter; 
 cout << "Average = " << average; 
  
  
 return 0; 
 

Основная проблема кода состоит в том, что первое введенное пользователем значение отличное от 0 не участвует в подсчете суммы введенных чисел. С другой стороны введенное значение нуля внутри цикла подсчитывается в общем количестве введенных значений.

Очевидно, что результат вычисления среднего значения будет неверный.

Также в программе игнорируется ситуация, когда пользователь может прервать ввод. Если он сделает это при первом приглашении к вводу, то переменная value будет иметь неопределенное значение.

Возникает вопрос, как правильно оформить цикл по вводу целочисленных значений?

Прежде чем ответить на этот вопрос, интересно посмотреть, что предлагают участники сайта в качестве ответов на исходный вопрос.

Сначала отметим, что переменные следует объявлять в наименьшей области объявления и там, где они непосредственно используются.

Фактически, имеет место бесконечный цикл, пока пользователь не введет 0. Поэтому переменную value следует объявить внутри цикла. Вне цикла ей делать нечего.

Бесконечный цикл можно объявить либо используя предложение цикла for, как, например,

 
 for ( ; ; ) 
 { 
    //... 
 }  
 

Либо используя цикл while

 
 while ( true ) 
 { 
    //... 
 }

Можно также использовать и цикл do-while, как, например,

 
 do 
 { 
    //... 
 } while ( true ); 
 

Но в этом случае такой код менее выразителен, так как читающий код пользователь узнает, что цикл бесконечный, только дойдя до конца конструкции цикла.

Наиболее выразительным будет использовать цикл while ( true ). При этом переменная value будет объявлена и видима только внутри цикла.

Вот как может выглядеть эта итоговая простая программа

 
 #include <iostream> 
  
 int main()  
 { 
     double sum = 0.0; 
     size_t counter = 0; 
      
     while ( true ) 
     { 
         std::cout << "Enter an integer value (0 - exit): "; 
          
         int value; 
          
         if ( not ( std::cin >> value ) or ( value == 0 ) ) break; 
          
         sum += value; 
         ++counter; 
     } 
      
     double average = counter == 0 ? sum : sum / counter; 
      
     std::cout << "Average of the values is " << average << '\n'; 
 } 
 

 Отправлено: 09.10.18 16:34. Заголовок: В продолжение темы м..

В продолжение темы можно рассмотреть следующий вопрос по C начинающего программиста на Stackoverflow Returning common chars.

В вопросе спрашивается как написать функцию, которая записывает в лексикографическом порядке общие символы двух других строк.

Вот реализация функции, представленная в вопросе.

 
 void strIntersect(char *str1, char *str2, char *str3) 
 { 
 int i,j, k; 
 i = 0; 
 j = 0; 
 k = 0; 
 while(str1[ i ]!='\0' || str2[ j ]!='\0') 
 { 
     if(strcmp(str1[ i ],str2[ j ])>0) 
     { 
         str3[ k ] = str1[ i ]; 
         k++; 
     } 
     else if (strcmp(str2[ j ],str1[ i ])>0) 
     { 
         str3[ k ] = str2[ j ]; 
         k++; 
     } 
 i++; 
 j++; 
 } 
 } 
 

Очевидно, что эта реализация функции некорректна по крайней мере потому что благодаря условию цикла

 
 while(str1[ i ]!='\0' || str2[ j ]!='\0')

функция имеет неопределенное поведение, так как в теле функции может иметь место выход за пределы одной из строк. А использование в данном контексте стандартной функции strcmp вообще не имеет смысла.

Интересно отметить, что показанные реализации функции в ответах на данный вопрос также не являются корректными.

Например, в ответе, выбранным автором вопроса как лучший ответ,

 
 int i = 0; 
 int k = 0; 
 while(str1[ i ] != '\0') { 
     int j = 0;  
     while(str2[ j ] != '\0') { 
         if (str1[ i ] == str2[ j ]) { 
            str3[ k ] = str1[ i ]; 
            k++; 
         } 
         j++; 
     } 
     i++; 
 } 
 

результирующая строка, во-первых, в общем случае не будет содержать символы в лексикографическом порядке, и, во-вторых, один и тот же символ может дублироваться в выходной строке.

В другом ответе реализация функции вообще может иметь бесконечный цикл

 
 void strIntersect(char *str1, char *str2, char *str3) 
 { 
   int i=0, j=0, k=0; 
   char commonCharsMap[128] = { 0 }; 
  
   while(str1[ i ] != '\0') 
   { 
     commonCharsMap[str1[ i++ ]] = 1; 
   } 
  
   while(str2[ j ] != '\0') 
   { 
     if(commonCharsMap[str2[ j ]] == 1)  
     { 
       commonCharsMap[str2[j++]] = 2; 
     } 
   } 
  
   for(i=0; i<128; i++) 
   { 
     if(commonCharsMap[ i ] == 2) 
     { 
       str3[k++] = i; 
     } 
   } 
   str3[k++] = '\0'; 
 } 
 

так как в этом цикле

 
   while(str2[ j ] != '\0') 
   { 
     if(commonCharsMap[str2[ j ]] == 1)  
     { 
       commonCharsMap[str2[j++]] = 2; 
     } 
   } 
 

переменная j инкрементируется только при условии выполнения условия в предложении if. Однако, совсем не обязательно, что это условие будет когда-либо выполнено.
Кроме того используется непонятное магическое число 128 в объявлении массива commonCharsMap.

Так как написать такую функцию?

Как ранее было уже замечено в предыдущем сообщении, если строки, заданные в виде параметров функции, не изменяются, то они должны быть объявлены с квалификатором const.

Кроме того имеется негласное соглашение в стандарте C для функций, работающих со строками, что они должны возвращать указатель на результирующую строку, и соответствующий параметр в функции должен стоять на первом местею

Вот как следует объявить функцию:

 
 char * strIntersect( char *s1, const char *s2, const char *s3 );

В реализации этой функции воспользуемся идеей, представленной во втором ответе. Только вместо магического числа 128 будем использовать массив размерностью CHAR_MAX - CHAR_MIN + 1. То есть функция позволит работать с расширенной таблицей ASCII как для типа char, который интерпретируется как signed char, так и для типа cgar, который интерпретируется как unsigned char.

Ниже в демонстрационной программе представлена реализация функции.

 
 #include <stdio.h> 
 #include <limits.h> 
  
 char * strIntersect( char *s1, const char *s2, const char *s3 ) 
 { 
     enum { SIZE = CHAR_MAX - CHAR_MIN + 1 }; 
     char intersection[SIZE] = { '\0' }; 
      
     for ( const char *p = s2; *p; ++p ) 
     { 
         ++intersection[*p - CHAR_MIN]; 
     } 
      
     for ( const char *p = s3; *p; ++p ) 
     { 
         ++intersection[*p - CHAR_MIN]; 
     } 
      
     size_t i = 0; 
     for ( size_t j = 0; j < SIZE; j++ ) 
     { 
         if ( intersection[j] == 2 ) s1[i++] = CHAR_MIN + j; 
     } 
      
     s1[ i ] = '\0'; 
      
     return s1; 
 } 
  
 int main( void ) 
 { 
     const char s1[] = "abcde"; 
     const char s2[] = "dec"; 
     char s3[sizeof( s1 )]; 
      
     puts( s1 ); 
     puts( s2 ); 
     puts( strIntersect( s3, s1, s2 ) ); 
 } 
 

Вывод программы на консоль:

 
 abcde 
 dec 
 cde

Альтернативный подход мог бы включать проверку на то, является ли символ буквенным или цифровым, а также, возможно, пробелом. То есть знаки, которые не являются перечисленными знаками игнорируются в выходной строке. С этой целью следует использовать такие стандартные функции, как isblank и isalnum.

Например

 
 #include <stdio.h> 
 #include <limits.h> 
 #include <ctype.h> 
  
 char * strIntersect( char *s1, const char *s2, const char *s3 ) 
 { 
     enum { SIZE = CHAR_MAX - CHAR_MIN + 1 }; 
     char intersection[SIZE] = { '\0' }; 
      
     for ( const char *p = s2; *p; ++p ) 
     { 
         if ( isalnum( ( unsigned char )*p ) || isblank( ( unsigned char )*p ) ) 
         { 
             ++intersection[*p - CHAR_MIN]; 
         }             
     } 
      
     for ( const char *p = s3; *p; ++p ) 
     { 
         if ( isalnum( ( unsigned char )*p ) || isblank( ( unsigned char )*p ) ) 
         { 
             ++intersection[*p - CHAR_MIN]; 
         }             
     } 
      
     size_t i = 0; 
     for ( size_t j = 0; j < SIZE; j++ ) 
     { 
         if ( intersection[j] == 2 ) s1[i++] = CHAR_MIN + j; 
     } 
      
     s1[ i ] = '\0'; 
      
     return s1; 
 } 
  
 int main( void ) 
 { 
     const char s1[] = "ab cd,e"; 
     const char s2[] = "d,e c"; 
     char s3[sizeof( s1 )]; 
      
     puts( s1 ); 
     puts( s2 ); 
     puts( strIntersect( s3, s1, s2 ) ); 
 } 
 

Вывод программы на консоль

 
 ab cd,e 
 d,e c 
  cde

Можно было бы также объявить эту функцию таким образом, чтобы она имела дополнительный параметр в виде указателя на функцию, которое задает условие, которому должны удовлетворять символы, помещаемые в результирующую строку.