Home ยป Shielding your CodeReview on GitHub

Shielding your CodeReview on GitHub

Solutons:


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.

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.

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.

Related Solutions

How to play YouTube audio in background/minimised?

Here's a solution using entirely free and open source software. The basic idea is that although YouTube can't play clips in the background, VLC for Android can play clips in the background, so all we need to do is pipe the clip to VLC where we can listen to it...

Why not use “which”? What to use then?

Here is all you never thought you would ever not want to know about it: Summary To get the pathname of an executable in a Bourne-like shell script (there are a few caveats; see below): ls=$(command -v ls) To find out if a given command exists: if command -v...

Split string into Array of Arrays [closed]

If I got correct what you want to receive as a result, then this code would make what you want: extension Array { func chunked(into size: Int) -> [[Element]] { return stride(from: 0, to: self.count, by: size).map { Array(self[$0 ..< Swift.min($0 + size,...

Retrieving n rows per group

Let's start with the basic scenario. If I want to get some number of rows out of a table, I have two main options: ranking functions; or TOP. First, let's consider the whole set from Production.TransactionHistory for a particular ProductID: SELECT...

Don’t understand how my mum’s Gmail account was hacked

IMPORTANT: this is based on data I got from your link, but the server might implement some protection. For example, once it has sent its "silver bullet" against a victim, it might answer with a faked "silver bullet" to the same request, so that anyone...

What is /storage/emulated/0/?

/storage/emulated/0/Download is the actual path to the files. /sdcard/Download is a symlink to the actual path of /storage/emulated/0/Download However, the actual files are located in the filesystem in /data/media, which is then mounted to /storage/emulated/0...

How can I pass a command line argument into a shell script?

The shell command and any arguments to that command appear as numbered shell variables: $0 has the string value of the command itself, something like script, ./script, /home/user/bin/script or whatever. Any arguments appear as "$1", "$2", "$3" and so on. The...

What is pointer to string in C?

argv is an array of pointers pointing to zero terminated c-strings. I painted the following pretty picture to help you visualize something about the pointers. And here is a code example that shows you how an operating system would pass arguments to your...

How do mobile carriers know video resolution over HTTPS connections?

This is an active area of research. I happen to have done some work in this area, so I'll share what I can about the basic idea (this work was with industry partners and I can't share the secret details ๐Ÿ™‚ ). The tl;dr is that it's often possible to identify an...

How do I change the name of my Android device?

To change the hostname (device name) you have to use the terminal (as root): For Eclair (2.1): echo MYNAME > /proc/sys/kernel/hostname For Froyo (2.2): (works also on most 2.3) setprop net.hostname MYNAME Then restart your wi-fi. To see the change, type...

How does reverse SSH tunneling work?

I love explaining this kind of thing through visualization. ๐Ÿ™‚ Think of your SSH connections as tubes. Big tubes. Normally, you'll reach through these tubes to run a shell on a remote computer. The shell runs in a virtual terminal (tty). But you know this part...

Difference between database vs user vs schema

In Oracle, users and schemas are essentially the same thing. You can consider that a user is the account you use to connect to a database, and a schema is the set of objects (tables, views, etc.) that belong to that account. See this post on Stack Overflow:...

What’s the output of this code written in java?

//if you're using Eclipse, press ctrl-shift-f to "beautify" your code and make it easier to read int arr[] = new int[3]; //create a new array containing 3 elements for (int i = 0; i < 3; i++) { arr[i] = i;//assign each successive value of i to an entry in...

How safe are password managers like LastPass?

We should distinguish between offline password managers (like Password Safe) and online password managers (like LastPass). Offline password managers carry relatively little risk. It is true that the saved passwords are a single point of failure. But then, your...

Can anyone tell me why this program go to infinite times?

while (i <= 2) { while (i > 0) { a = a + b; i--; <- out the inner while loop when i = 0 } printf("%d", a); i++; <- at here, the i==0 each time, so infinity loop } Because your nested loop always restores the value of i to 0, And 0 <= 2 is always...