Pull to refresh

Comments 43

Должно быть


$tmp = $collection->addFilterByProduct(...);
$tmp = $tmp->addShowInStoresFilter(...);
$tmp = $tmp->addPublicFilter(...);
$tmp = $tmp->addApprovedStatusFilter(...);
$tmp->addCreatedAtLessThanNowFilter(...);
unset($tmp)

для полной (почти) эквивалентности :)


А в целом, "более правильный" нужно заменить "более удобный для отладки отладчиком". :)


P.S. Я сторонник раннего выхода из функции/метода при валидации параметров и других подобных проверок. А для ситуаций типа onEvent() — вообще массиві или другие map структуры предпочитаю использовать типа $result = self::eventResultMap[$map] ?? 'defaultResult'


P.P.S. А тернарки как, не доставляют?

"более удобный для отладки отладчиком"

Согласен.


А тернарки как, не доставляют?

Доставляет всё, где нужно хоть чуть-чуть думать своим мозгом при трассировке. Я предпочитаю видеть результат выполнения программы глазами, а не мозгом :)

UFO just landed and posted this here
Согласен. Сработало искажение на базе моего опыта — я не припомню Builder'ов, которые не позволяли бы вызывать методы в цепочке, особенно те, что должны быть «одноразовые», потому я почти перестал различать эти два паттерна.
UFO just landed and posted this here

Согласен со всем, кроме:


2) читаемость кода важнее удобности дебага. Единый ретурн ухудщает читаемость.
У тебя есть метод. в определенный моент он ветвится. Психологически легче быстро отработать короткий случай и забыть о нем.

При чтении кода возврат в третей-пятой строке кода не отменяет необходимости читать метод до конца, чтобы его понять. Это при отладке можно выскочить на третей-пятой строке по return'у и не парится, что там далее (если условия так сработали), а для понимания всего метода нужно читать метод целиком. В таком случае


....
$result = 1234;
...
return $result;

ненамного хуже для читабельности, чем


...
return 1234;
...

если, разумеется, использовать для возврата переменную $result по-умолчанию. К тому же единая точка возврата дисциплинирует, заставляя надёжнее изолировать различные результаты через if-else, что весьма пригождается при рефакторинге. Но это мне мой опыт так говорит, у вас он может быть другим.

UFO just landed and posted this here
Делая ранний ретурн — ты гарантируешь что логика метода только про это.

Делать единственный возврат — вот это гарантия, что логика метода только про это. Ваш пример, переписанный для одной точки возврата:


if ($cond){
    //...
    $res = 'value';
} else {
    //...
}
return $res;

Такой подход заставляет читать код метода до конца, а не до первого return'а, и понимать его во всей полноте, а не только "основную логику" (или "наиболее часто встречающуюся"? или "самую тривиальную"? — что там обычно первым возвратом ставят?).


я не буду писать хорошо

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

Делать единственный возврат — вот это гарантия, что логика метода только про это.

Как раз не гарантия. Хорошо если визуально сразу видно две ветки, а что если их 10 и они не выделены как полноценные альтернативы? Типа


(if ($cond){
    $res = 'value';
}

if (!$cond) {
    // a lot of code
    $res='anotherValue';
}
return $res;

В таком случае ставьте return'ы — я буду дебажить с первой строки метода. Со временем ваш код может превратиться в подобие этого:


сэволюционировавший код
public function isApplicable()
{
    $p = $this->getProduct();
    if (!$p) {
        return false;
    }
    // has image for the current mode
    if (!$this->getValue('img'))
        return false;
    $now = Mage::getModel('core/date')->date();
    if ($this->getDateRangeEnabled() && ($now < $this->getFromDate() || $now > $this->getToDate())) {
        return false;
    }
    // individula products logic
    $inArray = in_array($p->getSku(), explode(',', $this->getIncludeSku()));
    if (!$p->getSku())
        $inArray = false;
    // include skus
    if (0 == $this->getIncludeType() && $inArray)
        return true;
    // exclude skus
    if (1 == $this->getIncludeType() && $inArray)
        return false;
    // use for skus only
    if (2 == $this->getIncludeType())
        return $inArray;

    if ($this->getPriceRangeEnabled()) {
        switch ($this->getByPrice()) {
            case '0': // Base Price
                $price = $p->getPrice();
                break;
            case '1': // Special Price
                $price = $p->getSpecialPrice(); // or $this->_info['special_price']
                break;
            case '2': // Final Price
                $price = Mage::helper('tax')->getPrice($p, $p->getFinalPrice());
                break;
            case '3': // Final Price Incl Tax
                $price = Mage::helper('tax')->getPrice($p, $p->getFinalPrice(), true);
                break;
            case '4': // Starting from Price
                $price = $this->_getMinimalPrice($p);
                break;
            case '5': // Starting to Price
                $price = $this->_getMaximalPrice($p);
                break;
        }
        if ($p->getTypeId() == 'bundle'){
            $priceMin = $p->getMinPrice();
            $priceMax = $p->getMaxPrice();
            if ($priceMin < $this->getFromPrice() && $priceMax > $this->getToPrice()) {
                return false;
            }
        }
        else if ($price < $this->getFromPrice() || $price > $this->getToPrice()) {
            return false;
        }
    }
    $attrCode = $this->getAttrCode();
    if ($attrCode) {
        if (!array_key_exists($attrCode, $p->getData())) { // has attribute condition
            return false;
        }
        // compatibility with the `Amasty: Custom Stock Status` extension
        // if the `Use Quantity Ranges Based Stock Status` property setted to `Yes`
        // so the value of the `Custom Stock Status` is dynamic
        // and setted to the product value is not used
        if (('custom_stock_status' === $attrCode) && (Mage::helper('core')->isModuleEnabled('Amasty_Stockstatus'))) {
            if ($this->getAttrValue() != Mage::helper('amstockstatus')->getCustomStockStatusId($p)) {
                return false;
            }
        } elseif ($this->getAttrValue()) {
            $v = $p->getData($attrCode);
            if (preg_match('/^[0-9,]+$/', $v)){
                if (!in_array($this->getAttrValue(), explode(',', $v))){
                    return false;
                }
            }
            elseif ($v != $this->getAttrValue()){
                return false;
            }
        } elseif (!$p->getData($attrCode)) { // sometimes needed for has attribute condition too
            return false;
        }
    }
    $catIds = $this->getCategory();
    if ($catIds) {
        $ids = $p->getCategoryIds();
        if (!is_array($ids))
            return false;
        $found = false;
        foreach (explode(',', $catIds) as $catId) {
            if (in_array($catId, $ids))
                $found = true;
        }
        if (!$found)
            return false;
    }
    $stockStatus = $this->getStockStatus();
    if ($stockStatus){
        $inStock = $p->getStockItem()->getIsInStock() ? 2 : 1;
        if ($inStock != $stockStatus)
            return false;
    }
    if ($this->getIsNew()){
        $isNew = $this->_isNew($p) ? 2 : 1;
        if ($this->getIsNew() != $isNew)
            return false;
    }
    if ($this->getIsSale()){
        $isSale = $this->_isSale() ? 2 : 1;
        if ($this->getIsSale() != $isSale)
            return false;
    }
    if ($this->getCustomerGroupEnabled() && ('' === $this->getCustomerGroups() || // need this condition, because in_array returns true for NOT LOGGED IN customers
        (!in_array(Mage::getSingleton('customer/session')->getCustomerGroupId(), explode(',', $this->getCustomerGroups())))))
        return false;
    // finally ...
    return true;
}

И это сделаете не вы, а те, кто будет модифицировать ваш код после вас.

Любой код может превратиться в такой. Сделаете c одной точкой возврата, а вам добавят промежуточных. Причём он станет ещё сложнее для понимания и изменения, чем изначально с ранними возвратами, потому что два подхода будет смешано.

Правильно, потому что чья-то "религия" позволяет ставить return'ы где ни попадя, а чья-то нет. В результате имеем микс. С одной точкой возврата получится такая вложенность по if'ам, что придётся разбивать проверку на связные части. Придётся делать код, более устойчивый к модификациям, т.к. придётся глубже нырять в предметную область и лучше понимать её. Поставить ещё один return гораздо проще.

А у кого-то есть компромиссы типа ранние возвраты только до основной логики. И никаких «придётся» нет, если никто или ничто не следит за вложенностью и т. п.

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

Не используй код код который...

Отличный совет! Жаль, что интеграторы не могут им воспользоваться.

Ой, про единую точку возврата уже накидали, но хочется ещё подискутировать не эту тему.

Читать вот это
function foo(baz) {
    if (baz < 0) {
        return 0;
    }

    //  ...
    //  do any for result
    return result;
}


Много проще чем это,
function foo(baz) {
    if (baz < 0) {
        result = 0;
    } else {
        //  ...
        //  do any for result
    }

    
    return result;
}


особенно если проверок может быть много
function foo(baz) {
    if (baz < 0) {
        result = 0;
        
    } else if(baz > 100 && baz <= 1000) {
        result = do_f1();
        
    } else if (baz > 1000 && baz <= 10000) {
        result = do_f2();
    
    } else if (baz > 10000 && baz <= 100000) {
        result = do_f3();
    
    } else{
        result = do_f4();
    }
    
    return result;
}


Эта гирлянда вообще вызывает ассоциации с ктулху.

А уж если как-то так получается, что иерархия if/else'ов уходит вглубь на пару уровней, то совсем тушите свет, потому что третьему уровню контекст теряется: кто и зачем туда попал?

Если вы пишите код для того, чтобы его читать, то да — так удобнее. Особенно, если для вас замена $result на return превращает ктулху-гирлянду в true-code-гирлянду:


function foo($baz)
{
    if ($baz < 0) {
        return 0;

    } else if ($baz > 100 && $baz <= 1000) {
        return do_f1();

    } else if ($baz > 1000 && $baz <= 10000) {
        return  do_f2();

    } else if ($baz > 10000 && $baz <= 100000) {
        return  do_f3();

    } else {
        return  do_f4();

    }
}

Лично у меня return из начала-середины метода вызывает ассоциации только с goto.

UFO just landed and posted this here

А я пишу код для того, чтобы его изменять. Изменять код буду я сам и мои коллеги. Изменяемость написанного кода — вот это "наше всё!" Я читал в детстве некоторые образчики perl-кода — ничего не понятно, но красиво-о-о. Занимательное чтиво.

Думаю, всё-таки, перед изменениями вы код читаете :) И, вполне вероятно, читаете чаще чем изменяете.

Вы правы, читаю я чаще, чем изменяю. Но читаю я именно для того, чтобы понять, где и что мне нужно изменить (или что здесь ничего менять не нужно). Код разбивается на классы/процедуры/функции не потому, что человек не может прочесть 2К строк подряд ("Войну и Мир" многие осилили). А для того, чтобы ограничить влияние изменений, особенно неправильных. Так что говорить, что мы пишем код для того, чтобы его читать — в корне не верно. Мы читаем, чтобы понимать. А понимаем, чтобы изменять. В противном случае было бы достаточно один раз написать код и один раз тесты к нему. Поэтому код должен быть не легко читаем, а легко изменяем. Кстати, удобнее читать код, который написан в одном файле, чем скакать от файла к файлу и обратно.

Под чтением я, естественно, имею в виду чтение с пониманием, а не то как я латынь читаю :)

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

А для чего вам понимание кода?

Самое частое, наверное — найти правильное место для изменений. Трудоемкость самих изменений — дело десятое.

Кстати, в тему свежая публикация. Там тоже задумались, а что такое читабельность кода и для чего она:


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

Выглядит как подмена понятий. Код может легко читаться, но сложно правиться. Ну, например,


if($cond) return true;

читается легче, чем


if ($cond) {
  return true;
}

но правится сложнее.

Согласен, несколько неточно. Нужно говорить о модифицируемости кода, а не о читабельности.

UFO just landed and posted this here
Код разбивается на классы/процедуры/функции не потому, что человек не может прочесть 2К строк подряд
Как раз таки именно потому! Прочитать код на 2к строк человек может

не находите противоречия?


где сайты представляли собой свалку файлов .php в корневой директории.

это ваше неверное представление о "ламповых" 2010-х, как неверно и ваше представление о том, что в свалке "легко менять" (возьмите на себя поддержку такой свалки хотя бы на пару лет).


У меня к вам такой неловкий вопрос…

У меня в профиле написаны языки, с которыми я имел дело. Magento использует Zend и Symfony для CLI. За Phalcon на могу ничего сказать — вообще не трогал. Laravel смотрел, но не более. Есть ещё неловкие вопросы?

UFO just landed and posted this here
> Лично у меня return из начала-середины метода вызывает ассоциации только с goto.

Именно! И альтернатива ранним возвратам как раз goto для меня. Но вот ваш пример для иллюстрации ранних возвратов не очень подходит — у вас все ветви равнозначные, кроме разве что первой. Ранний возврат или goto на возврат хорошо подходит когда есть несколько основных равнозначных ветвей и есть ветви для граничных случаев (в вашем пример, наверное, только отрицательные значения на неё подходят немного).
Это потому что вы неправильно всё исправили =))

function foo($baz)
{
    if ($baz < 0) {
        return 0;
    } 
    
    if ($baz > 100 && $baz <= 1000) {
        return do_f1();
    } 
    
    if ($baz > 1000 && $baz <= 10000) {
        return  do_f2();
    } 
    
    if ($baz > 10000 && $baz <= 100000) {
        return  do_f3();
    } 
    
    return  do_f4();
}


Вот так мне нравится больше

Я всего лишь переработал ваш код с $result'ами. Вы могли и его сделать чуть более красивым ;)

Весь код в этой ветке плох. Все эти ifы должны лежать на рантайме языка. Объектах, функциях или еще на чем. Тогда и дебажить нечего будет.

Чуть хуже то, что ты не можешь поставить безусловную точку останова (breakpoint) на ветке true или false. Придётся вместо одного клика в IDE поработать мышкой/клавиатурой чуть подольше, добавляя условную точку останова.

Это что за IDE? в нормальных становишься курсором куда надо, жмёшь F9 и
вуаля
Что мешает установить breakpoint на любой метод в каскадной цепочке?

Не знаю, у меня не получается. Научите. Я под IDEA работаю (PhpStorm).

А вот за это — спасибо! Попробую.

Sign up to leave a comment.

Articles