OberonCore
https://forum.oberoncore.ru/

Задача: преобразовать неструктурный код в структурный
https://forum.oberoncore.ru/viewtopic.php?f=27&t=2571		 Страница 1 из 1
Автор:  jackbauer [ Понедельник, 19 Апрель, 2010 04:30 ]
Заголовок сообщения:  Задача: преобразовать неструктурный код в структурный
Выделено: viewtopic.php?p=32457#p32457
Сергей Губанов писал(а):
Return из середины процедуры, кстати, тоже всегда может (и должен) быть преобразован в цепочку ЕСЛИ-ИНАЧЕ-ЕСЛИ.

Сегодня на работе отрефакторил две процедуры говнокода (каждая была по несколько сотен строк и насыщенна под завязку return-ами из середины).
...


Интересно, как бы преобразовал следующий код (break-ов нет, но есть goto - идея похожая). Заодно если кто хочет попытаться переложить его на вложенные if-ы, то это тоже приветствуется.

Условие: фунция не должна использовать исключения, ну и высокоуровневые C++ конструкции тоже, а то уж очень просто получится. Функция read_file делает простую вещь: считывает весь файл в content, content_size - размер считанных данных.
Код:
bool read_file(const char * file_name, char ** content, size_t * content_size)
{
    size_t cont_size = 0;
    char * cont = NULL;
    const size_t buf_size = 1024;
    char * buf = NULL;
    FILE * fh = NULL;
    size_t read_size = 0;
    bool success = false;

    buf = new (nothrow) char[buf_size];
    if (!buf)  goto error;

    fh = fopen(file_name, "r");
    if (!fh)  goto error;

    while (size_t bytes_read = fread(buf, 1, buf_size, fh))
    {
        if (read_size + bytes_read + 1 > cont_size)
        {
            size_t new_size = 2 * cont_size > cont_size + bytes_read + 1
                              ? 2 * cont_size
                              : cont_size + bytes_read + 1;
            char * new_cont = new (nothrow) char[new_size];
            if (!new_cont)  goto error;

            memcpy(new_cont, cont, read_size);
            delete [] cont;
            cont = new_cont;
            cont_size = new_size;
        }

        memcpy(cont + read_size, buf, bytes_read);
        read_size += bytes_read;
    }

    if (ferror(fh) != 0)  goto error;

    cont[read_size++] = '\0';
    *content = cont;
    *content_size = read_size;
    cont = NULL;
    success = true;

error:
    if (fh)
        fclose(fh);

    delete [] cont;
    delete [] buf;

    return success;
}
Автор:  Александр Ильин [ Понедельник, 19 Апрель, 2010 05:24 ]
Заголовок сообщения:  Re: Народные ополченцы изобрели новый способ кодить
Цитата:
goto error
...
error: return success
Оригинально : )
Автор:  jackbauer [ Понедельник, 19 Апрель, 2010 05:32 ]
Заголовок сообщения:  Re: Народные ополченцы изобрели новый способ кодить
Александр Ильин писал(а):
Цитата:
goto error
...
error: return success
Оригинально : )


Что такого? success - это bool: да или нет. В реальной жизни там что-то вроде error_code, где 0 - success, остальное failure. Еще в тему: никогда не встречал ERROR_SUCCESS?
Автор:  Александр Ильин [ Понедельник, 19 Апрель, 2010 05:48 ]
Заголовок сообщения:  Re: Народные ополченцы изобрели новый способ кодить
jackbauer писал(а):
Еще в тему: никогда не встречал ERROR_SUCCESS?
Встречал, и это забавно.
Оригинально здесь то, что метка называется "error", но выполняется в любом случае. Почему не "exit" или "cleanup"? : )

Ещё вопрос: нельзя ли было заранее узнать размер файла и выделить соответствующий буфер сразу?
Автор:  jackbauer [ Понедельник, 19 Апрель, 2010 05:54 ]
Заголовок сообщения:  Re: Народные ополченцы изобрели новый способ кодить
Александр Ильин писал(а):
jackbauer писал(а):
Еще в тему: никогда не встречал ERROR_SUCCESS?
Встречал, и это забавно.
Оригинально здесь то, что метка называется "error", но выполняется в любом случае. Почему не "exit" или "cleanup"? : )


можно и cleanup. Не это главное.

Цитата:
Ещё вопрос: нельзя ли было заранее узнать размер файла и выделить соответствующий буфер сразу?


fread при чтении из файла в текстовом режиме преобразует \n в платформно зависимую комбинацию (\r\n, например). Поэтому размер файла будет отличаться от количесва считанных байтов.
Автор:  Александр Ильин [ Понедельник, 19 Апрель, 2010 06:02 ]
Заголовок сообщения:  Re: Народные ополченцы изобрели новый способ кодить
jackbauer писал(а):
Интересно, как бы преобразовал следующий код (break-ов нет, но есть goto - идея похожая). Заодно если кто хочет попытаться переложить его на вложенные if-ы, то это тоже приветствуется.
Вот моя попытка. Предупреждаю сразу, на C я писал очень давно. : )
Код:
bool read_file(const char * file_name, char ** content, size_t * content_size)
{
    size_t cont_size = 0;
    char * cont = NULL;
    const size_t buf_size = 1024;
    char * buf = NULL;
    FILE * fh = NULL;
    size_t read_size = 0;
    bool success = false;

    buf = new (nothrow) char[buf_size];
    if (buf)
    {
        fh = fopen(file_name, "r");
        if (fh)
        {
            success = true;
            // предполагается, что логические выражения вычисляются сокращённо
            while (success && (size_t bytes_read = fread(buf, 1, buf_size, fh)))
            {
                if (read_size + bytes_read + 1 > cont_size)
                {
                    size_t new_size = 2 * cont_size > cont_size + bytes_read + 1
                                      ? 2 * cont_size
                                      : cont_size + bytes_read + 1;
                    char * new_cont = new (nothrow) char[new_size];
                    success = new_cont != NULL;
                    if (success)
                    {
                        memcpy(new_cont, cont, read_size);
                        delete [] cont;
                        cont = new_cont;
                        cont_size = new_size;
                    }
                }
                if (success)
                {
                    memcpy(cont + read_size, buf, bytes_read);
                    read_size += bytes_read;
                }
            }

            success = success && (ferror(fh) == 0);
            if (success)
            {
                cont[read_size++] = '\0';
                *content = cont;
                *content_size = read_size;
            }
            else
            {
                delete [] cont;
            }
            fclose(fh);
        }
        delete [] buf;
    }
    return success;
}
В цикле появился дополнительный if, его можно убрать, продублировав код либо вынеся его в отдельную (инлайн-)функцию:
Код:
            while (success && (size_t bytes_read = fread(buf, 1, buf_size, fh)))
            {
                if (read_size + bytes_read + 1 > cont_size)
                {
                    size_t new_size = 2 * cont_size > cont_size + bytes_read + 1
                                      ? 2 * cont_size
                                      : cont_size + bytes_read + 1;
                    char * new_cont = new (nothrow) char[new_size];
                    success = new_cont != NULL;
                    if (success)
                    {
                        memcpy(new_cont, cont, read_size);
                        delete [] cont;
                        cont = new_cont;
                        cont_size = new_size;
                        inlined_copy_buf_to_cont; // тут
                    }
                }
                else
                {
                    inlined_copy_buf_to_cont; // и тут
                }
            }
Автор:  jackbauer [ Понедельник, 19 Апрель, 2010 06:45 ]
Заголовок сообщения:  Re: Народные ополченцы изобрели новый способ кодить
Александр Ильин писал(а):
Вот моя попытка. Предупреждаю сразу, на C я писал очень давно. : )


Попытка засчитывается :). А теперь с точки зрения человека, который этот код видит впервые, в каком из вариантов более ясно прослеживается алгоритм работы функции?
Автор:  Александр Ильин [ Понедельник, 19 Апрель, 2010 08:06 ]
Заголовок сообщения:  Re: Народные ополченцы изобрели новый способ кодить
jackbauer писал(а):
А теперь с точки зрения человека, который этот код видит впервые, в каком из вариантов более ясно прослеживается алгоритм работы функции?
Пока народ подтягивается и формирует своё мнение, добавлю следующее замечание. На мой взгляд, наибольшей ясности можно добиться грамотной декомпозицией. Например, функция read_file должна бы выглядеть примерно так:
Код:
bool read_file(const char * file_name, char ** content, size_t * content_size)
{
    bool success = false;
    FILE * fh = fopen(file_name, "r");
    if (fh)
    {
        success = read_file_hnd (&fh, &content, &content_size); // за правильность передачи параметров не ручаюсь
        fclose(fh);
    }
    return success;
}
Провести такую декомпозицию невозможно до тех пор, пока в коде пристутствуют break/goto между уровнями вложенности блоков. Когда прыжки между уровнями устранены, любой вложенный уровень можно оформить в виде отдельной функции и дать ей осмысленное имя. При визуальной обозримости кода выделение в отдельную функцию может быть не обязательно, но читающий всё равно может мысленно сформулировать предназначение каждого блока, в том числе в виде пред- и постусловий. Именно таковая возможность, на мой взгляд, является главным преимуществом структурной записи алгоритма.
Автор:  Валерий Лаптев [ Понедельник, 19 Апрель, 2010 08:08 ]
Заголовок сообщения:  Re: Народные ополченцы изобрели новый способ кодить
jackbauer писал(а):
Цитата:
Ещё вопрос: нельзя ли было заранее узнать размер файла и выделить соответствующий буфер сразу?


fread при чтении из файла в текстовом режиме преобразует \n в платформно зависимую комбинацию (\r\n, например). Поэтому размер файла будет отличаться от количесва считанных байтов.

Вот неправда ваша!
fread ничего не преобразует. Эта функция считывает из файла ровно столько байтов, сколько указано.
Преобразование выполняется только для файлов, открытых как текстовые.
Код:
FILE * fh = fopen(file_name, "r");

Это открыто как текстовый. Открывайте ка двоичные - и будет вам счастье...
Код:
FILE * fh = fopen(file_name, "r[b]b[/b]");

Это открыто как двоичный.
Автор:  jackbauer [ Понедельник, 19 Апрель, 2010 08:11 ]
Заголовок сообщения:  Re: Народные ополченцы изобрели новый способ кодить
Валерий Лаптев писал(а):
jackbauer писал(а):
Цитата:
Ещё вопрос: нельзя ли было заранее узнать размер файла и выделить соответствующий буфер сразу?


fread при чтении из файла в текстовом режиме преобразует \n в платформно зависимую комбинацию (\r\n, например). Поэтому размер файла будет отличаться от количесва считанных байтов.

Вот неправда ваша!
fread ничего не преобразует. Эта функция считывает из файла ровно столько байтов, сколько указано.
Преобразование выполняется только для файлов, открытых как текстовые. Открывайте ка двоичные - и будет вам счастье...


fread при чтении из файла в текстовом режиме...

Мне нужно открывать именно в текстовом.
Автор:  Валерий Лаптев [ Понедельник, 19 Апрель, 2010 08:13 ]
Заголовок сообщения:  Re: Народные ополченцы изобрели новый способ кодить
jackbauer писал(а):
fread при чтении из файла в текстовом режиме...

Вы ж все равно читаете байты. Ну и открывайте в двоичном режиме - тогда никаких преобразований не будет.
Автор:  Илья Ермаков [ Понедельник, 19 Апрель, 2010 10:36 ]
Заголовок сообщения:  Re: Народные ополченцы изобрели новый способ кодить
Схема с просчётом success на каждом шаге - и далее навешиванием охраны на следующий:

IF success THEN
...
success :=
END;
IF success THEN
...

- используется часто.

Александр предложил, смотрю, вариант, когда расчёт success выносится между IF: success := success & ...
Наверное, это лучше.
Автор:  jackbauer [ Вторник, 20 Апрель, 2010 03:54 ]
Заголовок сообщения:  Re: Народные ополченцы изобрели новый способ кодить
Александр Ильин писал(а):
jackbauer писал(а):
А теперь с точки зрения человека, который этот код видит впервые, в каком из вариантов более ясно прослеживается алгоритм работы функции?
Пока народ подтягивается и формирует своё мнение, добавлю следующее замечание. На мой взгляд, наибольшей ясности можно добиться грамотной декомпозицией. Например, функция read_file должна бы выглядеть примерно так:
Код:
bool read_file(const char * file_name, char ** content, size_t * content_size)
{
    bool success = false;
    FILE * fh = fopen(file_name, "r");
    if (fh)
    {
        success = read_file_hnd (&fh, &content, &content_size); // за правильность передачи параметров не ручаюсь
        fclose(fh);
    }
    return success;
}
Провести такую декомпозицию невозможно до тех пор, пока в коде пристутствуют break/goto между уровнями вложенности блоков. Когда прыжки между уровнями устранены, любой вложенный уровень можно оформить в виде отдельной функции и дать ей осмысленное имя. При визуальной обозримости кода выделение в отдельную функцию может быть не обязательно, но читающий всё равно может мысленно сформулировать предназначение каждого блока, в том числе в виде пред- и постусловий. Именно таковая возможность, на мой взгляд, является главным преимуществом структурной записи алгоритма.


Так как ты упомянул "грамотную" декомпозицию, то значит ли это, что упомянутый здесь ранее метод вложенных if-ов ты считаешь неприемлимым?

А потом, почему ты остановился в декомпозиции только на первом уровне, read_file_hnd? Можно продолжить и дальше, не так ли? Внутри read_file_hnd() создадим буфер и вызовем еще одну функцию теперь уже с четырьмя параметрами: read_file_hnd_with_buffer(fh, buffer, content, content_size). Тут уже не так красиво получается, как хотелось бы.

Представим, что у нас последовательность действий не такая короткая, как в этом примере, а немного подлиннее, но все равно линейная:
1. получить ресурс1
2. создать ресурс2 с использованием ресурс1
3. получить ресурс3
4. создать ресурс4 с использованием ресурс2 и ресурс3
5. создать ресурс5 с использованием ресурс1
6. проделать некоторые операции с использованием ресурс3, 4 и 5.

Каждое из этих действий может завершиться с ошибкой, при которой нужно освободить все до этого полученные ресурсы, и выйти из функции. Если следовать твоему методу с созданием дополнительных функций на каждом шаге, то на шаге 2 у тебя будет еще одна функция с одним дополнительным параметром, на шаге 3 - двумя (нужно протащить ресурс1 и 2), на шаге 4, 5 и 6 - тремя. Допустим, что первоначальных параметра было 3, функция на последнем шаге будет иметь 6 параметров.

Все эти дополнительные функции, естественно, лишние, потому что перед нами простая, линейная последовательность действий.

Теперь допустим, что требования немного поменялись. На шаге 3 нам не нужно больше создавать ресурс3. Получается, что мы не только должны изменить реализацию, но и интерфейсы всех функций, которые следуют за шагом 3.

Поэтому с практической точки зрения, твой подход не работает, или как еще можно сказать, не масштабируется: с увеличением количества шагов резко возрастает количество действий, которые программист должен проделать, чтобы написать или внести изменение в код.
Автор:  jackbauer [ Вторник, 20 Апрель, 2010 04:06 ]
Заголовок сообщения:  Re: Народные ополченцы изобрели новый способ кодить
Илья Ермаков писал(а):
Схема с просчётом success на каждом шаге - и далее навешиванием охраны на следующий:

IF success THEN
...
success :=
END;
IF success THEN
...

- используется часто.


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

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

Цитата:
Александр предложил, смотрю, вариант, когда расчёт success выносится между IF: success := success & ...
Наверное, это лучше.


Тот же самый вопрос. С точки зрения человека, который этот код видит впервые, в каком из вариантов более ясно прослеживается алгоритм работы функции?
Автор:  Александр Ильин [ Вторник, 20 Апрель, 2010 07:14 ]
Заголовок сообщения:  Re: Народные ополченцы изобрели новый способ кодить
jackbauer писал(а):
Представим, что у нас последовательность действий не такая короткая, как в этом примере, а немного подлиннее, но все равно линейная:
1. получить ресурс1
2. создать ресурс2 с использованием ресурс1
3. получить ресурс3
4. создать ресурс4 с использованием ресурс2 и ресурс3
5. создать ресурс5 с использованием ресурс1
6. проделать некоторые операции с использованием ресурс3, 4 и 5.
Честно говоря, предыдущий пример был сложнее из-за выпрыгивания из цикла. Здесь всё гораздо проще.
Код:
PROCEDURE GetResourcesForXXX (VAR res3, res4, res5: Resource): BOOLEAN;
VAR
   res1, res2: Resource;
   success: BOOLEAN;
BEGIN
   InitRes1 (res1);
   InitRes2 (res2);
   success := GetRes1 (res1)
      & GetRes2 (res2, res1)
      & GetRes3 (res3)
      & GetRes4 (res4, res2, res3)
      & GetRes5 (res5, res1);
   FreeRes1 (res1);
   FreeRes2 (res2);
   RETURN success
END GetResourcesForXXX;

PROCEDURE XXX;
VAR res3, res4, res5: Resource;
BEGIN
   InitRes3 (res3);
   InitRes4 (res4);
   InitRes5 (res5);
   IF GetResourcesForXXX (res3, res4, res5) THEN
      DoXXX (res3, res4, res5); (* это не обязательно отдельная процедура, просто некий код, использующий ресурсы *)
   END;
   FreeRes3 (res3);
   FreeRes4 (res4);
   FreeRes5 (res5);
END XXX;
Процедура GetResourcesForXXX добавлена исключительно ради локализации ресурсов res1 и res2 - таким образом подчёркивается, что они не используются в DoXXX, да и освобождаются пораньше. Если сильно хочется, всё можно слепить в одну процедуру тривиальным рефакторингом.

Предполагается, что не является ошибкой освобождение ресурса, для которого выполнен Init, но не выполнен Get (или Get завершился с ошибкой). Таково подавляющее большинство ресурсов, в том числе в вашем примере: если buf == NULL, то delete [] buf ничего не делает и не выбрасывает ошибку. В Delphi если объект obj: TObject = nil, то obj.Free не вызывает деструктор и не выбрасывает исключение (в отличие от obj.Destroy, например). В WinApi CloseHandle (NULL) ничего не делает, и так далее. Другими словами, InitRes может заключаться просто в присвоении NULL сразу же при объявлении идентификатора (в языке Си), и всё.

Если же для некоторый ресурс следует освобождать только после успешного Get, то добавляется простейшая охрана, как в вашем примере: if (fh) fclose (fh).

Наконец, помните, что это не обязательно код, это прежде всего схема. Читабельность кода - это во многом легкость узнавания схемы. Поэтому не обязательно должны присутствовать дополнительные процедуры, часто достаточно поделить код на блоки краткими комментариями.
jackbauer писал(а):
Теперь допустим, что требования немного поменялись. На шаге 3 нам не нужно больше создавать ресурс3. Получается, что мы не только должны изменить реализацию, но и интерфейсы всех функций, которые следуют за шагом 3.
Выяснилось, что функция только одна, да и то не обязательно её создавать. Если же эту процедуру сделать локальной (т.е. поместить внутрь PROCEDURE XXX), то изменения в её интерфейсе никого не коснутся в принципе. Если же процедуру GetResourcesForXXX использует кто-то ещё помимо XXX, то её интерфейс необходимо изменить, так как интерфейс отражает требования к задаче XXX, и эти требования изменились.

Допустим, res3 стал не нужен. Удаляем переменную res3 из процедуры XXX, компилируем. По ошибкам компилятора легко вычищается всё остальное.
jackbauer писал(а):
Так как ты упомянул "грамотную" декомпозицию, то значит ли это, что упомянутый здесь ранее метод вложенных if-ов ты считаешь неприемлимым?
Я предпочитаю обращение на "вы". Глубокий уровень вложенности затрудняет чтение, поскольку становится трудно сопоставить начало блока и его конец, тем более если блок занимает более одного экрана по вертикали. Метод считаю приемлемым, но начиная с определённой глубины вложенности лучше перейти в отдельную процедуру (опять же, если не тянется слишком много параметров, и если процедура может пригодиться кому-то ещё для повторного использования). Это путь компромиссов, как и всё искусство дизайна.
jackbauer писал(а):
Поэтому с практической точки зрения, твой подход не работает, или как ещё можно сказать, не масштабируется: с увеличением количества шагов резко возрастает количество действий, которые программист должен проделать, чтобы написать или внести изменение в код.
К сожалению, не вижу данного вывода в свете моего ответа. Более того, этот вывод представляется мне несколько странным по сути, ведь на этом же основании следует отказаться от документирования ПО и написания тестов: слишком много работы при изменении интерфейсов или постановки задачи.

Как практикующий программист, добавлю: у меня этот подход работает.
Автор:  Илья Ермаков [ Вторник, 20 Апрель, 2010 07:22 ]
Заголовок сообщения:  Re: Народные ополченцы изобрели новый способ кодить
jackbauer писал(а):
Тот же самый вопрос. С точки зрения человека, который этот код видит впервые, в каком из вариантов более ясно прослеживается алгоритм работы функции?


Этот вопрос уже обсуждался.
Некорректно сравнивать "вообще человека" и грамотно действующего человека.
Первого не интересует очень многое, что интересует второго - и ему, по большому счёту, почти наплевать, какой вариант читать. Он изучает программу с точки зрения того, как она будет выполняться ("изображает из себя компьютер").
Второй не делает мысленных прогонов, а смотрит на "разрезы" между операторами - и ему нужно представлять, какие же утверждения о состоянии там выполнены. Ваша неструктурщина ему очень сильно портит возможность изучения.
Представьте, что у Вас есть фотография, где стеганографией закодированы какие-то данные. Вы смотрите на фотографию как фотографию - и рассуждаете "а вот подкорректирую я её цветность в фотошопе, ведь стало лучше". Что при этом скрытые (и главные) данные уничтожены - Вы даже не в курсе.

Вот в этой статье в пункте "Работа с текстом или конструкцией?" эта проблема рассмотрена:
viewtopic.php?f=67&t=2574
Страница 1 из 1 Часовой пояс: UTC + 3 часа
Powered by phpBB® Forum Software © phpBB Group
https://www.phpbb.com/