GitHubでのCodeReviewの保護


94

a feature-request on metaに応えて、で機能を作成するために本日数時間を費やしました。

このアプリケーションは、レビューシールドをGitHubリポジトリ(または他の場所)に追加します。

次のようになります:

Code Review

これを使用するには、次のように標準の画像/リンクマークダウンを使用します:

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

54737を目的の質問IDに置き換え、31562をユーザーIDに置き換えます(Announcer / Booster / Publicistバッジを取得できるようにするため)。

Code Review Shield Creatorを使用してシールドを作成することもできます。

私はshields.ioを使用して基本的なSVG XMLを取得し、それを適合させました。

バッジを表示する方法はいくつかあります:

  • 未回答の質問は、質問のスコアと赤い背景を表示します
  • 回答済みの質問には、回答の数とオレンジ色の背景が表示されます
  • 回答が承認された質問は、視聴回数と緑の背景を示しています

コードの仕組み

1日のStack Exchange APIの制限は10,000リクエストであるため、以前の結果をデータベーステーブルに保存し、最後のAPIリクエストから1時間以上経過していない場合にのみ新しいリクエストを実行することで、APIリクエストが多すぎないようにしています。その特定の質問のために。

コードには次の関数が含まれています:

  • buildURL($apiCall, $site, $filter, $apiKey):Stack Exchange API呼び出しのURLを作成します(将来の使用のために、apiCallパラメーターに「?」が含まれているかどうかを確認します)。
  • apiCall($apiCall, $site, $filter)curlを使用してStack Exchange APIへのHTTPリクエストを実行し、JSONデータを純粋な文字列として返します
  • fetchQuestion($qid, $db)apiCallによって取得されたJSONデータを連想配列として使用し、そこから対象データを抽出してデータベースを更新し、useDataを呼び出します。
  • useData($data):連想配列を取り、SVG XMLを作成します
  • dbOrAPI($qid, $db):主なエントリポイント。315620はコードレビューの質問IDで、315621はPDOオブジェクトです。データベースをチェックして315622の存在を確認し、それが多少最新の場合はそれを使用します。それ以外の場合は315623を呼び出します

このコードはhttps://github.com/Zomis/CodeReview-Shieldでも利用可能

<?php

require 'conf.php';

function buildURL($apiCall, $site, $filter, $apiKey) {
    if (strpos($apiCall, '?') === false) {
        $apiCall = $apiCall + "?dummy";
    }
    return "https://api.stackexchange.com/2.2/" . $apiCall
                . "&site=" . $site
                . "&filter=" . $filter . "&key=" . $apiKey;
}

function apiCall($apiCall, $site, $filter) {
    global $apiKey;
    $url = buildURL($apiCall, $site, $filter, $apiKey);
    $ch = curl_init($url);
    curl_setopt($ch, CURLOPT_ENCODING, 'gzip');
    curl_setopt($ch, CURLOPT_RETURNTRANSFER, true);
    $result = curl_exec($ch);

    if ($result === false) {
        $error = curl_error($ch);
        curl_close($ch);
        throw new Exception("Error calling Stack Exchange API: $error");
    }
    curl_close($ch);
    return $result;
}

function fetchQuestion($qid, $db) {
    $filter = "!)rcjzniPuafk4WNG65yr";
    $data = apiCall("questions/$qid?order=desc&sort=activity", 'codereview', $filter);
    $json = json_decode($data, true);
    $question = $json['items'][0];
    $dbfields = array("is_answered", "view_count", "favorite_count", "answer_count", "score", "accepted_answer_id");


    $sql = 'INSERT INTO cr_badge (question_id, is_answered, favorite_count, answer_count, view_count, score, fetch_time, accepted_answer_id) ' .
        'VALUES (:qid, :is_answered, :favorite_count, :answer_count, :view_count, :score, :time, :accepted_answer_id) ON DUPLICATE KEY UPDATE ' .
        'is_answered = :is_answered, favorite_count = :favorite_count, answer_count = :answer_count, view_count = :view_count, score = :score, fetch_time = :time, accepted_answer_id = :accepted_answer_id ;';
    $stmt = $db->prepare($sql);
    $sql_params = array();
    foreach ($dbfields as $field_name) {
        if (isset($question[$field_name])) {
            $sql_params[':' . $field_name] = $question[$field_name];
        } else {
            $sql_params[':' . $field_name] = 0;
        }
    }
    $sql_params[':qid'] = $qid;
    $sql_params[':time'] = time();
    $result = $stmt->execute($sql_params);
    if ($result) {
        useData($question);
    } else {
        print_r($stmt->errorInfo());
    }


    return $json;
}

function useData($data) {
    header('Content-type: image/svg+xml; charset=utf-8');
    $is_answered = $data['text'];
    $text = 'reviewed';
    if (isset($data['accepted_answer_id']) && $data['accepted_answer_id'] != 0) {
        $color = '97ca00';
        $mode = 'views';
    } elseif ($data['answer_count'] >= 1) {
        $color = 'ff8000';
        $right = $data['score'] . ' score';
        $mode = 'answers';
    } else {
        $color = 'e05d44';
        $text = 'reviewing';
        $mode = 'score';
    }
    if (isset($_GET['mode'])) {
        $mode = $_GET['mode'];
    }
    $data['answers'] = $data['answer_count'];
    $data['views'] = $data['view_count'];
    $right = $data[$mode] . ' ' . $mode;

    $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;
}

function dbOrAPI($qid, $db) {

    $sql = 'SELECT is_answered, favorite_count, answer_count, view_count, score, fetch_time, accepted_answer_id FROM cr_badge WHERE question_id = :qid;';

    $stmt = $db->prepare($sql);
    $result = $stmt->execute(array(':qid' => $qid));
    if ($result) {
        $row = $stmt->fetch(PDO::FETCH_ASSOC);
        $time = $row['fetch_time'];
        if ($time < time() - 3600) { // if time was updated more than one hour ago
            // fetch data again
            fetchQuestion($qid, $db);
        } else {
            useData($row);
        }
    } else {
        print_r($stmt->errorInfo());
    }
}

if (isset($_GET['qid'])) {
    $qid = $_GET['qid'];
} else {
    die("No qid set");
}

try {
    $db = new PDO($dbhostname, $dbuser, $dbpass);
} catch (PDOException $e) {
    echo 'Connection failed: ' . $e->getMessage();
    return false;
}

dbOrAPI($qid, $db);

主な懸念事項

PHPを使用して久しぶりなので、PHPの規則(ある場合)に準拠しているかどうか、および使用するために使用しているかどうかを知りたいです。

コメントも歓迎します。

47

On a personal note, your code is really clean, the idea is brilliant, and I really hope to see it implemented soon.


Your code is well implemented and structured, but there's syntax points that could be improved.

  1. I see a lot of basic if-else statements, if you're into ternaries, using them really slims down these statements, but it's at the cost of readability.

    See the following examples for usage:

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

    I do believe some versions of PHP support (a == a ?: doStuff()); syntax also, however, I could be mistaken.

  2. There's a few points that are either inconsistent or don't adhere to standards:

    $apiCall = $apiCall + "?dummy"; 
    

    Should be $apiCall .= "?dummy";, and you shouldn't use + when concatenating strings, it's best to use . instead.

    Switching between implementing the variable directly in the string 'words$varmorewords' or adding it like:. $var . , I would recommend the latter as it's more reader-friendly, and because the former can have issues, it's best to wrap in curly braces: 'words{$var}morewords' in place of the former.

  3. Using curl instead of get_file_contents is great, I see a lot of people make that mistake, and I've even too.

  4. You have two blank lines above your return $json statement in fetchQuestion(), they don't need to be there.

  5. in useData(), you create the variable $is_answered and then never use it, and I'd suggest replacing its value with $data['accepted_answer_id'] so that your if loop looks cleaner.

  6. You could consider keeping the SVG in another file and replacing in your variables, but I'm not 100% on its standing as a code standard / best practice.

  7. In useData(), rather than doing a double check (isset: (returns true for '') and != 0), you can just compare to > 0.

  8. You retrieve and store quite a few variables for each post that aren't currently used in the final image:
    "is_answered" : Not used, "view_count": Used, "favorite_count": Not used, "answer_count": Not used, "score": Not used, "accepted_answer_id": Not used.

    Although I can see future updates using these, and capturing them now is great, but, you could look at modifying that.

    On the topic of future implementations, a score counter on the button would be pretty cool too.

  9. You could consider implementing a namespace and class like structure into your project so it can be used externally, easier.


28

I guess this is about as beautiful as PHP gets :p (I had not fun discovering this monstrosity.)


One minor suggestion I have is to eliminate the duplication here:

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

I'm surprised our good friend of ternarys @Quill forgot to include this prime candidate:

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

Another thing that put me off a little bit is this mysterious piece in the middle the code:

$filter = "!)rcjzniPuafk4WNG65yr";

What is this about? Where did this value come from? Is it important?

Like all magic constants, it would be good to put it near the top of the file with a descriptive name.


Lastly, maybe it's not feasible at all, but it would be nice to be able to style the display using CSS, instead of the hard-coded $color values.


Finally, strpos($apiCall, '?') === false is cryptic enough (fault of PHP, not yours), that it might be worth encapsulating it in a helper method:

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

It keeps the rest of your code clean and nicely readable, and isolates the, ahem, garbage.


19

Disclamer:

My review will be short is longer than I expected but I will only focus on the function useData().

I've read it carefully and did my best to improve it and make it more readable for you.


Lets get it started!

  1. The first thing that pops in my head is that giant pile of un-indented 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;

It sure need some indentation. It's a total mess! Consider this:

    $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;

So much better now!


  1. There's still an useless attribution. Lets fix that too:
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. Alright, much better now. But you have 'stray' variables lost within your SVG.

To make it easier to read, consider wrapping the variables in brackets:

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;

Way better, isn't it?


  1. But now, you want to change a color. How would you do it? Change everything by hand?

I propose the following (partial) code:

$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;

Notice that I've removed the color attribution on the variable $colors on the else, and made it the default color.


  1. You have the following code:
if (isset($_GET['mode'])) {
    $mode = $_GET['mode'];
}
$data['answers'] = $data['answer_count'];
$data['views'] = $data['view_count'];
$right = $data[$mode] . ' ' . $mode;

Do you smell that? I smell code injection!

Please, always validate your input.

Simply use this if instead:

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

  1. This point is purely subjective.

You are blindly trusting that your code has no output before this function.

Instead of this:

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

Consider using this:

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

In case you happen to have an error, it will still send the SVG with the previous errors, but at least it won't be an error factory!

Due to it being subjective and not everybody agreeing on it, I've decided to remove it from the final code.


  1. As pointed out before, you have an useless variable ($is_answered). I've removed it as well, since it isn't doing anything there.

  1. A very picky point would be to change echo <<<END to echo <<<SVG.

This shows what the echo is all about and what is that huge block, without reading more than 12 characters.


Final result:

This is what the code looks like, with additional lines to increase readability:

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

Side-notes:

Before you say anything, this is primarly opinion-based and not objective!


I don't think that useData is a good name.

I strongly recommend to change it to lowercase_and_underscore (a.k.a. snake_case).

Why is that? If you write usedata by mistake, you will have an hard time to look into "Where in the living fudge is this declared???" just to notice that you have misspelled the name of the function and that PHP doesn't care about casing in the function name.

If you write USE_DATA, Use_Data or any variation, it is easier to find the name. Implicitly you split the name by the _ and compare part by part.
Try this experiment:

  1. Compare aVeryInterestingMethodWellSpelled with averyinterestingmethodwellspelled.
  2. Compare a_Very_Interesting_Method_Well_Spelled with a_very_interesting_method_well_spelled.

Which one is easier to compare?


I disagree with https://softwareengineering.stackexchange.com/questions/196416/whats-the-dominant-naming-convention-for-variables-in-php-camelcase-or-undersc on using camelCase for this exact point.

Also, PHP itself doesn't follow this! Look at all the function names.


But, even if you change the name to use_data, it will be a bad name.

Why is that? Well, the name gives the idea that you are trying to use some data to do something. But what is it doing? I don't know, I have to read the whole function to know.

My recomendation: print_svg.
It shows preciselly what the code does: it outputs SVG. Simple.