Задать вопрос
TheRikipm
@TheRikipm
Backend middle

Можете поревьюить?

Чувствую что код получился не самый читаемый, но не вижу что конкретно можно исправить.

<?php


namespace rikipm;

//Link with 'require' instead of autoloader because its too small "project".
//Of course in real work this way will not be used.
require 'VkConversationsStatisticInterface.php';


class VkConversationsStatistic implements VkConversationsStatisticInterface
{
    const ROOT_URL = 'https://api.vk.com/method/';
    const OVERDUE_TIME = 15 * 60; //Time, after which conversation becomes overdue

    private $access_token;
    private $date_from = 0;
    private $date_to = 999999999;

    /**
     * Send GET request to VK api (v 5.52)
     *
     * @throws \Exception if API return error.
     * @param string $method
     * @param array $data
     * @return \stdClass
     */
    private function sendRequest($method, $data = [])
    {
        $request = curl_init(self::ROOT_URL . $method . '?v=5.52&' . http_build_query($data) . '&access_token=' . $this->access_token);

        curl_setopt($request, CURLOPT_RETURNTRANSFER, true);

        $data = curl_exec($request);
        curl_close($request);

        $data = json_decode($data);
        if ($data->error) {
            throw new \Exception($data->error->error_msg);
        }
        return $data;
    }

    /**
     * This function handle cases when peer (user or group) send several messages in a row.
     * Only first message in "stack" will be counted.
     *
     * @param array $messages
     * @return array $messages
     */
    private static function filterMessageStacks($messages)
    {
        foreach ($messages as $key => $message) {
            if ($message->out == $messages[$key - 1]->out) {
                unset($messages[$key - 1]);
            }
        }
        $messages = array_values($messages);
        return $messages;
    }

    /**
     * This function will delete first message if it belongs to group.
     *
     * @param array $messages
     * @return array $messages
     */
    private static function filterFirstMessage($messages)
    {
        if ($messages[count($messages) - 1]->out == 1) {
            unset($messages[count($messages) - 1]);
            $messages = array_values($messages);
        }
        return $messages;
    }

    /**
     * This function will delete last message if it belongs to user.
     *
     * @param array $messages
     * @return array $messages
     */
    private static function filterLastMessage($messages)
    {
        if ($messages[0]->out == 0) {
            unset($messages[0]);
            $messages = array_values($messages);
        }
        return $messages;
    }

    /**
     * Return list of times between user message and group reply
     *
     * @return array|null
     */
    private function getReplyTimes()
    {
        $reply_times = [];

        $conversations = $this->sendRequest('messages.getConversations')->response->items;
        foreach ($conversations as $conversation) {
            $peer_id = $conversation->conversation->peer->id;
            //Messages ordered from newest to oldest!
            $messages = $this->sendRequest('messages.getHistory', ['peer_id' => $peer_id])->response->items;
            $messages = self::filterMessageStacks($messages);

            foreach ($messages as $key => $message) {
                if (!($message->date >= $this->date_from and $message->date <= $this->date_to)) {
                    unset($messages[$key]);
                }
            }
            $messages = array_values($messages);

            //Ignore first message if it belongs to group
            $messages = self::filterFirstMessage($messages);

            //Ignore last message if group dont replied
            $messages = self::filterLastMessage($messages);

            for ($i = 0; $i < count($messages); $i += 2) {
                $reply_times[] = $messages[$i]->date - $messages[$i + 1]->date;
            }
        }

        return $reply_times;
    }

    public function __construct($access_token)
    {
        $this->access_token = $access_token;
    }

    public function setDatePeriod($date_from, $date_to)
    {
        $this->date_from = $date_from;
        $this->date_to = $date_to;
    }

    /**
     * @return float|null
     */
    public function getAverageReplyTime()
    {
        $reply_times = $this->getReplyTimes();

        if (!$reply_times) {
            return null; //No specs what to return if messages not found
        }

        return array_sum($reply_times) / count($reply_times);
    }

    /**
     * @return int|null
     */
    public function getModeReplyTime()
    {
        $reply_times = $this->getReplyTimes();

        if (!$reply_times) {
            return null; //No specs what to return if messages not found
        }

        //Convert seconds to minutes
        foreach ($reply_times as $key => $reply_time) {
            $reply_times[$key] = (int)round($reply_time / 60); //Cant use intval because it rounds always floor
        }

        $frequencies = array_count_values($reply_times);
        return array_search(max($frequencies), $frequencies);
    }

    /**
     * @return array List of overdue conversation as stdClass objects
     */
    public function getOverdueConversations()
    {
        $overdue_conversations = [];

        $conversations = $this->sendRequest('messages.getConversations')->response->items;
        foreach ($conversations as $conversation) {
            $peer_id = $conversation->conversation->peer->id;
            //Messages ordered from newest to oldest!
            $messages = $this->sendRequest('messages.getHistory', ['peer_id' => $peer_id])->response->items;

            $message_date = $messages[count($messages) - 1]->date;
            if (!($message_date >= $this->date_from and $message_date <= $this->date_to)) {
                break;
            }

            $messages = self::filterMessageStacks($messages);

            //Ignore first message if it belongs to group
            $messages = self::filterFirstMessage($messages);

            //Ignore last message if group dont replied
            $messages = self::filterLastMessage($messages);

            for ($i = 0; $i < count($messages); $i += 2) {
                $reply_time = $messages[$i]->date - $messages[$i + 1]->date;
                if ($reply_time > self::OVERDUE_TIME) {
                    $overdue_conversations[] = $conversation->conversation;
                    break; //Prevent doubling
                }
            }
        }
        return $overdue_conversations;
    }
}
  • Вопрос задан
  • 308 просмотров
Подписаться 1 Простой 4 комментария
Решения вопроса 2
glaphire
@glaphire Куратор тега PHP
PHP developer
1. ссылка - не экономьте на autoloader, это не рационально, если уже начали использовать :)
2. ссылка - лучше собирать такие строки заранее в переменную и потом передавать как аргумент, неудобно поддерживать их "внутри скобок", а экономия на одной переменной ничего не даст
3. ссылка Структура респонса никак не описывается, т.е. нельзя ничего понять, пока не посмотришь в доку vk или руками не потестишь. Нужно или инкапсулировать респонс в класс, или дать ему какую-то предсказуемую структуру массива.
4. ccылка Внести больше ясности в "$key - 1" не описанием в phpdoc, а рефакторингом в максимально очевидный код.
5. ссылка Нет гарантии, что к элементу $messages[0] можно применить метод out, надо это как-то предотвращать.
6. ссылка конструктор затерялся среди множества методов, хорошо бы его вынести в начало :)
7. ссылка хорошим тоном считается выносить в константы любые числа, даже очевидные (60).

UPD.
1. Общее замечание - на входные параметры методов тоже надо typehints писать.
2. Много комментариев к коду - это признак того, что много неочевидных шагов, значит их надо дробить, инкапсулировать в методы, может даже выносить в отдельные классы, чтобы не перегружать текущий.
Ответ написан
Комментировать
index0h
@index0h
PHP, Golang. https://github.com/index0h
Критерии оценки беру тут: https://github.com/index0h/php-conventions конечно часть по оформлению, от части вкусовщина, но хотя бы PSR уважать надо.

1. В док блоках для свойств очень не плохо было бы указать типы и привести их к camelCase.
2. Почему private методы вначале класса?))
3. У вас отсутствует валидация аргументов в методах. Что будет, если в конструктор запихнуть не токен, а объект например, с остальными методами - тоже.
4. В приватных статических методах нет смысла (если честно статика в принципе штука довольно вредная, по ссылке описано почему).
5. filter* методы - не очень, откройте для себя array_shift и array_pop, вам в принципе нужны эти методы?
6. В методе getReplyTimes заюзайте array_filter
7. Я не знаю, что представляет собой $messages, но по хорошему - это должно быть что-то типа Entity с геттерами и сеттерами, с гарантиями структуры данных. Если это какой-то \stdClass - это очень печально.
Ответ написан
Пригласить эксперта
Ваш ответ на вопрос

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

Похожие вопросы