A Clean Code-ról nem elég beszélni, tudni kell meglátni a lehetőségeket és használni a kódunkban. Ehhez lehetne kamu, szánékosan rossz kódokat készíteni, ami aztán kijavítva gyönyörű lesz, de ez még mindig nem az élet. Választásom egy régi kedvenc frameworkömre esett, a Codeigniterre. Amiért érdekes ez a project, hogy igazán régi rendszerről beszélünk. Az EllisLab 2006-ban adta ki az első verzióját, amit aztán 2014-ben “ajándékba adott” a BCIT-nek. Jelenleg a 4-es verziónál tart, aminek már PHP 7.2 a minimuma, de lássuk be, nem kihívás, ha nem egy eredendően 5.x verziós kódot kellene a hétköznapokban karbantartani.

Ez a sorozat a Codeigniter 3 repositoryból fog szemezgetni. Leszögezném, hogy nem az a cél, hogy szétfikázzuk a rendszert, hisz öregebb a Clean Code könyvnél is, de nagyon jó alapot ad, mit lehet/kell kezdeni egy legacy kóddal, ha szeretnénk egy sokkal olvashatóbb és karbantarthatóbb rendszert kapni. Kezdjük is a system/core-on belül lévő Config.php átnézésével.

system/core/Config.php⌗

__construct⌗ 


public function __construct()
{
    $this->config =& get_config();

    // Set the base_url automatically if none was provided
    if (empty($this->config['base_url'])) {
        if (isset($_SERVER['SERVER_ADDR'])) {
            if (strpos($_SERVER['SERVER_ADDR'], ':') !== FALSE) {
                $server_addr = '['.$_SERVER['SERVER_ADDR'].']';
            }
            else {
                $server_addr = $_SERVER['SERVER_ADDR'];
            }

            $base_url = (is_https() ? 'https' : 'http').'://'.$server_addr
                .substr($_SERVER['SCRIPT_NAME'], 0, strpos($_SERVER['SCRIPT_NAME'], basename($_SERVER['SCRIPT_FILENAME'])));
        }
        else {
            $base_url = 'http://localhost/';
        }

        $this->set_item('base_url', $base_url);
    }

    log_message('info', 'Config Class Initialized');
}

Problémák:⌗

  • túl mély (3 if)
  • felesleges else használatok
  • comment, beszédes kód helyett

A __construct felelős a config betöltésért és ha nem lenne base_url, feltölti azt.
Hogyan kezdjünk hozzá a tisztításhoz? Szervezzük ki a bonyolult részeket:


public function __construct()
{
    $this->config =& get_config();
    $this->handle_config_base_url();

    log_message('info', 'Config Class Initialized');
}

protected function handle_config_base_url()
{
    if (empty($this->config['base_url'])) {
        if (isset($_SERVER['SERVER_ADDR'])) {
            if (strpos($_SERVER['SERVER_ADDR'], ':') !== FALSE) {
                $server_addr = '[' . $_SERVER['SERVER_ADDR'] . ']';
            } else {
                $server_addr = $_SERVER['SERVER_ADDR'];
            }

            $base_url = (is_https() ? 'https' : 'http') . '://' . $server_addr
                . substr($_SERVER['SCRIPT_NAME'], 0, strpos($_SERVER['SCRIPT_NAME'], basename($_SERVER['SCRIPT_FILENAME'])));
        } else {
            $base_url = 'http://localhost/';
        }

        $this->set_item('base_url', $base_url);
    }
}
A __construct megtisztult, a comment helyét átvette egy beszédes metódus, de a problémánk még megmaradt a handle_config_base_url metódusban.
Az új metódus azért felelős, hogy ellenőrizze van-e base_url a configban és ha nincs kalkulálja ki és állítsa be. Ez így 3 feladat egy metódusnak.
Ezek a feladatok legyenek a legkisebb egységek: 

protected function handle_config_base_url()
{
    if (empty($this->config['base_url'])) {
        $base_url = $this->calculate_base_url();
        $this->set_item('base_url', $base_url);
    }
}

protected function calculate_base_url()
{
    if (isset($_SERVER['SERVER_ADDR'])) {
        if (strpos($_SERVER['SERVER_ADDR'], ':') !== FALSE) {
            $server_addr = '[' . $_SERVER['SERVER_ADDR'] . ']';
        } else {
            $server_addr = $_SERVER['SERVER_ADDR'];
        }

        $base_url = (is_https() ? 'https' : 'http') . '://' . $server_addr
            . substr($_SERVER['SCRIPT_NAME'], 0, strpos($_SERVER['SCRIPT_NAME'], basename($_SERVER['SCRIPT_FILENAME'])));
    } else {
        $base_url = 'http://localhost/';
    }
    return $base_url;
}
A handle_config_base_url 3 sorban elintézi a szükséges feladatokat, természetesen ezek a sorok lehetnének külön metódusok (has_base_url, calculate_base_url, save_base_url), de ezek már így is elemi szinten vannak.
Törekedjünk minél kevesebb else ágat használni, ezek legtöbb esetben egy kis átszervezéssel feleslegessé válnak.
A calculate_base_url metódusban 2 else águnk van és mindkettő csak azért, hogy 1-1 változót külön-külön adjon meg.
Ezeket a változókat az if elé mozgatva feltölthetjük ‘default’ értékkel (az else ág tartalmával): 

protected function calculate_base_url()
{
    $base_url = 'http://localhost/';
    if (isset($_SERVER['SERVER_ADDR'])) {
        $server_addr = $_SERVER['SERVER_ADDR'];
        if (strpos($_SERVER['SERVER_ADDR'], ':') !== FALSE) {
            $server_addr = '[' . $_SERVER['SERVER_ADDR'] . ']';
        }

        $base_url = (is_https() ? 'https' : 'http') . '://' . $server_addr
            . substr($_SERVER['SCRIPT_NAME'], 0, strpos($_SERVER['SCRIPT_NAME'], basename($_SERVER['SCRIPT_FILENAME'])));
    }
    return $base_url;
}
Amikor egy változót csak azért töltünk fel, hogy az aztán visszatérjen, akkor a változó cserélhető return hívásra.
Ilyenkor az utolsó return definiálja a ‘default’ visszatérést: 

protected function calculate_base_url()
{
    if (isset($_SERVER['SERVER_ADDR'])) {
        $server_addr = $_SERVER['SERVER_ADDR'];
        if (strpos($_SERVER['SERVER_ADDR'], ':') !== FALSE) {
            $server_addr = '[' . $_SERVER['SERVER_ADDR'] . ']';
        }

        return (is_https() ? 'https' : 'http') . '://' . $server_addr
            . substr($_SERVER['SCRIPT_NAME'], 0, strpos($_SERVER['SCRIPT_NAME'], basename($_SERVER['SCRIPT_FILENAME'])));
    }
    return 'http://localhost/';
}
Itt még a $server_addr lekérése kiszervezhető egy külön metódusba, ami pár sorral ismét egyszerűsíti a calculate_base_url-t. Szintén alkalmazható a változó->return egyszerűsítés: 

protected function calculate_base_url()
{
    if (isset($_SERVER['SERVER_ADDR'])) {
        $server_addr = $this->get_server_address();

        return (is_https() ? 'https' : 'http') . '://' . $server_addr
            . substr($_SERVER['SCRIPT_NAME'], 0, strpos($_SERVER['SCRIPT_NAME'], basename($_SERVER['SCRIPT_FILENAME'])));
    }
    return 'http://localhost/';
}

protected function get_server_address()
{
    if (strpos($_SERVER['SERVER_ADDR'], ':') !== FALSE) {
        return '[' . $_SERVER['SERVER_ADDR'] . ']';
    }
    return $_SERVER['SERVER_ADDR'];
}

load()⌗ 


public function load($file = '', $use_sections = FALSE, $fail_gracefully = FALSE)
{
    $file = ($file === '') ? 'config' : str_replace('.php', '', $file);
    $loaded = FALSE;

    foreach ($this->_config_paths as $path) {
        foreach (array($file, ENVIRONMENT.DIRECTORY_SEPARATOR.$file) as $location) {
            $file_path = $path.'config/'.$location.'.php';
            if (in_array($file_path, $this->is_loaded, TRUE)) {
                return TRUE;
            }

            if ( ! file_exists($file_path)) {
                continue;
            }

            include($file_path);

            if ( ! isset($config) OR ! is_array($config)) {
                if ($fail_gracefully === TRUE) {
                    return FALSE;
                }

                show_error('Your '.$file_path.' file does not appear to contain a valid configuration array.');
            }

            if ($use_sections === TRUE) {
                $this->config[$file] = isset($this->config[$file])
                    ? array_merge($this->config[$file], $config)
                    : $config;
            }
            else {
                $this->config = array_merge($this->config, $config);
            }

            $this->is_loaded[] = $file_path;
            $config = NULL;
            $loaded = TRUE;
            log_message('info', 'Config file loaded: '.$file_path);
        }
    }

    if ($loaded === TRUE) {
        return TRUE;
    }
    elseif ($fail_gracefully === TRUE) {
        return FALSE;
    }

    show_error('The configuration file '.$file.'.php does not exist.');
}

Problémák:⌗

  • túl hosszú, nehezen átlátható
  • túl sok if
  • túl sok return

A load metódus minden lehetséges config helyről megpróbálja betölteni a még nem betöltött configot, amit ellenőriz és végül tárolja a $this->configban, egy metóduson belül…
Mikor egy metódust kell módosítani, mindig jusson eszünkbe, hogy ha ehhez unit tesztet kéne írni, akkor annak kétszer annyi tesztje kell legyen, mint amennyi elágazása van, feltéve, ha nincsenek egymásba ágyazva az if-ek. A foreach-ek szintén újabb és újabb eseteket hoznak be a tesztbe, ezért ezeket is minimalizálni kell.
A 2 foreach és a második if azért felelős, hogy csak valid fájl legyen betöltve. Ezeket szervezzük ki egy külön metódusba, amit aztán egyetlen foreach segítségével be tudunk járni: 


public function load($file = '', $use_sections = FALSE, $fail_gracefully = FALSE)
{
    $file = ($file === '') ? 'config' : str_replace('.php', '', $file);
    $loaded = FALSE;

    foreach ($this->get_config_paths($file) as $file_path) {
        if (in_array($file_path, $this->is_loaded, TRUE)) {
            return TRUE;
        }
        include($file_path);

        if (!isset($config) or !is_array($config)) {
            if ($fail_gracefully === TRUE) {
                return FALSE;
            }

            show_error('Your ' . $file_path . ' file does not appear to contain a valid configuration array.');
        }

        if ($use_sections === TRUE) {
            $this->config[$file] = isset($this->config[$file])
                ? array_merge($this->config[$file], $config)
                : $config;
        } else {
            $this->config = array_merge($this->config, $config);
        }

        $this->is_loaded[] = $file_path;
        $config = NULL;
        $loaded = TRUE;
        log_message('info', 'Config file loaded: ' . $file_path);
    }

    if ($loaded === TRUE) {
        return TRUE;
    } elseif ($fail_gracefully === TRUE) {
        return FALSE;
    }

    show_error('The configuration file ' . $file . '.php does not exist.');
}

protected function get_config_paths($file)
{
    $paths = [];
    foreach ($this->_config_paths as $path) {
        foreach (array($file, ENVIRONMENT . DIRECTORY_SEPARATOR . $file) as $location) {
            $file_path = $path . 'config/' . $location . '.php';
            if (!file_exists($file_path)) {
                continue;
            }
            $paths[] = $file_path;
        }
    }
    return $paths;
}
Ezek után szervezzük ki a config letárolását: 

public function load($file = '', $use_sections = FALSE, $fail_gracefully = FALSE)
{
    $file = ($file === '') ? 'config' : str_replace('.php', '', $file);
    $loaded = FALSE;

    foreach ($this->get_config_paths($file) as $file_path) {
        if (in_array($file_path, $this->is_loaded, TRUE)) {
            return TRUE;
        }
        include($file_path);

        if (!isset($config) or !is_array($config)) {
            if ($fail_gracefully === TRUE) {
                return FALSE;
            }

            show_error('Your ' . $file_path . ' file does not appear to contain a valid configuration array.');
        }

        $this->update_config($file, $config, $use_sections);

        $this->is_loaded[] = $file_path;
        $config = NULL;
        $loaded = TRUE;
        log_message('info', 'Config file loaded: ' . $file_path);
    }

    if ($loaded === TRUE) {
        return TRUE;
    } elseif ($fail_gracefully === TRUE) {
        return FALSE;
    }

    show_error('The configuration file ' . $file . '.php does not exist.');
}

protected function update_config(array $file, array $config, $use_sections)
{
    if ($use_sections === TRUE) {
        $this->config[$file] = isset($this->config[$file])
            ? array_merge($this->config[$file], $config)
            : $config;
    } else {
        $this->config = array_merge($this->config, $config);
    }
}
Következő lépésben a config tényleges kinyerését és validálását is mozgassuk ki: 

public function load($file = '', $use_sections = FALSE, $fail_gracefully = FALSE)
{
    $file = ($file === '') ? 'config' : str_replace('.php', '', $file);
    $loaded = FALSE;

    foreach ($this->get_config_paths($file) as $file_path) {
        if (in_array($file_path, $this->is_loaded, TRUE)) {
            return TRUE;
        }

        if (null !== ($config = $this->get_config_from_path($file_path,$fail_gracefully))){
            $this->update_config($file, $config, $use_sections);
            $this->is_loaded[] = $file_path;
            $loaded = TRUE;
            log_message('info', 'Config file loaded: ' . $file_path);
        }
    }

    if ($loaded === TRUE) {
        return TRUE;
    } elseif ($fail_gracefully === TRUE) {
        return FALSE;
    }

    show_error('The configuration file ' . $file . '.php does not exist.');
}

protected function get_config_from_path($file_path,$fail_gracefully)
{
    $config = null;
    include($file_path);

    if (!is_array($config) && TRUE !== $fail_gracefully) {
        show_error('Your ' . $file_path . ' file does not appear to contain a valid configuration array.');
    }
    return $config;
}
Végül még jöhet pár csinosítás:

  • az is_loaded félrevezető. Az is vagy has kezdetű metódusoknak bool-t kell visszaadni, ez itt egy tömb, ami elérési utakat tartalmaz. Legyen helyette loaded_configs
  • ha egy változót hasonlítunk össze egy statikus tartalommal, akkor kerüljön előre a statikus rész (yoda condition). Ha véletlen a 2 vagy 3 egyenlőség helyett csak 1 kerül közéjük, nem fog tudni módosulni a változó tartalma
  • amennyiben if-ben return használunk, nincs szükség elseif-re, egyszerűen cserélhető if-re. 
    
public function load($file = '', $use_sections = FALSE, $fail_gracefully = FALSE)
{
    $file = ($file === '') ? 'config' : str_replace('.php', '', $file);
    $loaded = FALSE;

    foreach ($this->get_config_paths($file) as $file_path) {
        if (in_array($file_path, $this->loaded_configs, TRUE)) {
            return TRUE;
        }

        if (null !== ($config = $this->get_config_from_path($file_path,$fail_gracefully))){
            $this->update_config($file, $config, $use_sections);
            $this->loaded_configs[] = $file_path;
            $loaded = TRUE;
            log_message('info', 'Config file loaded: ' . $file_path);
        }
    }

    if (TRUE === $loaded) {
        return TRUE;
    }
    if (TRUE === $fail_gracefully) {
        return FALSE;
    }

    show_error('The configuration file ' . $file . '.php does not exist.');
}

slash_item⌗ 


public function slash_item($item)
{
    if ( ! isset($this->config[$item])) {
        return NULL;
    }
    elseif (trim($this->config[$item]) === '') {
        return '';
    }

    return rtrim($this->config[$item], '/').'/';
}

Problémák:⌗

  • az elseif cserélhető if-re, ha returnt tartalmaz az if rész
  • yoda condition

public function slash_item($item)
{
    if ( ! isset($this->config[$item])) {
        return NULL;
    }
    if ('' === trim($this->config[$item])) {
        return '';
    }

    return rtrim($this->config[$item], '/').'/';
}

site_url / base_url⌗ 


public function site_url($uri = '', $protocol = NULL)
{
    $base_url = $this->slash_item('base_url');

    if (isset($protocol)) {
        // For protocol-relative links
        if ($protocol === '') {
            $base_url = substr($base_url, strpos($base_url, '//'));
        }
        else {
            $base_url = $protocol.substr($base_url, strpos($base_url, '://'));
        }
    }

    if (empty($uri)) {
        return $base_url.$this->item('index_page');
    }

    $uri = $this->_uri_string($uri);

    if ($this->item('enable_query_strings') === FALSE) {
        $suffix = isset($this->config['url_suffix']) ? $this->config['url_suffix'] : '';

        if ($suffix !== '') {
            if (($offset = strpos($uri, '?')) !== FALSE) {
                $uri = substr($uri, 0, $offset).$suffix.substr($uri, $offset);
            }
            else {
                $uri .= $suffix;
            }
        }

        return $base_url.$this->slash_item('index_page').$uri;
    }
    elseif (strpos($uri, '?') === FALSE) {
        $uri = '?'.$uri;
    }

    return $base_url.$this->item('index_page').$uri;
}


public function base_url($uri = '', $protocol = NULL)
{
    $base_url = $this->slash_item('base_url');

    if (isset($protocol)) {
        // For protocol-relative links
        if ($protocol === '') {
            $base_url = substr($base_url, strpos($base_url, '//'));
        }
        else {
            $base_url = $protocol.substr($base_url, strpos($base_url, '://'));
        }
    }

    return $base_url.$this->_uri_string($uri);
}

Problémák:⌗

  • a 2 metódusban duplikáció van
  • a site_url hosszú és bonyolult

Elöször is kerüljön ki a duplikáció. A site_url első pár sora teljesen kiváltható a base_url hívással: 


public function site_url($uri = '', $protocol = NULL)
{
    $base_url = base_url('',$protocol);

    if (empty($uri)) {
        return $base_url.$this->item('index_page');
    }

    $uri = $this->_uri_string($uri);

    if ($this->item('enable_query_strings') === FALSE)
    {
        $suffix = isset($this->config['url_suffix']) ? $this->config['url_suffix'] : '';

        if ($suffix !== '') {
            if (($offset = strpos($uri, '?')) !== FALSE) {
                $uri = substr($uri, 0, $offset).$suffix.substr($uri, $offset);
            }
            else {
                $uri .= $suffix;
            }
        }

        return $base_url.$this->slash_item('index_page').$uri;
    }
    elseif (strpos($uri, '?') === FALSE) {
        $uri = '?'.$uri;
    }

    return $base_url.$this->item('index_page').$uri;
}

Mivel a kód nagyrésze az uri manipulálással foglalkozik, így ez kerüljön ki külön metódusba, ami már csak a megtisztított urit adja vissza: 


public function site_url($uri = '', $protocol = NULL)
{
    $base_url = base_url('',$protocol);

    if (empty($uri)) {
        return $base_url.$this->item('index_page');
    }

    return $base_url.$this->item('index_page').$this->get_site_url_uri($uri);
}

protected function get_site_url_uri($uri)
{
    $uri = $this->_uri_string($uri);

    if ($this->item('enable_query_strings') === FALSE) {
        $suffix = isset($this->config['url_suffix']) ? $this->config['url_suffix'] : '';

        if ($suffix !== '') {
            if (($offset = strpos($uri, '?')) !== FALSE) {
                $uri = substr($uri, 0, $offset) . $suffix . substr($uri, $offset);
            } else {
                $uri .= $suffix;
            }
        }

        return '/' . $uri;
    }
    if (strpos($uri, '?') === FALSE) {
        $uri = '?' . $uri;
    }

    return $uri;
}
A get_site_url_uri még kicsit zajos a suffix kezeléstől, ezt is szervezzük ki: 

protected function get_site_url_uri($uri)
{
    $uri = $this->_uri_string($uri);

    if ($this->item('enable_query_strings') === FALSE) {
        $uri = $this->get_suffixed_uri($uri);

        return '/' . $uri;
    }
    if (strpos($uri, '?') === FALSE) {
        $uri = '?' . $uri;
    }

    return $uri;
}

protected function get_suffixed_uri($uri)
{
    $suffix = isset($this->config['url_suffix']) ? $this->config['url_suffix'] : '';

    if ('' !== $suffix) {
        if (($offset = strpos($uri, '?')) !== FALSE) {
            return substr($uri, 0, $offset) . $suffix . substr($uri, $offset);
        }
        $uri .= $suffix;
    }
    return $uri;
}

_uri_string⌗ 


protected function _uri_string($uri)
{
    if ($this->item('enable_query_strings') === FALSE) {
        is_array($uri) && $uri = implode('/', $uri);
        return ltrim($uri, '/');
    }
    elseif (is_array($uri)) {
        return http_build_query($uri);
    }

    return $uri;
}

Problémák⌗

  • az if nélküli $uri módosítás kicsit fura
  • az elseif cserélhető if-re vagy az utolsó return-nél kiváltható ?: használatával

protected function _uri_string($uri)
{
    if ($this->item('enable_query_strings') === FALSE) {
        if (is_array($uri)) {
            $uri = implode('/', $uri);
        }
        return ltrim($uri, '/');
    }
    return is_array($uri) ? http_build_query($uri) : $uri;
}

A módosított Config.php innen tölthető le. A Clean Codehoz az is hozzátartozik, hogy a fájlokban azonos formázást és szintaktikát használjunk.
Ezt a refaktorálás jegyében olymódon megtörtem, hogy a Codeigniter a block tagokat mindig új sorban kezdi, ami kényelmetlenül hosszítja a beillesztett kódokat.
A további refaktorálások is ezt a formázást fogják követni, így tekinthetjük ezt egy új refaktorálási szabálynak, ami minden új kódra kötelezően alkalmazandó.