метод isPermutation

Я написал метод isPermute. Могу ли я получить советы и рекомендации по улучшению стиля кодирования?

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <stdbool.h>

bool isPermute(char*, char*);
char* sort(char*);

int main(int argv, char **argc)
{
  printf("%s and %s are %spermutations of each other. \n", argc[1], argc[2], isPermute(argc[1], argc[2]) ? 
    "" : "not ");

  return 0;
}

bool isPermute(char* s1, char* s2)
{

  int size1 = strlen(s1);
  int size2 = strlen(s2);

  //default cases
  if((size1 != size2) || (size1 == 0) || (size2==0))
    return false;  

  if(strcmp(sort(s1), sort(s2)))
    return false;

  else
    return true;
}

char* sort(char* str1)
{
  int d=0, size = strlen(str1);
  char character;
  char *original = str1;

  char *result = (char *)malloc(size);

  for ( character = 'a' ; character <= 'z' ; character++ )
  {
     int i;
     for ( i = 0 ; i < size ; i++ )
     {
        if ( str1[i] == character )
        {
           result[d] = str1[i];
           d++;
        }
     }
     str1 = original;
  }
  return result;
}
11 голосов | спросил Jonathan 29 TueEurope/Moscow2015-12-29T02:05:10+03:00Europe/Moscow12bEurope/MoscowTue, 29 Dec 2015 02:05:10 +0300 2015, 02:05:10

5 ответов


12

Прежде всего:

bool isPermute(char*, char*);
char* sort(char*);

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

Соглашение для int main не указано:

int main(int argv, char **argc)

Это обычно противоположное: argc для аргумента count и argv для аргументов, переданных программе.

Итак, это действительно должно быть:

int main(int argc, char **argv)

Далее:

printf("%s and %s are %spermutations of each other. \n", argc[1], argc[2], 
    isPermute(argc[1], argc[2]) ? "" : "not ");

Вы никогда не проверяли, действительно ли программа передала два параметра, и поэтому вместо этого она должна быть (argc - это счетчик arg с этого момента, а argv - аргументы)

if(argc == 3) 
{
    printf("%s and %s are %spermutations of each other. \n", argv[1], argv[2], 
        isPermute(argv[1], argv[2]) ? "" : "not ");
}
else 
{
    printf("Incorrect amount of parameters passed!\n");
    return -1;
}

Следующее:

if(strcmp(sort(s1), sort(s2)))
    return false;
else
    return true;

можно обрезать так:

return !strcmp(sort(s1), sort(s2));

Следующее:

char *result = (char *)malloc(size);

Вам не нужно иметь явный листинг (char *) как malloc возвращает void*, поэтому он может просто быть

char *result = malloc(size); 

Это не умаляет ясности каким-либо реальным способом.

Кроме того, я заметил, что вы объявляете переменные для циклов for вне цикла, хотя это необязательно (если вы не используете C89, и в этом случае игнорируйте это)

Итак, это:

for ( character = 'a' ; character <= 'z' ; character++ )
{
     ...
}

станет

for (char character = 'a'; character <= 'z'; character++)
{
    ...
}

То же самое применимо и для другого цикла for.

for ( i = 0 ; i < size ; i++ )
{ 
    ...
}

станет

for (int i = 0; i < size; i++)
{
    ...
}
ответил mmk 29 TueEurope/Moscow2015-12-29T02:57:12+03:00Europe/Moscow12bEurope/MoscowTue, 29 Dec 2015 02:57:12 +0300 2015, 02:57:12
5

Угловой корпус

Это условие:

if((size1 != size2) || (size1 == 0) || (size2==0))
  return false;

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

Логический обратный анти-шаблон

  if(strcmp(sort(s1), sort(s2)))
    return false;

  else
    return true;

становится:

return ! strcmp(sort(s1), sort(s2))

Он делает то же самое, но короче & проще.

ответил Caridorc 29 TueEurope/Moscow2015-12-29T02:35:30+03:00Europe/Moscow12bEurope/MoscowTue, 29 Dec 2015 02:35:30 +0300 2015, 02:35:30
5

Проблемы с памятью

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

     char *result = (char *)malloc(size);

Конечный нулевой символ не записывается в результат, но требуется для строки результата, но требуется для функции strcmp() для работы правильно.

Алгоритм

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

Расширение алгоритма ко всем символам, кроме нуля, увеличивает время выполнения из-за вложенных циклов из O(size * 26) в O(size * 255), что может быть нежелательным для небольших строк. Посмотри на https://stackoverflow.com/questions/33294426/sort-string-by- character-in-c-programming для сортировки символов с использованием quicksort.

Документация

Функции sort() и isPermute() отсутствуют полная документация о том, что они делают, параметр и его возвращаемые значения. Что-то вроде

    /** sort() takes a zero terminated character string and returns a
    string of the same size with contents sorted lexicographically.
    The returned string has to be deallocated by the caller using free(). */
ответил bshm 29 TueEurope/Moscow2015-12-29T08:35:13+03:00Europe/Moscow12bEurope/MoscowTue, 29 Dec 2015 08:35:13 +0300 2015, 08:35:13
2

эти две строки заставляют компилятор выводить предупреждения о преобразовании в разные типы, потому что strlen() возвращает size_t, а не int

  int size1 = strlen(s1);
  int size2 = strlen(s2);

предложите использовать:

  size_t size1 = strlen(s1);
  size_t size2 = strlen(s2);

для удобочитаемости, следуйте аксиоме: только один оператор на строку и (самое большее) одно объявление переменной для оператора

SO эта строка:

int d=0, size = strlen(str1);

становится:

int d=0;
size_t size = strlen(str1); // remember strlen() returns a 'size_t` not an 'int'

после применения указанных выше изменений, то эти строки:

int i;
for ( i = 0 ; i < size ; i++ )

стать

 for ( size_t i = 0 ; i < size ; i++ )

Всегда проверяйте первый параметр на main (), чтобы убедиться, что в командной строке было правильное количество параметров перед доступом к чему-либо за пределами (в вашем случае) argc[0] И когда в командной строке не было правильного количества параметров, вывод на stderr через fprintf(stderr, ...), как программа должна быть вызвана

Примечание: по соглашению основная подпись записывается следующим образом:

int main( int argc, char *argv[] )

где argc - это COUNT количества параметров командной строки

где argv[] - это VECTOR указателей на символьные строки параметров

ответил user3629249 30 WedEurope/Moscow2015-12-30T05:06:58+03:00Europe/Moscow12bEurope/MoscowWed, 30 Dec 2015 05:06:58 +0300 2015, 05:06:58
2

Речь идет не о стиле кодирования, а об эффективном алгоритме: вместо сортировки символов в строках вы можете просто посчитать их.

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

Две строки являются анаграммами (перестановками) друг друга, если все счетчики равны нулю.

ответил CiaPan 30 WedEurope/Moscow2015-12-30T13:48:22+03:00Europe/Moscow12bEurope/MoscowWed, 30 Dec 2015 13:48:22 +0300 2015, 13:48:22

Похожие вопросы

Популярные теги

security × 330linux × 316macos × 2827 × 268performance × 244command-line × 241sql-server × 235joomla-3.x × 222java × 189c++ × 186windows × 180cisco × 168bash × 158c# × 142gmail × 139arduino-uno × 139javascript × 134ssh × 133seo × 132mysql × 132