Защитите свой CodeReview на GitHub

В ответ на запрос функции по метафайлу я потратил несколько часов на создание этой функции в .

Это приложение добавляет обзор экрана в ваш репозиторий GitHub (или где бы вы его ни захотели).

Он выглядит следующим образом:

Обзор кода

Чтобы использовать его, используйте стандартное удержание изображения /ссылки, например:

[![Code Review](https://www.zomis.net/codereview/shield/?qid=54737)](https://codereview.stackexchange.com/q/54737/31562)

Замените 54737 идентификатором вопроса, на который вы хотите направить, и замените 31562 на свой идентификатор пользователя (чтобы вы могли получить Значки /Бустер /Публикация ).

Вы также можете использовать Code View Shield Creator для создания своего щита.

Я использовал shields.io , чтобы получить базовый SVG-XML и адаптировать его.

Существуют различные способы отображения значка:

  • Неотвеченные вопросы показывает оценку вопроса и красный фон
  • Ответные вопросы показывает количество ответов и оранжевый фон
  • Вопросы с принятым ответом показывают количество просмотров и зеленый фон

Как работает код

Из-за ежедневного ограничения API-интерфейса Stack Exchange из 10 000 запросов я избегаю слишком много запросов API, сохраняя предыдущие результаты в таблице базы данных и выполняя только новый запрос, если прошло больше часа с момента последнего запроса API для этого конкретного вопроса.

Код содержит следующие функции:

  • buildURL($apiCall, $site, $filter, $apiKey): создает URL-адрес для вызова API стека Exchange (для будущего использования я проверяю, содержит ли параметр apiCall '?' или нет) .
  • apiCall($apiCall, $site, $filter): выполняет HTTP-запрос API-интерфейс стека используя curl, возвращает данные JSON как чистую строку
  • fetchQuestion($qid, $db): использует данные JSON, полученные с помощью apiCall как ассоциативный массив, извлекает из него интересные данные, обновляет базу данных и вызывает useData ​​код>.
  • useData: принимает ассоциативный массив и создает для него SVG XML
  • useData($data): основная точка входа. dbOrAPI($qid, $db) - это идентификатор запроса кода и $qid - это объект PDO. Проверяет базу данных на наличие $db и использует ее, если она несколько обновлена, в противном случае вызывает qid

Этот код также доступен в https://github.com/Zomis/CodeReview-Shield

fetchQuestion

Основные проблемы

Прошло некоторое время с тех пор, как я использовал PHP, поэтому мне интересно узнать, придерживаюсь ли я соглашений PHP (если они есть), и если я использую его, поскольку он предназначен для использования.

Любые комментарии также приветствуются.

79 голосов | спросил Simon Forsberg 1 J000000Wednesday15 2015, 16:47:16

3 ответа


40

В личной заметке ваш код действительно чист, идея блестящая, и я действительно надеюсь увидеть ее в ближайшее время.


Ваш код хорошо реализован и структурирован, но есть моменты синтаксиса, которые могут быть улучшены.

  1. Я вижу много базовых if-else операторов, если вы используете троицы, их использование действительно уменьшает эти утверждения, но это за счет удобства чтения.

    Ниже приведены примеры использования:

    ($time < time() - 3600 ? fetchQuestion($qid, $db) : useData($row));
    ($result ? useData($question) : print_r($stmt->errorInfo()));
    

    Я действительно верю в некоторые версии поддержки PHP (a == a ?: doStuff()); также, однако, я могу ошибаться.

  2. Есть несколько моментов, которые либо несовместимы, либо не соответствуют стандартам:

    $apiCall = $apiCall + "?dummy"; 
    

    Должен быть $apiCall .= "?dummy";, и вы не должны использовать + при конкатенации строк, лучше использовать ..

    Переключение между реализацией переменной непосредственно в строке 'words$varmorewords' или ее добавлением: . $var ., я бы порекомендовал последнее, поскольку он более читабельнее, и поскольку у первого могут быть проблемы, лучше всего вставить фигурные скобки: 'words{$var}morewords' вместо первого.

  3. Использование curl вместо get_file_contents отлично, я вижу, что многие люди делают эту ошибку, и я даже тоже.

  4. У вас есть две пустые строки над оператором return $json в fetchQuestion(), они не обязательно должны быть там.

  5. в useData(), вы создаете переменную $is_answered, а затем никогда не используете ее, и я предлагаю заменить ее значение на $data['accepted_answer_id'], чтобы ваш цикл if выглядел более чистым.

  6. Вы могли рассмотреть вопрос о сохранении SVG в другом файле и замене в ваших переменных, но я не на 100% от его репутации как стандартного кода /лучшей практики.

  7. В useData() вместо выполнения двойной проверки (isset: ( возвращает true для '' ) и != 0), вы можете просто сравнить с > 0.

  8. Вы извлекаете и сохраняете немало переменных для каждого сообщения, которые в настоящее время не используются в конечном изображении:
    "is_answered": Не используется, "view_count": Используется "favorite_count": Не используется, "answer_count": Не используется, "score": Не используется, "accepted_answer_id": не используется.

    Хотя я могу видеть будущие обновления, используя их, и захватывать их теперь отлично, но вы можете посмотреть на это.

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

  9. Вы могли бы рассмотреть возможность использования пространства имен namespace и структуры типа типа в ваш проект, чтобы его можно было использовать извне, проще.

ответил Quill 1 J000000Wednesday15 2015, 17:47:37
26

Я думаю, это примерно так же красиво, как PHP: p (У меня было не забава, открывающая этот монстра .)


Одно небольшое предложение, которое у меня есть, заключается в устранении дублирования здесь:

    if (isset($question[$field_name])) {
        $sql_params[':' . $field_name] = $question[$field_name];
    } else {
        $sql_params[':' . $field_name] = 0;
    }

Я удивлен, наш хороший друг тройников @Quill забыл включить этого главного кандидата:

$sql_params[':' . $field_name] = isset($question[$field_name]) ? $question[$field_name] : 0;

Еще одна вещь, которая меня немного сбила, - это таинственная штука в середине кода:

$filter = "!)rcjzniPuafk4WNG65yr";

Что это? Откуда взялось это значение? Это важно?

Как и все магические константы, было бы неплохо поставить его в верхней части файла с описательным именем.


Наконец, возможно, это вообще невозможно, но было бы неплохо иметь возможность стилизовать дисплей с помощью CSS, вместо жестко закодированных значений $color.


Наконец, strpos($apiCall, '?') === false достаточно критический (ошибка PHP, а не ваш), что его целесообразно инкапсулировать в вспомогательный метод:

function contains($haystack, $needle) {
    return strpos($haystack, $needle) !== false;
}

Он сохраняет остальную часть вашего кода чистой и красиво читаемой и изолирует мусор, а также мусор.

ответил janos 1 J000000Wednesday15 2015, 22:17:21
17

Юридическая информация:

Мой отзыв будет коротким длиннее, чем я ожидал, но я сосредоточусь только на функции useData().

Я внимательно прочитал его и сделал все возможное, чтобы улучшить его и сделать его более читаемым для вас.


Давайте начнем!

  1. Первое, что появляется в моей голове, - это гигантская куча бездереватого SVG:
     $svg = <<<END
<svg xmlns="http://www.w3.org/2000/svg" width="137" height="20">
<linearGradient id="b" x2="0" y2="100%">
<stop offset="0" stop-color="#bbb" stop-opacity=".1"/>
<stop offset="1" stop-opacity=".1"/>
</linearGradient>
<mask id="a">
<rect width="137" height="20" rx="3" fill="#fff"/>
</mask>
<g mask="url(#a)">
<path fill="#555" d="M0 0h62v20H0z"/>
<path fill="#$color" d="M62 0h75v20H62z"/>
<path fill="url(#b)" d="M0 0h137v20H0z"/>
</g>
<g fill="#fff" text-anchor="middle" font-family="DejaVu Sans,Verdana,Geneva,sans-serif" font-size="11">
<text x="31" y="15" fill="#010101" fill-opacity=".3">$text</text>
<text x="31" y="14">$text</text>
<text x="98.5" y="15" fill="#010101" fill-opacity=".3">$right</text>
<text x="98.5" y="14">$right</text>
</g>
</svg>
END;
    echo $svg;

Для этого обязательно потребуется отступы. Это полный беспорядок! Рассмотрим это:

     $svg = <<<END
<svg xmlns="http://www.w3.org/2000/svg" width="137" height="20">
    <linearGradient id="b" x2="0" y2="100%">
        <stop offset="0" stop-color="#bbb" stop-opacity=".1"/>
        <stop offset="1" stop-opacity=".1"/>
    </linearGradient>
    <mask id="a">
        <rect width="137" height="20" rx="3" fill="#fff"/>
    </mask>
    <g mask="url(#a)">
        <path fill="#555" d="M0 0h62v20H0z"/>
        <path fill="#$color" d="M62 0h75v20H62z"/>
        <path fill="url(#b)" d="M0 0h137v20H0z"/>
    </g>
    <g fill="#fff" text-anchor="middle" font-family="DejaVu Sans,Verdana,Geneva,sans-serif" font-size="11">
        <text x="31" y="15" fill="#010101" fill-opacity=".3">$text</text>
        <text x="31" y="14">$text</text>
        <text x="98.5" y="15" fill="#010101" fill-opacity=".3">$right</text>
        <text x="98.5" y="14">$right</text>
    </g>
</svg>
END;
    echo $svg;

Теперь намного лучше!


  1. По-прежнему существует бесполезная атрибуция. Давайте также исправим это:
 echo <<<END
<svg xmlns="http://www.w3.org/2000/svg" width="137" height="20">
    <linearGradient id="b" x2="0" y2="100%">
        <stop offset="0" stop-color="#bbb" stop-opacity=".1"/>
        <stop offset="1" stop-opacity=".1"/>
    </linearGradient>
    <mask id="a">
        <rect width="137" height="20" rx="3" fill="#fff"/>
    </mask>
    <g mask="url(#a)">
        <path fill="#555" d="M0 0h62v20H0z"/>
        <path fill="#$color" d="M62 0h75v20H62z"/>
        <path fill="url(#b)" d="M0 0h137v20H0z"/>
    </g>
    <g fill="#fff" text-anchor="middle" font-family="DejaVu Sans,Verdana,Geneva,sans-serif" font-size="11">
        <text x="31" y="15" fill="#010101" fill-opacity=".3">$text</text>
        <text x="31" y="14">$text</text>
        <text x="98.5" y="15" fill="#010101" fill-opacity=".3">$right</text>
        <text x="98.5" y="14">$right</text>
    </g>
</svg>
END;

  1. Хорошо, теперь намного лучше. Но у вас есть «бродячие» переменные, потерянные в вашем SVG.

Чтобы упростить чтение, рассмотрите перенос переменных в скобках:

echo <<<END
<svg xmlns="http://www.w3.org/2000/svg" width="137" height="20">
    <linearGradient id="b" x2="0" y2="100%">
        <stop offset="0" stop-color="#bbb" stop-opacity=".1"/>
        <stop offset="1" stop-opacity=".1"/>
    </linearGradient>
    <mask id="a">
        <rect width="137" height="20" rx="3" fill="#fff"/>
    </mask>
    <g mask="url(#a)">
        <path fill="#555" d="M0 0h62v20H0z"/>
        <path fill="#{$color}" d="M62 0h75v20H62z"/>
        <path fill="url(#b)" d="M0 0h137v20H0z"/>
    </g>
    <g fill="#fff" text-anchor="middle" font-family="DejaVu Sans,Verdana,Geneva,sans-serif" font-size="11">
        <text x="31" y="15" fill="#010101" fill-opacity=".3">{$text}</text>
        <text x="31" y="14">{$text}</text>
        <text x="98.5" y="15" fill="#010101" fill-opacity=".3">{$right}</text>
        <text x="98.5" y="14">{$right}</text>
    </g>
</svg>
END;

Лучше, не так ли?


  1. Но теперь вы хотите изменить цвет. Как бы вы это сделали? Измените все вручную?

Я предлагаю следующий (частичный) код:

 $colors = array(
    'gradient'=>'bbb',
    'mask'=>'fff',
    'back'=>array('555', 'e05d44'),
    'text'=>'010101',
    'right'=>'010101'
);
if (isset($data['accepted_answer_id']) && $data['accepted_answer_id'] != 0) {
    $color['back'][1] = '97ca00';
    $mode = 'views';
} elseif ($data['answer_count'] >= 1) {
    $colors['back'][1] = 'ff8000';
    $right = $data['score'] . ' score';
    $mode = 'answers';
} else {
    $text = 'reviewing';
    $mode = 'score';
}

// [...]

    echo <<<END
<svg xmlns="http://www.w3.org/2000/svg" width="137" height="20">
    <linearGradient id="b" x2="0" y2="100%">
        <stop offset="0" stop-color="#{$colors['gradient']}" stop-opacity=".1"/>
        <stop offset="1" stop-opacity=".1"/>
    </linearGradient>
    <mask id="a">
        <rect width="137" height="20" rx="3" fill="#{$colors['mask']}"/>
    </mask>
    <g mask="url(#a)">
        <path fill="#{$colors['back'][0]}" d="M0 0h62v20H0z"/>
        <path fill="#{$colors['back'][1]}" d="M62 0h75v20H62z"/>
        <path fill="url(#b)" d="M0 0h137v20H0z"/>
    </g>
    <g fill="#fff" text-anchor="middle" font-family="DejaVu Sans,Verdana,Geneva,sans-serif" font-size="11">
        <text x="31" y="15" fill="#{$colors['text']}" fill-opacity=".3">{$text}</text>
        <text x="31" y="14">{$text}</text>
        <text x="98.5" y="15" fill="#{$colors['right']}" fill-opacity=".3">{$right}</text>
        <text x="98.5" y="14">{$right}</text>
    </g>
</svg>
END;

Обратите внимание, что я удалил атрибут цвета для переменной $colors в else и сделал цвет по умолчанию.


  1. У вас есть следующий код:
 if (isset($_GET['mode'])) {
    $mode = $_GET['mode'];
}
$data['answers'] = $data['answer_count'];
$data['views'] = $data['view_count'];
$right = $data[$mode] . ' ' . $mode;

Вы чувствуете запах? Я чувствую запах кода!

Пожалуйста, всегда проверяйте свой ввод .

Вместо этого используйте if:

if (isset($_GET['mode']) && in_array($_GET['mode'], array('views','answers','score'))) {
    $mode = $_GET['mode'];
}

  1. Этот пункт является чисто субъективным.

Вы слепо доверяете, что ваш код не имеет выхода до этой функции.

Вместо этого:

header('Content-type: image/svg+xml; charset=utf-8');

Подумайте об этом:

if (!headers_sent()) {
    header('Content-type: image/svg+xml; charset=utf-8');
}

Если у вас возникла ошибка, она по-прежнему отправит SVG с предыдущими ошибками, но, по крайней мере, это не будет фабрикой ошибок!

Из-за того, что он субъективен, и не все согласны с этим, я решил удалить его из окончательного кода.


  1. Как указано выше , у вас есть бесполезная переменная ($is_answered). Я также удалил его, так как он ничего не делает.

  1. Очень сложной задачей было бы изменить echo <<<END на echo <<<SVG.

Это показывает, что такое эхо, и что такое огромный блок, без чтения более 12 символов.


Конечный результат:

Вот как выглядит код, с дополнительными линиями для повышения удобочитаемости:

 function useData($data) {
    header('Content-type: image/svg+xml; charset=utf-8');

    $is_answered = $data['text'];
    $text = 'reviewed';
    $colors = array(
        'gradient'=>'bbb',
        'mask'=>'fff',
        'back'=>array('555', 'e05d44'),
        'text'=>'010101',
        'right'=>'010101'
    );

    if (isset($data['accepted_answer_id']) && $data['accepted_answer_id'] != 0) {
        $color['back'][1] = '97ca00';
        $mode = 'views';
    } elseif ($data['answer_count'] >= 1) {
        $colors['back'][1] = 'ff8000';
        $right = $data['score'] . ' score';
        $mode = 'answers';
    } else {
        $text = 'reviewing';
        $mode = 'score';
    }

    if (isset($_GET['mode']) && in_array($_GET['mode'], array('views','answers','score'))) {
        $mode = $_GET['mode'];
    }

    $data['answers'] = $data['answer_count'];
    $data['views'] = $data['view_count'];

    $right = $data[$mode] . ' ' . $mode;

    echo <<<SVG
<svg xmlns="http://www.w3.org/2000/svg" width="137" height="20">
    <linearGradient id="b" x2="0" y2="100%">
        <stop offset="0" stop-color="#{$colors['gradient']}" stop-opacity=".1"/>
        <stop offset="1" stop-opacity=".1"/>
    </linearGradient>
    <mask id="a">
        <rect width="137" height="20" rx="3" fill="#{$colors['mask']}"/>
    </mask>
    <g mask="url(#a)">
        <path fill="#{$colors['back'][0]}" d="M0 0h62v20H0z"/>
        <path fill="#{$colors['back'][1]}" d="M62 0h75v20H62z"/>
        <path fill="url(#b)" d="M0 0h137v20H0z"/>
    </g>
    <g fill="#fff" text-anchor="middle" font-family="DejaVu Sans,Verdana,Geneva,sans-serif" font-size="11">
        <text x="31" y="15" fill="#{$colors['text']}" fill-opacity=".3">{$text}</text>
        <text x="31" y="14">{$text}</text>
        <text x="98.5" y="15" fill="#{$colors['right']}" fill-opacity=".3">{$right}</text>
        <text x="98.5" y="14">{$right}</text>
    </g>
</svg>
SVG;
}

Побочные примечания:

Прежде чем говорить что-нибудь, это в первую очередь основано на мнениях и не объективно!


Я не думаю, что useData - хорошее имя.

Я настоятельно рекомендую изменить его на lowercase_and_underscore (a.k.a. snake_case).

Почему? Если вы по ошибке напишете usedata, вам будет сложно просмотреть "Where in the living fudge is this declared???", чтобы заметить, что у вас есть орфографическая ошибка имя функции и что PHP не заботится об обсадке в имени функции.

Если вы пишете USE_DATA, Use_Data или любые варианты, легче найти имя. Неявно вы разбиваете имя по _ и сравниваете по частям.
Попробуйте этот эксперимент:

  1. Сравните aVeryInterestingMethodWellSpelled с averyinterestingmethodwellspelled.
  2. Сравните a_Very_Interesting_Method_Well_Spelled с a_very_interesting_method_well_spelled.

Какой из них легче сравнивать?


Я не согласен с https://softwareengineering.stackexchange.com/questions/196416/whats-the-dominant-naming-convention-for-variables-in-php-camelcase-or-undersc по использованию camelCase для этой точной точки.

Кроме того, сам PHP не следует этому! Посмотрите на все имена функций.


Но даже если вы измените имя на use_data, это будет плохое имя.

Почему? Ну, название дает идею, что вы пытаетесь использовать некоторые данные, чтобы что-то сделать. Но что он делает? Я не знаю, я должен прочитать всю функцию, чтобы знать.

Моя рекомендация: print_svg.
Он показывает, что делает код: выводит SVG. Простой.

ответил Ismael Miguel 2 J000000Thursday15 2015, 13:49:22

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

Популярные теги

security × 330linux × 316macos × 2827 × 268performance × 244command-line × 241sql-server × 235joomla-3.x × 222java × 189c++ × 186windows × 180cisco × 168bash × 158c# × 142gmail × 139arduino-uno × 139javascript × 134ssh × 133seo × 132mysql × 132