Пример рефакторинга «извлечение функции» для алгоритма перезаписи прав доступа

Исходный код

function SetFileAccessPermission($path, $arPermissions, $bOverWrite=true)
{
    CMain::InitPathVars($site, $path);
    $DOC_ROOT = CSite::GetSiteDocRoot($site);

    if(strlen($path) <= 0)
        $path="/";
    if(($p = bxstrrpos($path, "/"))!==false)
    {
        $path_file = substr($path, $p+1);
        $path_dir = substr($path, 0, $p);
    }
    elsereturnfalse;

    if($path_file=="" && $path_dir=="")
        $path_file = "/";

    $PERM = Array();
    if(file_exists($DOC_ROOT.$path_dir."/.access.php"))
        @include($DOC_ROOT.$path_dir."/.access.php");
    
    $FILE_PERM = $PERM[$path_file];
    if(!is_array($FILE_PERM))
        $FILE_PERM=Array();

    if(!$bOverWrite && count($FILE_PERM)>0)
        returntrue;

    $bDiff = false;

    $str="<?\n";
    foreach($arPermissionsas$group=>$perm)
    {
        if(strlen($perm) > 0)
            $str.="\$PERM[\"".$path_file."\"][\"".$group."\"]=\"".str_replace("\"", "\\\"", $perm)."\";\n";
        if(!$bDiff && $FILE_PERM[$group]!=$perm)
            $bDiff=true;
    }

    foreach($PERMas$file=>$arPerm)
    {
        if(strval($file) !==$path_file)
            foreach($arPermas$group=>$perm)
                $str.="\$PERM[\"".$file."\"][\"".$group."\"]=\"".str_replace("\"", "\\\"", $perm)."\";\n";
    }

    if(!$bDiff)
    {
        foreach($FILE_PERMas$group=>$perm)
            if($arPermissions[$group]!=$perm)
            {
                $bDiff==true;
                break;
            }
    }

    $str.="?".">";
    
    $this->SaveFileContent($DOC_ROOT.$path_dir."/.access.php", $str);
    $GLOBALS["CACHE_MANAGER"]->CleanDir("menu");
    unset($this->FILE_PERMISSION_CACHE[$site."|".$path_dir."/.access.php"]);
    
    if($bDiff)
    {
        $db_events = GetModuleEvents("main", "OnChangePermissions");
        while($arEvent = $db_events->Fetch())
            ExecuteModuleEvent($arEvent, Array($site, $path), $arPermissions);
    }
    returntrue;
}

Как можно улучшить исходный код

При виде столь длинной функции у каждого программиста должна рефлекторно возникать мысль: «О, да здесь много ответственностей, сейчас я их поделю!».

Для выполнения рефакторинга воспользуюсь методом составления плана. После напряжённого анализа кода я выявил в коде функции следующие ответственности:

сформировать путь к файлу с закешированными правами доступа $DOC_ROOT.$path_dir."/.access.php";
сформировать путь к файлу, права доступа которого нужно закешировать $path_file;
проверить наличие прав доступа к нужному файлу !$bOverWrite && count($FILE_PERM)>0;
сформировать новый набор права доступа;
обнаружить наличие расхождений между новыми и старыми правами;
записать новые права в соответствующий файл;
сбросить соответствующие кеши;
оповестить подписчиков события об изменении прав доступа.

Цель рефакторинга «извлечение функции» — не просто распихать код, а скрыть реализацию. Одна из глобальных целей этого мероприятия — обеспечить возможность максимально безболезненного изменения реализаций.

Например, если я захочу изменить способ кеширования прав доступа с файла access.php на какой-то другой вариант, мне придётся жёстко переписать одну часть SetFileAccessPermission, связанную с файлом, не сломав другую, связанную с собственно правами доступа. И потом ещё переписать все места, где случаются обращения к этому файлу — а они случаются.

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

Ещё одна важная мысль, которую хочу донести: нельзя смешивать ответственности на том основании, что в их реализации есть похожие участки кода. Например, здесь плотно смешаны алгоритмы формирования пути к файлу с закешированными правами и пути к файлу, права доступа к которому нужно закешировать. Тот факт, что оба проходят черех похожие манипуляции с $path нисколько их не сближает — это всё равно две совершенно разные по смыслу величины, а значит, должны рассчитываться двумя независимыми функциями.

Другая пара смешанных ответственностей: формирование файла с правами доступа и признака изменения прав доступа ($str и $bDiff) — они тоже должны вычисляться раздельно. Мой рецепт: сначала сформировать массив с правами доступа, и только потом отдельно формировать из него файл и отдельно определять наличие изменений. Здесь общим участком является цикл по массиву $arPermissions — но опять же, это не основание для смешивания.

Вариант рефакторинга исходного кода

function SetFileAccessPermission($filePath, $arNewFilePermissions, $bOverWrite=true)
{
    $permissionSubject = GetPermissionSubject($filePath);
    if ($permissionSubject === null) 
        returnfalse;

    $arFullPermissions = ReadPermissions($filePath);

    $arOldFilePermissions = isset($arFullPermissions[$permissionSubject]) ? $arFullPermissions[$permissionSubject] : array();

    if (!empty($arOldFilePermissions) && !$Overwrite) 
        returntrue;
    
    $arFullPermissions[$permissionSubject] = $arNewFilePermissions;
    WritePermissions($filePath, $arFullPermissions);

    if (!PermissionEqual($arNewFilePermissions, $arOldFilePermissions)) {
        NotifyChangePermissions($filePath, $arNewFilePermissions);
    }
}

function ReadPermissions($filePath)
{
    $permissionsFilePath = GetPermissionFilePath($filePath);
    $PERM = Array();
    if(file_exists($permissionsFilePath))
        @include($permissionsFilePath);
    return$PERM;
}

function WritePermissions($filePath, $arFullPermissions)
{
    $permissionsFilePath = GetPermissionFilePath($filePath);
    $permissionsStr = RenderPermissions($arFullPermissions);
    $this->SaveFileContent($permissionsFilePath, $permissionsStr);

    $GLOBALS["CACHE_MANAGER"]->CleanDir("menu");
    unset($this->FILE_PERMISSION_CACHE[]);
}

function RenderPermissions($permissions)
{
    $str="<?\n";
    foreach($permissionsas$file=>$arPerm)
    {
        foreach($arPermas$group=>$perm)
            $str.="\$PERM[\"".$file."\"][\"".$group."\"]=\"".str_replace("\"", "\\\"", $perm)."\";\n";
    }
    return$str;
}

function GetPermissionFilePath($filePath)
{
    CMain::InitPathVars($site, $filePath);
    list($path_dir, $path_file) = SplitPath($filePath);
    $DOC_ROOT = CSite::GetSiteDocRoot($site);
    return$DOC_ROOT.$path_dir."/.access.php";
}

function GetPermissionCacheKey($filePath)
{
    CMain::InitPathVars($site, $filePath);
    list($path_dir, $path_file) = SplitPath($filePath);
    return$site."|".$path_dir."/.access.php";
}

function GetPermissionSubject($path)
{
    CMain::InitPathVars($site, $path);
    list($path_dir, $path_file) = SplitPath($path);
    if ($path_dir === null)
        returnnull;
    return$path_file=="" && $path_dir=="" ? "/" : $path_file;
}

function SplitPath($path)
{
    if(($p = bxstrrpos($path, "/"))!==false)
        returnarray(substr($path, 0, $p), substr($path, $p+1));
    elsereturnarray(null, $path);
}

function PermissionEqual($arNewFilePermissions, $arOldFilePermissions)
{
    foreach($arNewFilePermissionsas$group=>$perm)
        if($arOldFilePermissions[$group]!=$perm)
            returnfalse;

    foreach($arOldFilePermissionsas$group=>$perm)
        if($arNewFilePermissions[$group]!=$perm)
            returnfalse;

    returntrue;
}

function NotifyChangePermissions($filePath, $arNewFilePermissions)
{
    CMain::InitPathVars($site, $filePath);
    $db_events = GetModuleEvents("main", "OnChangePermissions");
    while($arEvent = $db_events->Fetch())
        ExecuteModuleEvent($arEvent, Array($site, $filePath), $arNewFilePermissions);
}

Как нетрудно видеть, отрефакторенный код не соответствует один-в-один первоначально составленному плану: какие-то пункты пришлось разбить на подпункты.

Что ещё можно улучшить

Получившаяся версия функции SetFileAccessPermission всё ещё не идеальна. Она знает слишком много подробностей о том, как устроено кеширование прав доступа, хотя её предназначение — работать с правами доступа к одиночному файлу. Попробую избавить её от лишних знаний.

function SetFileAccessPermission($filePath, $arNewFilePermissions, $bOverWrite=true)
{
    $arOldFilePermissions = GetFilePermissions($filePath);

    if (!empty($arOldFilePermissions) && !$Overwrite) 
        returntrue;
    
    SaveFilePermissions($filePath, $arNewFilePermissions);

    if (!PermissionEqual($arNewFilePermissions, $arOldFilePermissions)) {
        NotifyChangePermissions($filePath, $arNewFilePermissions);
    }
}

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

Предвижу возражения в том плане, что для реализации функций GetFilePermissions и SaveFilePermissions придётся дважды читать файл с диска. Мой ответ: «проблемы индейцев шерифа не волнуют». Высокоуровневая функция не должна заботиться о том, как будут реализованы вызываемые ей низкоуровневые функции. Эта забота и есть та самая преждевременная оптимизация, о вреде которой часто упоминают.

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

При работе «неоптимального» кода в реальных условиях может оказаться, что ясность алгоритмов ценнее тех незначительных накладных расходов, привносимых не всегда «оптимальными» реализациями. Ясность кода — важное условие высокой скорости разработки, и пока что высокая скорость выгоднее для бизнеса, чем дополнительное ядро процессора на сервере.

Реализацию GetFilePermissions и SaveFilePermissions оставляю заинтересованному читателю в качестве упражнения.

Govnokodu.Net

Пример рефакторинга «извлечение функции» для алгоритма перезаписи прав доступа

Исходный код

Как можно улучшить исходный код

Вариант рефакторинга исходного кода

Что ещё можно улучшить

Теория