Годный ли обработчик формы?

Question

[leni_m]

Привет Тостер!
Вообщем столкнулся с динамическими(могут присутствовать на странице, а могут и отсутствовать) полями формы, и написал обработчик, куда вместо $options попадает $_POST

public static function saveStaff($options)
    {
        $db = Db::getConnection();
        // Проверка есть ли хотя бы одно поле
        if (isset($options['director_surname']) || isset($options['director_name'])
            || isset($options['director_ot4']) || isset($options['director_oklad'])
            || isset($options['director_date_start']))
        {
            $sql = 'UPDATE staff SET ';
            if (isset($options['director_surname'])) {
                $sql .= 'surname = :surname,';
            }
            if (isset($options['director_name'])) {
                $sql .= 'name = :name,';
            }
            if (isset($options['director_ot4'])) {
                $sql .= 'ot4 = :ot4,';
            }
            if (isset($options['director_oklad'])) {
                $sql .= 'oklad = :oklad,';
            }
            if (isset($options['director_date_start'])) {
                $sql .= 'date_start = :date_start,';
            }
            // Удаляем последнюю запятую в запросе
            $sql = substr($sql,0,-1);

            $sql .= ' WHERE id = 1;';

            $result = $db->prepare($sql);
            if (isset($options['director_surname'])) {
                $result->bindParam(':surname', $options['director_surname'], PDO::PARAM_STR);
            }
            if (isset($options['director_name'])) {
                $result->bindParam(':name', $options['director_name'], PDO::PARAM_STR);
            }
            if (isset($options['director_ot4'])) {
                $result->bindParam(':ot4', $options['director_ot4'], PDO::PARAM_STR);
            }
            if (isset($options['director_oklad'])) {
                $result->bindParam(':oklad', $options['director_oklad'], PDO::PARAM_STR);
            }
            if (isset($options['director_date_start'])) {
                $result->bindParam(':date_start', $options['director_date_start'], PDO::PARAM_STR);
            }
            $result->execute();
        }
    }

Но чего-то я чую, что все эти проверки лишние и только захламляют код.
Может как-то можно оптимизировать?

Comments

[Дмитрий Гаджиев]

старая мудрость: если код работает, не лезь в него!!!

Answer 1

[Eugene Mosyukov]

Не делайте такие длинные методы. Не копипасте. Отделяйте построение запроса от данных. И не надо все делать static.

public static function saveStaff($options) 
{
    if (self::isValidStaff($options)) {
        $staffFields = self::getFilledStaffFileds($options);
        $qb = new QueryBuilder( Db::getConnection() );
        $qb->where('id = 1')->update('staff', $staffFields);
    }
}

public static function getFilledStaffFileds($options)
{
    $staff = [];
    if ($options['director_surname']) {
        $staff['surname'] = $options['director_surname'];
    }
    if ($options['director_surname']) {
        $staff['name'] = $options['director_name'];
    }
    if ($options['director_surname']) {
        $staff['ot4'] = $options['director_ot4'];
    }
    if ($options['director_surname']) {
        $staff['oklad'] = $options['director_oklad'];
    }
    if ($options['director_surname']) {
        $staff['date_start'] = $options['director_date_start'];
    }

    return $staff;
}

function static function isValidStaff($options)
{
  return isset($options['surname']) 
    || isset($options['name'])
    || isset($options['ot4']) 
    || isset($options['oklad'])
    || isset($options['date_start']);
}

И использовать какой-нибудь построитель запросов, типа такого:

class QueryBuilder {
    protected $db;
    protected $where = '';
    
    public function __construct($db) {
        $this->db = $db;
    }
    
    public function where($where)
    {
        $this->where = trim($where);
        return $this;
    }
    
    public function update($table, $params)
    {
        $values = [];
        foreach ($params as $key => $value) {
            $values[] = "`{$key}` = :{$key}";
        }
        
        $sql = 'UPDATE `' . $table . '` SET ' . join(', ', $values);
        if ($this->where) {
            $sql .= ' WHERE ' . $this->where;
        }

        $result = $this->db->prepare($sql);
        foreach ($params as $key => $value) {
            $result->bindParam(':' . $key, $value, PDO::PARAM_STR);
        }
        return $result->execute();
    }
}

P. S. Лучше пользоваться готовыми компонентами.
P. P. S. Наверняка есть опечатки, набирал без IDE.

Answer 2

[Олег Мифле]

Я бы заменил эту кучу if`ов на switch, типа:

switch (true) {
    case !empty($options['director_ot4']):
    //some action
        break;

    case !empty($options['director_date_start']):
     //some action
          break;
}

И вместо условия проверки в начале вставил бы что-то типа

if (empty($options)) {
    return;
}

Comments

[Станислав Б]

там же не if-elseif конструкция. в $options может быть не 1 ключ.

[Олег Мифле]

Станислав Б: Ну да. Под конец дня уже голова кипит. Тогда можно foreach`ом пробежаться.

[Eugene Mosyukov]

switch лучше использовать как можно реже.

[Олег Мифле]

Eugene Mosyukov: очень спорный вопрос.

[Eugene Mosyukov]

Олег Мифле: ну вот как-то странно со свитчем происходит, его крайне редко используют по делу.

А еще любят допускать досадные ошибки, опуская в одном из кейсов соответствующий break. И читая код порой сложно однозначно сказать, break опущен намерено или ошибочно.

[Олег Мифле]

Eugene Mosyukov: по этому важно соблюдать code style и избегать неоднозначных конструкций.

[Eugene Mosyukov]

Олег Мифле: самое важное для switch-а — не перегружать его сложной логикой, иначе все преимущества теряются. Его удобно использовать, когда нужно одно простое значение сравнить с другим простым (лучше с константой). Вот тогда получается наглядно и лаконично. И, в общем, так как он и задумывался.