@Chaly95

Этот код сильно ужасен)?

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

<?php
include "bitrix.php";

include "function.php";
require_once  '../src/db.php';

use Aura\Database;
$id = $_GET['id'];
$id_bot = $id;
$config = require_once '../config.php';
$db = new Database($config['db']);
$select = $db->query("SELECT * FROM tg_bot WHERE  tg_bot.id='$id'");
$bot_token = $select[0]['api_key'];
$bot_url = $select[0]['url'];

define('BOT_TOKEN', "$bot_token");
define('API_URL', 'https://api.telegram.org/bot'.BOT_TOKEN.'/');
define('BOT_URL', $bot_url);
define('WEBHOOK_URL', BOT_URL);

if (php_sapi_name() == 'cli') {
    // webhook
    apiRequest('setWebhook', array('url' => isset($argv[1]) && $argv[1] == 'delete' ? '' : WEBHOOK_URL));
    exit;
}


$content = file_get_contents("php://input");
$update = json_decode($content, true);

if (!$update) {
    exit;
}


if (isset($update["message"])) {

    $message = $update['message'];
    $message_id = $message['message_id'];
    $chat_id = $message['chat']['id'];
    $customer_id = $id_bot + $chat_id;
    $getChat = apiRequestJson('getChat', array('chat_id' => $chat_id));
    $chatuser = $getChat['first_name']. " " .$getChat['last_name'] ;
    $telegramuser = $getChat['username'];
    $phone = $message["contact"]["phone_number"];
    $printphone = print_r($message['contact']);
    $curtime = date(H);

    $select_customers = $db->query("SELECT * FROM customers WHERE id='$customer_id'");
    $id_campaign = $select_customers[0]['id_campaign'];

    $message_query_campaign = $db->query("SELECT * FROM campaign WHERE id='$id_campaign'");
    $message_campaign = $message_query_campaign[0]['message'];
    $calendar_title = $message_query_campaign[0]['calendar_title'];
    $time_title = $message_query_campaign[0]['time_title'];


    if(isset($message['contact'])){

        $phone_contact = $db->query("UPDATE customers SET phone = '$phone' WHERE customers.id = '$customer_id';");
        if ($select_customers[0]['last_steps']=='register'){
            addinfo("$customer_id",array("PHONE" => array( array( "VALUE" => "$phone", "VALUE_TYPE" => "WORK" ) )
            ,"UF_CRM_1568478724" => "3"
            ));
        add_client_1c ($getChat['first_name'], $getChat['last_name'], $phone, $id, '12345', $customer_id, $id_campaign);
        $register = $db->query("UPDATE customers SET status = 1 WHERE customers.id = '$customer_id';");
        $calendar =  get_calendar((int)date('m'), (int)date('Y'));
        apiRequestJson("sendMessage", array('chat_id' => $chat_id, "text" => $calendar_title, 'reply_markup' => array(
            "inline_keyboard" => $calendar
        )));
        }elseif ($select_customers[0]['last_steps']=='contact'){
            addinfo("$customer_id",array("PHONE" => array( array( "VALUE" => "$phone", "VALUE_TYPE" => "WORK" ) )
            ,"UF_CRM_1568478724" => "2"
            ));
            apiRequestJson("sendMessage", array('chat_id' =>  $chat_id, "text" => $time_title));
        }
    }





    elseif (isset($message['text'])) {




            $text = mb_strtolower($message['text']);
            $steps = $db->query("SELECT * FROM `steps` WHERE id_campaign='$id_campaign' AND pre_text='$text'"); // pre_text
       if ($steps == true){
        if ($steps[0]['type']=='contact'){
            $last_steps = $db->query("UPDATE customers SET last_steps = 'contact' WHERE customers.id = '$customer_id';");}
        elseif ($steps[0]['type']=='register'){
            $last_steps = $db->query("UPDATE customers SET last_steps = 'register' WHERE customers.id = '$customer_id';");}

            foreach ($steps as $key => $value) {


                if (iconv_strlen ($value['img_url']) >= 15){
                    $img_url = $value['img_url'];
                    apiRequestJson("sendPhoto", array('chat_id' => $chat_id, 'photo' => "$img_url"));
                }



                $id_steps = $value['id'];

                if ($select_customers[0]['status'] == 0) {// зарегистрирован
                    $button = $db->query("SELECT * FROM button WHERE id_steps='$id_steps' AND NOT status_customer='1' ORDER BY `button`.`position` ASC"); // шага
                }elseif ($select_customers[0]['status'] == 1){ // зарегистрирован
                    $button = $db->query("SELECT * FROM button WHERE id_steps='$id_steps' AND NOT status_customer='2' ORDER BY `button`.`position` ASC"); // шага
                }else{
                    $button = $db->query("SELECT * FROM button WHERE id_steps='$id_steps' ORDER BY `button`.`position` ASC"); // шага
                }

                if ($button == true) {
                    foreach ($button as $keys => $values) {

                        if ($values['type'] === 'text') {
                            $button_array[] = array($values['text']);
                        } elseif ($values['type'] === 'contact') {
                            $button_array[] = array(
                                array(
                                    'text' => $values['text'],
                                    'request_contact' => true
                                )
                            );
                        } elseif ($values['type'] == 'url') {

                            $button_array[] = array(
                                array(
                                    'text' => $values['text'],
                                    'request_contact' => true
                                )
                            );
                        }
                    }

                    if ($text == $value['pre_text']) {
                        apiRequestJson("sendMessage", array('chat_id' => $chat_id, "text" => $value['text'], 'reply_markup' => array(
                            'keyboard' => $button_array,
                            'one_time_keyboard' => true,
                            'resize_keyboard' => true)));
                    }
                } else {
                    if ($text == $value['pre_text']) {
                        apiRequestJson("sendMessage", array('chat_id' => $chat_id, "text" => $value['text']));
                    }
                }
            };}else{
           if (strpos($text, "/start") === 0) {}
          else{
           apiRequestJson("sendMessage", array('chat_id' => $chat_id, "text" => 'Я не маю відповіді на ваше питання, але я можу вас направити до нашого менеджера', 'reply_markup' => array(
               "inline_keyboard" => array(array(array(
                   "text" => "Перейти у чат з менеджером",
                   "url" => "tg://resolve?domain=AURA_FK_bot",
                   "callback_data" => "button_0"
               ))))));
           apiRequestJson("sendMessage", array('chat_id' => $chat_id, "text" => "Або ви можете повернутися назад $customer_id", 'reply_markup' => array(
               'keyboard' => array(array('Назад')),
               'one_time_keyboard' => true,
               'resize_keyboard' => true)));}
           $message_input = $db->query("INSERT INTO `chat_tg` (`id`, `id_bot`, `id_chat`, `name`, `message`) VALUES (NULL, '$id', '$customer_id', '$chatuser', '$text');");
       }



        if (strpos($text, "/start") === 0) {


            $regex_start = '/(\/start\s)(\d+)/';
            if(preg_match($regex_start, $text, $match)) {
                $id_start = $match[2];
            }

            if(isset($id_start)){

                $message_query_campaign = $db->query("SELECT * FROM campaign WHERE id='$id_start'");
                $message_campaign = $message_query_campaign[0]['message'];
                $title_campaign = $message_query_campaign[0]['title'];
                addlead("Заявка Telegram BOT кампания: ". $title_campaign,$customer_id, $title_campaign, $chatuser, $telegramuser,'');
            }elseif(isset($id_campaign)){

                $message_query_campaign = $db->query("SELECT message FROM campaign WHERE id='$id_campaign'");
                $message_campaign = $message_query_campaign[0]['message'];
                $id_start = $id_campaign;
            }else{
                $message_campaign = "hello, i am Test bot FC AURA";
            }


            $query_campaign = $db->query("SELECT * FROM campaign WHERE id='$id_start'");
            if ($query_campaign[0]['status'] == 1) { // сообщений
                apiRequestJson("sendMessage", array('chat_id' => $chat_id, "text" =>  $message_campaign, 'reply_markup' => array(
                    'keyboard' => array(
                        array('Детальніше'),
                    ),
                    'one_time_keyboard' => true,
                    'resize_keyboard' => true)));
            }else{
                apiRequestJson("sendMessage", array('chat_id' => $chat_id, "text" => 'Кампания выключена'));
            }

            if (empty($select_customers)){
                apiRequestJson("sendMessage", array('chat_id' => $chat_id, "text" => "Добавлен пользователь в базу данных $customer_id"));
            $insert = $db->query("INSERT INTO customers (id, telegram_id, id_bot, tg_user, id_campaign, name, phone) VALUES('$customer_id','$chat_id','$id','$telegramuser','$id_start','$chatuser','')");// добавляем

            }else{
                $update = $db->query("UPDATE customers SET id = '$customer_id', telegram_id = '$chat_id', id_bot = '$id', tg_user = '$telegramuser', id_campaign = '$id_start', name = '$chatuser', phone = '' WHERE customers.id = '$customer_id';");// обновляем
            }


        }

    }

    else {
        apiRequest("sendMessage", array('chat_id' => $chat_id, "text" => 'Я понимаю только текстовые сообщения (')); // сообщение
    }
}
  • Вопрос задан
  • 363 просмотра
Решения вопроса 3
zoonman
@zoonman
CEO @ LinuxQuestions.ru
Ну что же, начнём? Только не плакать потом, сами попросили.

Итак, начнем с архитектуры. Начнем с того, что код чем только не занимается, он и из консоли запускает и как веб-хук и т.д. Сам по себе он является куском битрикса в том или ином виде.
Далее - код использует ООП, но написан в процедурном стиле.

Уже за это надо проявлять насилие над личностью.

<?php
include "bitrix.php";
include "function.php";
require_once  '../src/db.php';


Просто используйте require_once.

use должен быть вверху файла.

$id = $_GET['id'];
Будет генерировать ошибки, если скрипт был вызван без id.

$select = $db->query("SELECT * FROM tg_bot WHERE  tg_bot.id='$id'");

Добро пожаловать в мир SQL-инъекций.

$bot_token = $select[0]['api_key'];
Что произойдет, если бот не существует? Ошибки.

define('BOT_TOKEN', "$bot_token");
Бить железной линейкой по пальцам. Сильно. Больно. Долго.

Нельзя использовать переменные внутри define. Эта инструкция придумана для объявления констант. Объявлять константы используя переменные недопустимо.

if (php_sapi_name() == 'cli') {
    apiRequest('setWebhook', array('url' => isset($argv[1]) && $argv[1] == 'delete' ? '' : WEBHOOK_URL));
    exit;
}

Жуть какая-то.

$content = file_get_contents("php://input");
$update = json_decode($content, true);

if (!$update) {
    exit;
}

Я так понимаю, что здесь происходит вызов самого веб-хука.

Ну а дальше просто идет месиво. Смешались в кучу кони, люди...

Что с этим можно сделать?

Вызубрить ООП и getjump.github.io/ru-php-the-right-way

Вспомните принцип "Разделяй и властвуй".
Разделяйте код по функционалу и уровням абстрации.
Например разделите код на работу с базой, на работу с API, на бизнес-логику.

Например ваш код мог бы выглядеть так:

// добавляем автозагрузку классов
require_once 'vendor/autoload.php';
// импортируем конфигурацию
require_once 'config.php';

use Chaly/Bot;

// проверяем, что наш скрипт вызван с индентификтором бота
if (!isset($_GET['id'])) {
  exit(1); // ненулевой статус говорит об ошибке
}

// загрузить робота из базы данных
$bot = Bot::loadById($_GET['id']);
if (!$bot) {
  exit(1);
}

// запустить робота
$bot->run();


Как может выглядеть тело run()

public function run() {
  if (Cli::isValid()) {
    switch (Cli::getCommand()) {
        case 'delete':
            $this->delete();
            break;
        case 'register':
            $this->register();
            break;
    }
    return;
  }

  $payload = $this->getWebHookPayload();
  if (!$payload) {
    return;
  }

  if (isset($payload['message'])) {
    $this->interpretMessage($payload['message']);
  }

  // и так далее, думаю смысл понятен
}
Ответ написан
Dimastik86
@Dimastik86
(isset($brain))?: die;
старайся писать так, чтобы было как можно проще и понятнее с этим кодом работать... используй ф-ции, исключения, старайся избегать вложенных друг в друга if, переназначения переменных, посмотри паттерны и тд...

а это если тебе же через 2 недели показать, ты скорей всего будешь сидеть и сам же пытаться понять что куда...
Ответ написан
@BATPYIIIKOB
PHP, JS
1. Функция должна занимать не более одной страницы в высоту.
2. Общие блоки - выдели в отдельные функции
3. Комментарии кто будет писать?
4. Зачем выделять переменные и не использовать их?
$curtime = date(H);
5. Разбейте код на отдельные функции - наименования по смыслу (не говорю про ООП, нет смысла). А то всё это похоже на if-овый бред. Непонятен алгоритм. Код должен читаться как книжка.
6. советую почитать https://refactoring.guru/ru/refactoring/smells
https://refactoring.guru/ru/decompose-conditional - по ифам

да и....
ЗАБЕЙ НА БИТРИКС!
Ответ написан
Пригласить эксперта
Ответы на вопрос 2
usdglander
@usdglander Куратор тега PHP
Yipee-ki-yay
После include "bitrix.php"; да. Ужасен. Не глядя.
Ответ написан
@sidni
Php Developer
Все зависит в рамках какой задачи Вы пишите.
Как бы если этот скрипт написан за минимальный прайс и минимальный тайминг, на какой нибудь халтурке (фриланс), то ничего страшного другими словами "а на что Вы надеялись".
Если Вы являетесь штатным программистом в конторе или этот проект долгосрочен для Вас и Вы потеряли кучу времени на реализацию этого кода, то Вам нужно пересмотреть свои взгляды на будущее, в современном пхп мире так уже никто не пишит.
1) Сейчас стараются писать в ООП стиле Ваш код имеет вид процедурного кода больше перетекающий в "лапшекод".
Как минимум необходимо находить независимые атомарные операции и обворачивать в классы
например голый sql запрос не может быть на одном уровне бизнес логикой и проверкой входных данных (см. паттерн Репозиторий), так же нельзя тут же обращаться к апи телеграмма может быть стоить выделить в отдельный универсальный класс отправки сообщений (вдруг вам надо будет отправлять вместо телеграмма, в вайбер) (можно глянуть паттерн Декоратор) и т. д.
2)
include "bitrix.php";

include "function.php";
require_once '../src/db.php';

Но при этом
use Aura\Database;

3) избегайте if else особенно когда они занимают пол экрана это очень ухудшает читаемость и дальнейшее расширение приложения
4) используйте Exception правильное использования сократит количество if else
можно дальше продолжать но начните с этого
Ответ написан
Ваш ответ на вопрос

Войдите, чтобы написать ответ

Войти через центр авторизации
Похожие вопросы