Как можно сократить говно-код?

Question

[cannabioid1337]

Уже как можно ужал этот ужас, но все равно ощущение что этот код очень грязный.
Как можно его сделать красивее?

public function index(Request $request)
    {
        // проверки

        if ($request->input("type") == "bet" ||
            $request->input("type") == "withdraw" ||
            $request->input("type") !== "create") 
        {
            if (!Auth::check()) {
                return json_encode(["errors" => "Вы должны быть авторизованы"]);
            }
        }

        if ($request->input("type") == "bet" ||
            $request->input("type") == "create") 
            {
                if (User::getUser()->balance < intval($request->input("bet"))) 
                {
                    return json_encode(["errors" => "Недостаточно средств"]);
                }
            }
        //

        $game = $request->input("game");

        switch ($game) {
            case "dice":
                $validator = Validator::make($request->all(), [
                    "bet" => "required|numeric|min:1|max:10000",
                    "coef" => "required|numeric|min:1.05|max:100",
                ]);

                if ($validator->fails()) 
                {
                    return json_encode(["errors" => $validator->errors()->first()]);
                }

                $this->bet = intval($request->input("bet"));
                $this->coef = floatval($request->input("coef"));

                return $this->dice();
                break;

            case "coin":
                if ($request->input("type") !== "bet" && $request->input("type") !== "withdraw") 
                    {
                        $validator = Validator::make($request->all(), [
                            "bet" => "required|numeric|min:1|max:15000",
                        ]);

                        if ($validator->fails() && $request->input("type") !== "is_active") 
                        {
                            return json_encode(["errors" => $validator->errors()->first()]);
                        }
                    } 

                $this->bet = intval($request->input("bet"));
                $this->side = $request->input("side");
                $this->type = $request->input("type");

                return $this->coin();
                break;

            case "mines":
                if (
                    $request->input("type") !== "bet" &&
                    $request->input("type") !== "get_coefs"
                ) {
                    $validator = Validator::make($request->all(), [
                        "bet" => "required|numeric|min:1|max:5000",
                        "mines" => "required|numeric|min:3|max:24",
                    ]);

                    if  ($validator->fails() &&
                        $request->input("type") !== "is_active" &&
                        $request->input("type") !== "withdraw") 
                        {
                            return json_encode(["errors" => $validator->errors()->first(),]);
                        }
                }

                $this->bet = intval($request->input("bet"));
                $this->mines = $request->input("mines");
                $this->mine = $request->input("mine");
                $this->type = $request->input("type");
                return $this->mines();
                break;

            case "wheel":
                $validator = Validator::make($request->all(), [
                    "bet" => "required|numeric|min:1"
                ]);

                if ($validator->fails() && $request->input("type") == "create")
                {
                    return json_encode(["errors" => $validator->errors()->first(),]);
                }

                $this->type = $request->input("type");
                $this->color = $request->input("color");
                $this->bet = $request->input("bet");
                $this->game_id = $request->input("game_id");
                return $this->wheel();
                break;

            case "bonus_wheel":
                $this->type = $request->input("type");
                return $this->wheel_bonus();
                break;

            default:
                return "unknown game";
        }
    }

Comments

[JhaoDa]

Как можно его сделать красивее?

Отрефакторить. Нам сделать это за тебя?

[twobomb]

Лень читать, но случайно взглянул на первое условие. Что за бред?

if ($request->input("type") == "bet" ||
            $request->input("type") == "withdraw" ||
            $request->input("type") !== "create")

Его можно сократить до
if($request->input("type") !== "create")
Потому-что если type будет bet или withdraw или чёрт лысый, оно всегда будет верным потому-что оно не равно create!!! Возможно там нужно было &&....

[Дмитрий]

ну все похожие участки кода, типо $this->type = $request->input("type"); можно за свич вынести

[profesor08]

twobomb,

if($request->input("type") !== "create")

Лучше и, самое главное, понятнее будет так if($request->input("type") === "create"), чтоб по месту проверки на какие-то значение, выполнить код, связанный с этой проверкой код и данными, которые в ней используются. Иначе получается, чтоб ты проверяешь на create, но выполняешь другой, код, а код связанный с create, выполняешь хрен пойми где.

Answer 1

[rPman]

Кучи if ($request->input("type") заменяй на switch case, тем более далее по тексту у тебя оно используется, пиши код хотя бы в одном стиле.

Я бы не рекомендовал контроль прав доступа, наличия баланса отделять от кода обработки самих действий, это аукается потом когда расширяешь функционал и забываешь добавить где то выше по коду проверку. Т.е. везде на каждое действие с подтипом делай проверку - а есть ли права, а хватает ли денег, а не повторно ли ты это делаешь и т.п. Если боишься за неэффективность такого кода, проверки оформляют в виде методов, внутри кеширование (если к примеру к базе данных запрос идет). Помним что преждевременная оптимизация - зло.

Про права доступа, у тебя точно 2 состояния - public и authorized? может у тебя еще есть какой-нибудь superuser? или еще есть права на основе данных (например если статус операции 'такой то' то действие запрещено)... хотя метод по любому должен быть ->allowed() который собирает данные по текущему пользователю, состоянию, данным и т.п. хотя лучше разделять права доступа по роли и по состоянию, пусть будет пара методов.

Вот такие строчки "required|numeric|min:1|max:10000" - bad practics, магические константы, что значит 10к, 15, 3, 24, 5000... везде разные, что почему зачем, уже через пол года ты не вспомнишь а уж другой программист и подавно не поймет. Оформляй в виде функции-генератора или даже класса, документируй все значения, даже если это значения от балды, когда ты их выдумывал какой то смысл ты в это вкладывал - опиши это в коментариях в этой функции или классе, не обязательно все выносить в конфиг файл, сам отдельно лежащий файл с таким классом уже этот конфиг. Мало того, почему сериализация массива такая странная, на рынке существует уже безоговорочный стандарт json, и machine и human readable, накладные расходы минимальны (можно заставить паковать nginx, по трафику будет не на много больше чем умно оформленный protobuf), тем более у тебя там ошибки уже в json выдаются.

Comments

[jazzus]

Одну лапшу заменить на другую. К чему Ларавел с конфигами и проверками на суперюзера если есть свои классные методы. главное, чтобы говнокод был в одном стиле и с комментами.

[rPman]

ОПС, я на подумал что используется фреймворк

Answer 2

[jazzus]

Валидацию убрать в форм реквест, правила доступа в политики. Остальное зависит от логики. См инструменты Ларавел кроме контроллеров и роутов.