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

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

---

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

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 и структуры типа типа в ваш проект, чтобы его можно было использовать извне, проще.

Я думаю, это примерно так же красиво, как 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().

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

---

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

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;

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

---

2. По-прежнему существует бесполезная атрибуция. Давайте также исправим это:

 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;

---

3. Хорошо, теперь намного лучше. Но у вас есть «бродячие» переменные, потерянные в вашем 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;

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

---

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

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

 $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 и сделал цвет по умолчанию.

---

5. У вас есть следующий код:

 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'];
}

---

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

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

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

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

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

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

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

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

---

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

---

8. Очень сложной задачей было бы изменить 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. Простой.