RattleInGlasses / ps_oop

0 stars 0 forks source link

Замечания по программе Replace #1

Open alexey-malov opened 9 years ago

alexey-malov commented 9 years ago
//return 0 if str1 == str2
//return 1 if str1 != str2
int StrCompare(char *pStr1, char *pStr2)

Лучше использовать для таких случаев тип bool и функции дать имя, отвечающее на вопрос "Да" или "Нет",

alexey-malov commented 9 years ago
//return lenght of string or -1 if the length is too big to store it in int
signed int StrLen(char* str)
{
    int i = 0;
    while (str[i] != 0)
    {
        if (i++ == UINT_MAX)
        {
            return -1;
        }
    }
    return i;
}

Есть функция strlen, свой велосипед, в данном случае, не нужен. Функция не будет корректно работать при длине строки в диапазоне (INT_MAX; UINT_MAX) Для длины контейнеров следует использовать тип size_t, который и вовращается strlen (на 64 битных платформах size_t будет 64 битным, а инт - 32)

alexey-malov commented 9 years ago
bool ByteReplace(FILE *pInput, char *const pSearchStr, char *const pReplaceStr, FILE *const pOutput)
{
    signed int buffSize = StrLen(pSearchStr) + 1;
    if (buffSize == 0)
    {
        //error
        return true;
    }

       ...

    free(pStrBuff);
    return false;
}

В случае успеха функции обычно возвращают true, а при неуспехе - false.

Такое код читается естественнее:

if (CopyFile(..., ...))
{
    DoSomething();
}

чем: if (!CopyFile(..., ...)) // Читается как: если не удалось скопировать файл, то сделай что-то { DoSomething(); }

alexey-malov commented 9 years ago
signed int buffSize = StrLen(pSearchStr) + 1;

тип int знаковый по умолчанию всегда. Только тип char может быть беззнаковым либо знаковым в зависимости от настроек компилятора

alexey-malov commented 9 years ago

StrCompare можно тоже не изобретать. Есть strcmp

alexey-malov commented 9 years ago
void CheckArgc(int argc)
{
    if (argc < 2)
    {
        printf(MSG_DESCRIPTION);
        exit(0);
    }
    if (argc < 3)
    {
        printf(MSG_ERR_NOT_ENOUGH_ARGUMENTS);
        exit(1);
    }
    if (argc > 5)
    {
        printf(MSG_ERR_TOO_MANY_ARGUMENTS);
        exit(1);
    }
}

exit лучше не использовать. Вот почему:

alexey-malov commented 9 years ago

80*0,6=48 баллов

alexey-malov commented 9 years ago
    FILE *pInputFile;
    FILE *pOutputFile;
    if (!OpenFile(argv[1], "rb", MSG_ERR_NO_INPUT, &pInputFile)
        || (!OpenFile(argv[2], "wb", MSG_ERR_NO_OUTPUT, &pOutputFile)))
    {
        return 1;
    }

Если такой код будет в часто используемой функции, то при ошибке открытия выходного файла, входной закрыт не будет. При разработке на C++ лучше использовать файловые потоки. Либо в случае использовать FILE*, добавить явное закрытие во всех случаях выхода из функции

alexey-malov commented 9 years ago

80*0,8 = 64