Задание на собеседование - прокомментировать код

code-ex.php

<?php
/**
 * Я переписал предложенный код в соответствии с моим комментарием ниже и моим видением(учитывая что остального кода, кроме этого фрагмента я не видел)
 */
 
class AnyModel extends SomeBaseModel {
   /**
    * AnyModel constructor.
    *
    * @param int $userId
    * @param int $orderId
    * @param int $proposalId
    */
   public function __construct($userId = null, $orderId = null, $proposalId = null)
   {
       parent::__construct();
 
       // Если $userId задан - переопределяем соединение с БД
       if ($userId) {
           $this->userId = $userId;
 
           if ($orderId) {
               $className = get_class($this);
               switch ($className) {
                   case ProposalModel::class:
                       $this->orderId = $orderId;
                   case OrderModel::class:
                       $this->orderId = $orderId;
                       break;
               }
           }
 
           // Вызов getter'а getDefaultSharedId()
           $this->mdb = $this->db->connectTo($this->db->defaultSharedId);
       }
   }
}

Ответ

0.0 Мои комментарии предполагают что версия php 5.6 или может быть обновлена до нее(или до 7.0.4 еще лучше)

1.1 Общая картина: код устаревший.
1.2 судя по всему он наследуется от класса который определяет свойство db(самописный объект работы с к базой, но не подключение). То что родительский конструктор вызван в начале, предполагает что в родительском конструкторе создается подключение.
1.3 В свойстве mdb лежит экземпляр Connection'а
1.4 Судя по именам классов не используются namespace'ы и psr-4 autoloader.
1.5 Так же похоже что часть кода(или менталитет разработчиков) мигрировала с php4, так как свойства начинаются с подчеркивания. В php5 это уже не есть хороший тон ведь. В psr-2 четко сказано: "Одиночный знак подчёркивания в начале имени свойства НЕ СЛЕДУЕТ использовать как признак защищённой (protected) или приватной (private) области видимости."
1.6 Кажется именование через подчеркивание свойств и методов уже давно проиграло в пользу верблюжей нотации. Все(Yii2, Symfony, ZF, Laravel) уже используют верблюжью нотацию. Ладно, предположу что просто код очень старый, с тех времен когда еще не было namespace'ов
1.7 В коде нет какого-то базового класса Object который определяет геттеры и сеттеры. А ведь удобнее было бы писать $this->userId = $user_id что вызвало бы сеттер $this->setUserId($userId), вместо $this->set_user_id($user_id). Как же так без сеттеров и геттеров то? =)

2.0 Думаю !is_null($user_id) лучше записать как !$user_id если есть гарантия что $user_id не будет равен 0, или $user_id !== null, если значение 0 возможно. Оператор !== в любом случае сработает быстрее чем вызов функции(is_null) и отрицание результата(!). Вы скажете "это не существенно"... и будете правы. Но все же жизнь состоит из мелочей ;)

3.1 func_*_arg() функции конечно не попали под deprecated, и не попадут наверное, но все же я бы написал так:

public function __construct($user_id = null, ...$options) {
...
  if ($options) { // Можно не использовать !empty(), так как массив без элементов - ложное значение.
  ...
  switch ...
  ...
  case 'Proposal...
    $this->_order_id = $options[1];
    if (isset($options[2])) $this->_proposal_id = $options[2];
    ...

Это если все же использовать динамические аргументы. Но зачем вообще они здесь нужны? Почему бы не записать так? (вношу изменения только касающиеся аргументов)

// Не зная вашей архитектуры, предположу что все primary_key начинаются с 1. И 0 тут быть не может!!!
/**
 * @param int $user_id ...
 * @param int $order_id ...
 * @param int $proposal_id ...
 */
public function __construct($user_id = null, $order_id = null, $proposal_id = null) {
  parent::__construct();

  // Если есть $user_id, то переопределяем соединение с БД
  if ($order_id) { 
    $class_name = get_class($this);

    switch ($class_name) {
      case 'Order_model':
        $this->_order_id = $order_id;
        break;

      case 'Proposal_model':
        $this->_order_id = $order_id;
        if ($proposal_id) $this->_proposal_id = $proposal_id;
        break;
    }
    ...
  }
  ...
}

4.0 На счет switch.

4.1 Для чего там пустой case ? Для симметричности? Почему бы тогда все модели не записать туда, разве их только 3? Ну и если для симметричности то блок default уж точно для симметричности нужен :) PS: на самом деле предполагаю что этот case был не пустой, но после удаления кода - там его поленились удалить или подумали что пригодится

4.2 Повторяющие действия. (предположу что у вас в компании не запрещены проваливания в switch, и так как рука Крокфорда еще не коснулась php :))) )

switch ($class_name) {
  case 'Proposal_model':
    if ($proposal_id) $this->_proposal_id = $proposal_id;
  case 'Order_model':
    $this->_order_id = $order_id;
    break;
}

так ведь проще?

4.3 Возможно (если модель точно должна попасть в один из case'ов) лучше добавить default блок в котором вызов логирующего ошибку(status=warning) метода.

5.0 На самом деле первое что бросается в глаза это странный отступ блока else на 4 с низу строке. Решил отложить это на конец. Тут или удалена часть кода или что-то сделано неправильно мне кажется(но не уверен, так как всего кода не вижу).

5.1 В самом случае else нужно влево сдвинуть tab'ом.

5.2 судя из комментария "Если задан $user_id, то..." соединение установлено в родительском конструкторе. И того у нас 2 соединения. Это плохо конечно. Накладные ресурсы... (если первое нигде больше не нужно).
5.3 Особенно не состыкуется этот блок else с комментарием "Если есть $user_id, то...". Это похоже на команду создания нового подключения, которое должно быть в блоке if, а не в блоке else этого if'а.