Какой тут может быть *овнокод?

Question

[Vitaly Vitaly]

Здравствуйте, коллега увидев метод в контроллере сказал, что он один сплошной быдлокод, именно сам код, объясните мне, может я не из нашего мира? Как по мне так все нормально.

public function getIndex()
    {
        $title = 'Мои компании';

        $where = array('uid' => Sentry::getUser()->id);
        if (Input::has('status')) $where['status'] = (Input::get('status') ? 1 : 2);

        $company_list = new CompanyList();
        $company = $company_list->get($where)->paginate(1);

        return View::make('dashboard.account', compact('title'))->with(array(
            'company' => $company
        ));
    }

Answer 1

[Сергей Протько]

1) Форматирование кода ужасно. Начнем с этого.
2) что тайтл делает в контроллере?
3) что за 1 и 2, что они значат? Стоит заменить на константы хотя бы.
4) уже пару лет можно использовать сокращенный синтаксис массивов
5) У вас же есть IoC, почему вы вызываете new CompanyList()? К чему это вообще относится? Что происходит?

Короче... ужас... контроллер это то место, по которому должно быть понятно что там происходит. Что-то в этом духе:

class CompanyListController {

    /**
     * @var CompanyRepositoryInterface
     */
    private $companyRepository;

    function __construct(CompanyRepositoryInterface $companyRepository)
    {
         $this->companyRepository = $companyRepository;
    }

    public function getIndex() 
    {
            $companies = $this
                 ->companyRepository
                 ->getCompanyByUser(Sentry::getUser()->id, Input::get('status', false))
            ;

            return View::make('dashboard.account')->with(compact('companies'));
    }

Comments

[Vitaly Vitaly]

Input::get('status', false) - не подойдет, по причине того, что возможно 3 состояния.
Все записи - empty, неактивные - 0, активные - 1,
я забыл поменять цифру с 2 на 1, в таблице поле status (tinyint(1)) 1 или 0 принимает значение. Поэтому если !has('status) - то мы выводим все компании.

[Vitaly Vitaly]

Vitaly Vitaly: с 2 на 0*

[Сергей Протько]

Vitaly Vitaly: false вполне себе хороший тригер что бы отключить фильтрацию по статусу. И да, 0, 1, 2 это тупняк. Company::PENDING или что-то в этом духе намного более выразительнее

[Сергей Протько]

Vitaly Vitaly: заметте, вся логика по конструированию запроса инкапсулируется в репозитории.

[Vitaly Vitaly]

Сергей Протько: ну это же уже многослойный торт получается

[Vitaly Vitaly]

Сергей Протько: не понимаю смысла, поясните, может приведете остальную часть? или линк на прочтение, просто думал что именно в простом отображении нет смысла городить слои, но интересно

[Vitaly Vitaly]

Сергей Протько: про false, верно, изменил.

[Сергей Протько]

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

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

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

[Vitaly Vitaly]

Сергей Протько: согласен. нет какой-нибудь хорошей информации почитать про реализации на laravel ваших примеров?

[Сергей Протько]

Vitaly Vitaly: ну типа документация.

Вот еще можно вот это посмотреть и сопутствующие термины гуглить:
https://www.youtube.com/watch?v=ajhqScWECMo

[Vitaly Vitaly]

Сергей Протько: спасибо, посмотрю, но именно в данной задачи - тут CRUD.

[Сергей Протько]

Vitaly Vitaly: потому эту ссылку и скинул - там как раз таки на примере стандартного CRUD рассматривается все.

[Vitaly Vitaly]

Сергей Протько: ;) благодарю!

[kamadi]

Сергей Протько: Спасибо за ссылку на видео.Классное видео. Многое для себя узнал. Если есть , можете скинуть ссылки на видео похожее на этот?

[Евгений]

а где еще тайтлу быть, как не в контроллере?

[Сергей Протько]

Nc_Soft: все что относится ко вью должно быть во вью. Во всяком случае в таком виде.

[Евгений]

Сергей Протько: ну данные тоже к представлению относятся, но их же никто не тянет во вьюхе, а получают в контроллере и передают

[Сергей Протько]

Nc_Soft: тайтл - не данные в этом контексте. Что бы отредактировать часть вьюхи вам надо лесть в контроллер - как по мне это не ок.

[Евгений]

Сергей Протько: Нет, не согласен.
Тайтл вообще в лейауте выводится и привязывается к маршруту, каждый урл = свой тайтл.
Более того, часто тайтл для каждого раздела берется из бд, чтобы его можно было менять для сео фишечек.

[Сергей Протько]

Nc_Soft: да, и все это выполняется исключительно во вью, не затрагивая контроллер никоим образом. Контроллер должен только сказать "отдай ему в респонсе отрендренную вьюшку" и экспоузит для нее данные. А уже внутри View происходит вся эта канитель с установкой заголовков, закардкожены они или же из базы берутся - дело третье.

Answer 2

[JhaoDa]

Как минимум:
— смесь CamelCase и snake_case,
— смесь compact() и View::make->with().

Comments

[Vitaly Vitaly]

в данном случае можно использовать compact, т.к. title и company - не являются аттрибутами класса?
CamelCase и snake_case - согласен, спасибо.

[JhaoDa]

Vitaly Vitaly: я не понял про атрибуты класса. $company это такая же переменная, как и $title, а значит можно использовать compact().

[Максим Кудрявцев]

А чем в коде выше плох CamelCase и snake_case? Тем что они вместе используются?

[Евгений]

ларавел сам использует микс: поля в тех же таблицах для связей зовет user_id , но названия методов итп в camelCase

Answer 3

[Сергей Гладковский]

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

Answer 4

[65536]

помимо сказанного, используйте автоформат. возьмите конфиг у коллеги, или накрутите свой и всегда им пользуйтесь