Защитите свой CodeReview на GitHub
В ответ на запрос функции по метафайлу я потратил несколько часов на создание этой функции в php .
Это приложение добавляет обзор экрана в ваш репозиторий GitHub (или где бы вы его ни захотели).
Он выглядит следующим образом:
Чтобы использовать его, используйте стандартное удержание изображения /ссылки, например:
[](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 (если они есть), и если я использую его, поскольку он предназначен для использования.
Любые комментарии также приветствуются.
3 ответа
В личной заметке ваш код действительно чист, идея блестящая, и я действительно надеюсь увидеть ее в ближайшее время.
Ваш код хорошо реализован и структурирован, но есть моменты синтаксиса, которые могут быть улучшены.
-
Я вижу много базовых
if-else
операторов, если вы используете троицы, их использование действительно уменьшает эти утверждения, но это за счет удобства чтения.Ниже приведены примеры использования:
($time < time() - 3600 ? fetchQuestion($qid, $db) : useData($row)); ($result ? useData($question) : print_r($stmt->errorInfo()));
Я действительно верю в некоторые версии поддержки PHP
(a == a ?: doStuff());
также, однако, я могу ошибаться. -
Есть несколько моментов, которые либо несовместимы, либо не соответствуют стандартам:
$apiCall = $apiCall + "?dummy";
Должен быть
$apiCall .= "?dummy";
, и вы не должны использовать+
при конкатенации строк, лучше использовать.
.Переключение между реализацией переменной непосредственно в строке
'words$varmorewords'
или ее добавлением:. $var .
, я бы порекомендовал последнее, поскольку он более читабельнее, и поскольку у первого могут быть проблемы, лучше всего вставить фигурные скобки:'words{$var}morewords'
вместо первого. -
Использование
curl
вместоget_file_contents
отлично, я вижу, что многие люди делают эту ошибку, и я даже тоже. -
У вас есть две пустые строки над оператором
return $json
вfetchQuestion()
, они не обязательно должны быть там. -
в
useData()
, вы создаете переменную$is_answered
, а затем никогда не используете ее, и я предлагаю заменить ее значение на$data['accepted_answer_id']
, чтобы ваш циклif
выглядел более чистым. -
Вы могли рассмотреть вопрос о сохранении SVG в другом файле и замене в ваших переменных, но я не на 100% от его репутации как стандартного кода /лучшей практики.
-
В
useData()
вместо выполнения двойной проверки (isset
: ( возвращает true для''
) и!= 0
), вы можете просто сравнить с> 0
. -
Вы извлекаете и сохраняете немало переменных для каждого сообщения, которые в настоящее время не используются в конечном изображении:"is_answered"
: Не используется,"view_count"
: Используется"favorite_count"
: Не используется,"answer_count"
: Не используется,"score"
: Не используется,"accepted_answer_id"
: не используется.Хотя я могу видеть будущие обновления, используя их, и захватывать их теперь отлично, но вы можете посмотреть на это.
По теме будущих реализаций счетчик счетчиков на кнопке тоже будет довольно крутым.
-
Вы могли бы рассмотреть возможность использования пространства имен
namespace
и структуры типа типа в ваш проект, чтобы его можно было использовать извне, проще.
Я думаю, это примерно так же красиво, как 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;
}
Он сохраняет остальную часть вашего кода чистой и красиво читаемой и изолирует мусор, а также мусор.
Юридическая информация:
Мой отзыв будет коротким длиннее, чем я ожидал, но я сосредоточусь только на функции useData()
.
Я внимательно прочитал его и сделал все возможное, чтобы улучшить его и сделать его более читаемым для вас.
Давайте начнем!
- Первое, что появляется в моей голове, - это гигантская куча бездереватого 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;
Теперь намного лучше!
- По-прежнему существует бесполезная атрибуция. Давайте также исправим это:
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;
- Хорошо, теперь намного лучше. Но у вас есть «бродячие» переменные, потерянные в вашем 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;
Лучше, не так ли?
- Но теперь вы хотите изменить цвет. Как бы вы это сделали? Измените все вручную?
Я предлагаю следующий (частичный) код:
$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
и сделал цвет по умолчанию.
- У вас есть следующий код:
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'];
}
- Этот пункт является чисто субъективным.
Вы слепо доверяете, что ваш код не имеет выхода до этой функции.
Вместо этого:
header('Content-type: image/svg+xml; charset=utf-8');
Подумайте об этом:
if (!headers_sent()) {
header('Content-type: image/svg+xml; charset=utf-8');
}
Если у вас возникла ошибка, она по-прежнему отправит SVG с предыдущими ошибками, но, по крайней мере, это не будет фабрикой ошибок!
Из-за того, что он субъективен, и не все согласны с этим, я решил удалить его из окончательного кода.
-
Как указано выше , у вас есть бесполезная переменная (
$is_answered
). Я также удалил его, так как он ничего не делает.
- Очень сложной задачей было бы изменить
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
или любые варианты, легче найти имя. Неявно вы разбиваете имя по _
и сравниваете по частям.
Попробуйте этот эксперимент:
- Сравните
aVeryInterestingMethodWellSpelled
сaveryinterestingmethodwellspelled
. - Сравните
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. Простой.