Czysty kod- czy aby nie przesada? Opcje

[daniel1302]

Witam, ostatnio jestem w trakcie lektury książki R.C Martin'a Czysty kod, i jest tam pełno kodu, który rozbijany jest na jedno dwu liniowe przykłady o dość specyficznych opisowych nazwach. Postanowiłem napisać kawałek kodu wg Jego schematu, akurat miałem refaktoryzować model słownika do pewnej aplikacji finansowej, gdzie kod jest fatalny, ale małymi krokami staram się to zmieniać i tak oto napisałem fragmęt:

[PHP] pobierz, plaintext 
<?php
class CriteriumDictionary 
{
    const   TYPE_ACTIVITY   = 0,
            TYPE_SIZE       = 1,
            TYPE_TRADE      = 2,
            TYPE_VIOVODE    = 3,
            TYPE_CAPITAL    = 4,
            TYPE_REGION     = 5,
            TYPE_INCOME     = 6,
            NO_SUBCRITERIUM = 0
    ;
 
    public static $type = [
        self::TYPE_ACTIVITY => 'Działalność',
        self::TYPE_SIZE     => 'Rozmiar',
        self::TYPE_TRADE    => 'Branża',
        self::TYPE_VIOVODE  => 'Województwo',
        self::TYPE_CAPITAL  => 'Kapitał',
        self::TYPE_REGION   => 'Region',
        self::TYPE_INCOME   => 'Przychód'
    ];
 
    public static $activity = [
        1   => 'Usługowa',
        2   => 'Produkcyjna',
        3   => 'Handlowa'
    ];
 
    public static $size = [
        1   => [
            'name' => 'Mała',
            'desc' => 'poniżej 50 pracowników'
        ],
        2   => [
            'name' => 'Średnia',
            'desc' => 'od 50 do 249 pracowników'
        ],
        3   => [
            'name' => 'Duża',
            'desc' => 'od 250 do 500 pracowników'
        ],
        4   => [
            'name' => 'Bardzo duża',
            'desc' => 'powyżej 500 pracowników'
        ]
    ];
 
    public static $trade = [
        1  => 'Bankowość',
        2  => 'Budownictwo',
        3  => 'Energetyka i ciepłownictwo',
        4  => 'Handel',
        5  => 'Logistyka, transport',
        6  => 'Przemysł',
        7  => 'IT i telekomunikacja',
        8  => 'Ubezpieczenia',
        9  => 'Usługi dla ludności',
        10 => 'Usługi dla biznesu',
        11 => 'Outsourcing',
        12 => 'Automotive',
        13 => 'Chemiczna',
        14 => 'Elektroniczna i elektrotechniczna',
        15 => 'Materiały budowlane',
        16 => 'Medyczna i farmaceutyczna',
        17 => 'Metalowa i metalurgiczna',
        18 => 'Spożywcza'
    ];
 
    public static $voivode = [
        1   => 'zachodniopomorskie',
        2   => 'pomorskie',
        3   => 'warmińsko-mazurskie',
        4   => 'podlaskie',
        5   => 'lubuskie',
        6   => 'wielkopolskie',
        7   => 'kujawsko-pomorskie',
        8   => 'mazowieckie',
        9   => 'łódzkie',
        10  => 'lubelskie',
        11  => 'dolnośląskie',
        12  => 'opolskie',
        13  => 'śląskie',
        14  => 'małopolskie',
        15  => 'podkarpackie',
        16  => 'świętokrzyskie'
    ];
 
    public static $capital = [
        1   => 'Przewaga kapitału polskiego (powyżej 50%)',
        2   => 'Przewaga kapitału zagranicznego (powyżej 50%)'
    ];
 
    public static $region = [
        1   => [
            'name'      => 'Wschód',
            'voivodes'  => [10, 15, 4, 16],
        ],
        2   => [
            'name'      => 'Zachód',
            'voivodes'  => [5, 6, 1, 11, 12]
        ],
        3   => [
            'name'      => 'Północ',
            'voivodes'  => [7, 2, 3]
        ],
        4   => [
            'name'      => 'Południe',
            'voivodes'  => [13, 14]
        ],
        4   => [
            'name'      => 'Centrum',
            'voivodes'  => [8, 9]
        ],
    ];
 
    public static $income = [
        1   => 'do 100 mln PLN',
        2   => 'od 100 do 1000 mln PLN',
        3   => 'powyżej 1000 mln PLN'
    ];
 
    public static function get($criterium, $subcriterium=0) {
        self::throwExceptionIfCriteriumIsNotExists((int)$criterium);
        return self::getOneSubcriteriumIfSelectedOrAll((int)$criterium, (int)$subcriterium);
    }
 
    public static function getNames($criterium) {
        self::throwExceptionIfCriteriumIsNotExists((int)$criterium);
 
        return self::extractNamesForCriterium((int)$criterium);
    }
 
    private static function extractNamesForCriterium($criterium) {
        $subcriterias = self::getOneSubcriteriumIfSelectedOrAll((int)$criterium, self::NO_SUBCRITERIUM);
 
        switch ($criterium) {            
            case self::TYPE_SIZE:                  
            case self::TYPE_REGION:     
                $names = [];
                foreach ($subcriterias as $subcriteriumId => $subcriterium) {
                    $names[$subcriteriumId] = $subcriterium['name'];
                }
            default:                    
                $names =& $subcriterias; 
                break;
        }
 
        return $names;
    }
 
    private static function throwExceptionIfCriteriumIsNotExists($criterium) {
        if (self::isCriteriumExists($criterium) === false) {
            throw new Exception('Kryterium(numer '.$criterium.'), które jest wymaganie nie istnieje w słowniku');
        }
    }
 
    private static function getOneSubcriteriumIfSelectedOrAll($criterium, $subcriterium) {
        if (self::isSelectedSubcriterium($subcriterium)) {
            self::throwExceptionIfSubcriteriumIsNotExists($criterium, $subcriterium);
 
            return self::getData($criterium, $subcriterium);
        }
 
        return $criterium;
    }
 
    private static function isSelectedSubcriterium($subcriterium) {
        return $subcriterium > 0;
    }
 
    private function throwExceptionIfSubcriteriumIsNotExists($criterium, $subcriterium) {
        if (isSubcriteriumExists($criterium, $subcriterium) === false) {
            throw new Exception('Subkryterium '.$subcriterium.' dla kryterium '.$criterium.' które jest wymaganie nie istnieje');
        }        
    }
 
    private static function getData($criterium, $subcriterium=0) {
        if (!isset(self::$type[$criterium])) {
            return [];
        }
 
        if (isset(self::$type[$criterium][$subcriterium])) {
            return self::$type[$criterium][$subcriterium];
        }
 
        return self::$type[$criterium];
    }
 
    public static function isCriteriumExists($criterium) {
        return isset(self::$type[$criterium]);
    }
 
    public static function isSubcriteriumExists($criterium, $subcriterium) {
        return isset(self::$type[$criterium]) && isset(self::$type[$criterium][$subcriterium]);
    }
}
[PHP] pobierz, plaintext 

Czy z tymi nazwami to nie przesada? Wzorowałem się na ksiązce czysty kod. Jak wy rozwiązujecie sprawy nazw lepiej dać:

[PHP] pobierz, plaintext 
if (isset($this->accountData[12]) && !empty($this->accountData[12])) { 
//do something 
}
[PHP] pobierz, plaintext 

czy może:

[PHP] pobierz, plaintext 
if ($this->companyNameIsFilled()) {
//do something
}
[PHP] pobierz, plaintext 

Jakie inne dobre praktyki Wy polecacie? Czy takie tworzenie metod to nie przesada?

Ten post edytował daniel1302 9.10.2016, 10:57:27

[kpt_lucek]

@nospor

Zgodzę się z tabelami słownikowymi, pod warunkiem nie trzymania tam AI, poza tym, imo i tak lepiej trzymać stałą w obiekcie, zdecydowanie lepiej jest zrobić warunek:

[PHP] pobierz, plaintext 
class SomeDictionary 
{
    const MIGHTY_PROPERTY = 1;
}
 
if($someObject->someMethod(SomeDictionary::MIGHTY_PROPERTY)){/**/}
[PHP] pobierz, plaintext 

zamiast

[PHP] pobierz, plaintext 
if($someObject->someMethod(1)){/**/}
[PHP] pobierz, plaintext 

Chociażby dlatego, że nie ważne w którym miejscu systemu odwołam się do const'a, to zawsze mam pewność co do jego wartości (zakładając zachowanie wartości const :: id z tabeli), dodatkowo, jeżeli z jakiegoś głupiego powodu chcę zmienić ten ID, to wystarczy zrobić to w miejscu, gdzie go definiuję.

Ten post edytował kpt_lucek 10.10.2016, 08:35:18