3

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

Есть таблица Orders, состоящая из id, user_id, created_at, type, value, status, в данном примере нас интересуют только поля value и type. value - любое целочисленное положительное число, type - либо 0, либо 1 (0 - списание, 1 - начисление)

И собсно простой метод getPoints в классе User, который подсчитывает количество доступных очков пользователя

public function getPoints() {
        $return = 0;
        foreach (Order::findAll(['user_id'=> $this->id, 'status' => 1]) as $k => $value) {
            if ($value['type'] == 0)
                $return -= $value['value'];
            if ($value['type'] == 1)
                $return += $value['value'];
        }
        return $return;
}

От нечего делать я решил его немного изменить

public function getPoints() {
        $return = 0;
        foreach (Order::findAll(['user_id'=> $this->id, 'status' => 1]) as $value)
                $return += (-1 + 2*(int)$value['type'])*$value['value']; //$value['type'] равно либо 0, либо 1. 0 - вычитание, 1 - сумма. Формула -1 + 2*$value['type'] нужна для сокращения кода -1 + 2*0 = -1 -1 + 2*1 = 1

        return $return;
    }

То есть по сути просто избавился от условий и заменил это дело формулой (предполагается, что значения 0 и 1 никогда не будут меняться). Так вообще нормально делать? Стоит ли?

4
  • массу способов вроде придумать можно, и надо было выбрать настолько сложный? $return += ($type == 1 ? $value : -$value)
    – teran
    25 янв 2018 в 10:40
  • либо там $k = [-1, 1] и += $k[$type] * $value
    – teran
    25 янв 2018 в 10:42
  • 1
    но если честно, такие штуки надо просто считать на стороне БД, примерно как тут ru.stackoverflow.com/a/774879/223826
    – teran
    25 янв 2018 в 10:43
  • @teran во, вот за это отдельное спасибо, про отдельный запрос как-то не подумал
    – Peresada
    25 янв 2018 в 10:46

2 ответа 2

5

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

P.S. По этому поводу на харбе недавно хорошая статья вышла: https://habrahabr.ru/post/347166/

2

Если уже "рефакторить", то где то так

public function getPoints() {
        $return = 0;
        foreach (Order::findAll(['user_id'=> $this->id, 'status' => 1]) as $k => $value) {
            $type = $value['type'];
            $val = $value['value'];
            if ($type == 0) {
                $return -= $val;
            else if ($type == 1) {
                $return += $val;
            } else {
              # а тут добавить вывод в лог, может что то пошло не так
            }
        }
        return $return;
}
4
  • а если бы нужно было обрабатывать 10 полей $value, а не 2?
    – Peresada
    25 янв 2018 в 10:44
  • меня просто эти строчки смутили,$type = $value['type']; $val = $value['value']; неужели они добавляют "читаемость"?
    – Peresada
    25 янв 2018 в 10:45
  • @Peresada именно они и добавляют, замечу что у меня в комментах к вопросу, так же написано :)
    – teran
    25 янв 2018 в 10:46
  • Да, они сильно добавляют, так как возле них можно поставить комментарий. А бывает, что в базе поле переименовали... Если же полей 10, то тут нужно подумать, может они не нужны или нужно сделать себе класс-обертку.
    – KoVadim
    25 янв 2018 в 10:53

Ваш ответ

By clicking “Отправить ответ”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Всё ещё ищете ответ? Посмотрите другие вопросы с метками или задайте свой вопрос.