VPF::Оцените код Си - Форум программистов

gggl

Дата 18.11.2011, 16:50 (ссылка)

(нет голосов)

Загрузка ...

Новичок

Профиль
Группа: Участник
Сообщений: 19
Регистрация: 22.9.2007

Репутация: нет
Всего: нет

Посмотрите, пожалуйста, хорошо ли оформлен код, может есть какие неточности?

Задание было: Реализовать структуру данных хеш-массив на основе CRC32, метод решения коллизий: связный список

Код


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

#include <stddef.h>
#include <stdint.h>
/*-----------------------------------------------------------------------------*/
#define TABLE_SIZE 40
#define MAX_STR_SIZE 25
/*-----------------------------------------------------------------------------*/
uint_least32_t CRC32(unsigned char *buf, size_t len)
{
    uint_least32_t crc_table[256];
    uint_least32_t crc; int i, j;
 
    for (i = 0; i < 256; i++)
    {
        crc = i;
        for (j = 0; j < 8; j++)
            crc = crc & 1 ? (crc >> 1) ^ 0xEDB88320UL : crc >> 1;
 
        crc_table[i] = crc;
    };
 
    crc = 0xFFFFFFFFUL;
 
    while (len--) 
        crc = crc_table[(crc ^ *buf++) & 0xFF] ^ (crc >> 8);
 
    return crc ^ 0xFFFFFFFFUL;
}
/*-----------------------------------------------------------------------------*/
struct hash_list
{
    char data[MAX_STR_SIZE];
    struct hash_list *next;
};
/*-----------------------------------------------------------------------------*/
void list_store(struct hash_list *node, char *data)
{
    if (!node->next)
    {
        node->next = calloc(1, sizeof(struct hash_list));
        if (!node->next)
        {
            printf("Ошибка при распределении памяти #2\n");
            exit(1);
        }                
        strcpy(node->next->data,data);            
    }    
    else
    {
        list_store(node->next, data); 
    }

}
/*-----------------------------------------------------------------------------*/
void show(struct hash_list *list)
{
    unsigned i;
    for(i = 0; i <= TABLE_SIZE; ++i)
    {    
        if (list[i].data[0] == '\0') continue;
        show_node(&list[i],&i);
    }

}

void show_node(struct hash_list *node, unsigned *key)
{
    if (!node->next) printf("%u : %s; \n",*key,node->data);
    else
    {
        printf("%u : %s -> ",*key,node->data);            
        show_node(node->next, key);
    }
}
/*-----------------------------------------------------------------------------*/
void free_mem(struct hash_list *list)
{
    unsigned i;
    for(i = 0; i <= TABLE_SIZE; ++i)
    {    
        if (!list[i].next) continue;
        free_node(&list[i]);
    }
    free(list);
}

void free_node(struct hash_list *node)
{        
    if (!node->next->next) free(node->next);
    else
    {
        free_node(node->next);
        free(node->next);
    }
}
/*-----------------------------------------------------------------------------*/

int main(void)
{
    char file_name[255];
    char buf[MAX_STR_SIZE];
    buf[0] = '\0';
    FILE *fp;
    unsigned collision = 0;    

    unsigned utmp;    

    struct hash_list *list = calloc(TABLE_SIZE, sizeof(struct hash_list));
    if(!list) 
    {
        printf("Ошибка при распределении памяти #1\n");
        exit(1);
    }

    //printf("Ведите имя файла\n");    
    //gets(file_name);        
    if((fp=fopen("/home/mor/expire/hash_string/CRL.words","r")) == NULL)
    {
        printf("Ошибка при открытии файла.\n");
        exit(1);
    }

    while (!feof(fp))
    {
        fscanf(fp,"%s",buf);        
        if(buf[0]=='\0') continue;        
        utmp = (CRC32(buf,strlen(buf))) % TABLE_SIZE;    
        if (list[utmp].data[0] != '\0')
        {
            ++collision;
            list_store(&list[utmp],buf);
        }
        else
        {
            strcpy(list[utmp].data,buf);
        }

        buf[0] = '\0';
    }

    
    
    //printf("%u\n",CRC32(buf,0));
    
    show(list);
    printf("Количество коллизий: %u\n",collision);    


    fclose(fp);
    free_mem(list);


    return 0;
}

Это сообщение отредактировал(а) gggl - 19.11.2011, 16:58

bsa

Дата 18.11.2011, 17:38 (ссылка)

(голосов:1)

Загрузка ...

Эксперт

Профиль
Группа: Модератор
Сообщений: 9185
Регистрация: 6.4.2006
Где: Москва, Россия

Репутация: 85
Всего: 196

Не вижу ни одной причины, по которой необходимы вложенные функции. Тем более, что в общем случае их вызов значительно сложнее, чем вызов обычных. Поэтому, вынеси их наружу.
Не пиши на одной строчке больше одного действия (касается в первую очередь конструкций if(...) continue;).
Давай переменным осмысленные названия. l - плохое название для переменной. Короткие имена имеет смысл давать только переменным-счетчикам циклов и пр. Причем в том случае, если нет других переменных с короткими именами. Иначе будешь путаться.

--------------------

Правильно заданный вопрос - половина ответа
Отзывы о работе модератора.

baldina

Дата 18.11.2011, 17:52 (ссылка)

(нет голосов)

Загрузка ...

Эксперт

Профиль
Группа: Завсегдатай
Сообщений: 3433
Регистрация: 5.12.2007
Где: Москва

Репутация: 15
Всего: 101

неплохо оформлена функция CRC32(). не хуже, чем в википедии ;-)

gggl

Дата 18.11.2011, 19:39 (ссылка)

(нет голосов)

Загрузка ...

Новичок

Профиль
Группа: Участник
Сообщений: 19
Регистрация: 22.9.2007

Репутация: нет
Всего: нет

bsa

Поправил код в первом посте; переименовал list в hash_list а l в list лучше, что то, несмог придумать; насчет continue мне кажется выглядит как то не очень красиво

Код


if (list[i].data[0] == '\0')
    continue;

вынес функции наружу, но поивились странные ворнинги, хотя вроде все работает

Код


main.c: In function ‘list_store’:
main.c:48: warning: incompatible implicit declaration of built-in function ‘strcpy’
main.c: At top level:
main.c:68: warning: conflicting types for ‘show_node’
main.c:63: note: previous implicit declaration of ‘show_node’ was here
main.c:89: warning: conflicting types for ‘free_node’
main.c:84: note: previous implicit declaration of ‘free_node’ was here
main.c: In function ‘main’:
main.c:129: warning: incompatible implicit declaration of built-in function ‘strlen’
main.c:137: warning: incompatible implicit declaration of built-in function ‘strcpy’

baldina
А зачем изобретать велосипед smile

Это сообщение отредактировал(а) gggl - 18.11.2011, 22:45

fish9370

Дата 19.11.2011, 13:04 (ссылка)

(нет голосов)

Загрузка ...

Опытный

Профиль
Группа: Участник
Сообщений: 663
Регистрация: 15.4.2007
Где: Москва

Репутация: 1
Всего: 1

Цитата(gggl @ 18.11.2011, 16:50

)

struct hash_list *list =(struct hash_list*) calloc(TABLE_SIZE, sizeof(struct hash_list));
if(!list)     {
    printf("Ошибка при распределении памяти #1\n");
    exit(1);
}

1) в Си при выделении памяти, не принято приводить к какому-либо типу данных
2) поскольку ошибка выделения памяти всегда критическия, выделение памяти обычно заворачивают в обертку, где выполняются все проверки

что-то типа того:

Код


#define MALLOC_FAILURE_MSG \
        x_verbose(LOG_DEBUG, "Memory Allocation Failure in function %s at line %d of %s\n", func, lineno, file);
/*!
 * \brief A wrapper for malloc()
 *
 * x_malloc() is a wrapper for malloc() that will generate an log
 * message in the case that the allocation fails.
 *
 * The argument and return value are the same as malloc()
 */

#define x_malloc(len) \
        _x_malloc((len), __FILE__, __LINE__, __PRETTY_FUNCTION__)

inline void *attribute_malloc _x_malloc(size_t len, const char *file, int lineno, const char *func)
{
        void *p;

        if (!(p = malloc(len)))
                MALLOC_FAILURE_MSG;

        return p;
}

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

--------------------

undefined

gggl

Дата 19.11.2011, 16:53 (ссылка)

(нет голосов)

Загрузка ...

Новичок

Профиль
Группа: Участник
Сообщений: 19
Регистрация: 22.9.2007

Репутация: нет
Всего: нет

fish9370
Вы предлагаете мне написать свою inline функцию для выделения памяти?
а (1) исправил, на счет стиля оформления, так велел препод..

ну и всетаки мне не нравятся поивившиеся ворнинги, откуда они могли взяться?

Это сообщение отредактировал(а) gggl - 19.11.2011, 16:57

gggl

Дата 19.11.2011, 17:25 (ссылка)

(нет голосов)

Загрузка ...

Новичок

Профиль
Группа: Участник
Сообщений: 19
Регистрация: 22.9.2007

Репутация: нет
Всего: нет

И еще как правильно

Код


if (!node->next)
    printf("%u : %s; \n",*key,node->data);
else
{
    printf("%u : %s -> ",*key,node->data);            
    show_node(node->next, key);
}

или фигурные скобки обязательны?

Код


if (!node->next)
{
    printf("%u : %s; \n",*key,node->data);
}
else
{
    printf("%u : %s -> ",*key,node->data);            
    show_node(node->next, key);
}

alexvs11

Дата 19.11.2011, 18:15 (ссылка)

(нет голосов)

Загрузка ...

hell is here

Профиль
Группа: Участник
Сообщений: 518
Регистрация: 21.8.2010

Репутация: 2
Всего: 10

читабельней второй вариант

fish9370

Дата 19.11.2011, 18:20 (ссылка)

(нет голосов)

Загрузка ...

Опытный

Профиль
Группа: Участник
Сообщений: 663
Регистрация: 15.4.2007
Где: Москва

Репутация: 1
Всего: 1

Цитата(gggl @ 19.11.2011, 16:53

)

Вы предлагаете мне написать свою inline функцию для выделения памяти?

нет, я просто рассказал как обычно делают, в этом случае

Цитата(gggl @ 19.11.2011, 17:25

)

или фигурные скобки обязательны?

это на твое усмотрение

мне больше нравится так:

Код


if (node->next) {
    printf("%u : %s -> ",*key,node->data);            
    show_node(node->next, key);
} else
    printf("%u : %s; \n",*key,node->data);

обычно в проекте есть специальный документ, с названием что-то типа CODING STYLE, в котором и описываются правила оформления кода

--------------------

undefined

math64

Дата 19.11.2011, 22:27 (ссылка)

(нет голосов)

Загрузка ...

Эксперт

Профиль
Группа: Завсегдатай
Сообщений: 2505
Регистрация: 12.4.2007

Репутация: 12
Всего: 72

ворнинги из-за того, что:
1. Нет #include <string.h>
2. Eсли функция вызывается до её определения, нужно сделать предварительное объявление.
Чтобы не думать - делается объавление всех функций, можно с вынесением в заголовок.
3. Результат malloc/calloc лучше привести к нужному типу - если придётся компилировать C++, будет Ok.
4. Если в if части есть фигурные скобки, в else ставь тоже и наоборот.
Если есть if внутри if - ставь фигурные скобки, иначе если понадобится else, он присоединится к внутреннему if.

fish9370

Дата 20.11.2011, 17:15 (ссылка)

(нет голосов)

Загрузка ...

Опытный

Профиль
Группа: Участник
Сообщений: 663
Регистрация: 15.4.2007
Где: Москва

Репутация: 1
Всего: 1

Цитата(math64 @ 19.11.2011, 22:27

)

3. Результат malloc/calloc лучше привести к нужному типу - если придётся компилировать C++, будет Ok.

я все же настаиваю, что в Си так не принято. Если понадобится потом скомпилить с Си++ на это есть extern "C".

--------------------

undefined

1 Пользователей читают эту тему (1 Гостей и 0 Скрытых Пользователей)
0 Пользователей:
« Предыдущая тема \| C/C++: Для новичков \| Следующая тема »