2
\$\begingroup\$

I have written low quality php code, I'm not a good coder ;) Help me, please, to optimise it ;)

This simple code check bitstamp.net exchange rate, compare it with previous rate and print the result.

<?php
function getPrice($url) {
    $decode = @file_get_contents($url);
    return json_decode($decode, true);
}

$btc = getPrice('https://api.binance.com/api/v1/ticker/24hr?symbol=BTCUSDT');
$btcusdlast = round($btc["lastPrice"], 1);
$rbtc = ($btc["priceChange"]>0) ? 'up' : ' down';

$ltc = getPrice('https://api.binance.com/api/v1/ticker/24hr?symbol=LTCUSDT');
$ltcusdlast = round($ltc["lastPrice"], 1);
$rltc = ($ltc["priceChange"]>0) ? 'up' : ' down';

?>

    <h2>BTC 
    $ <?=$btcusdlast, ' ', $rbtc;?></h2>

    <h2>LTC 
    $ <?=$ltcusdlast, ' ', $rltc;?></h2>

But it's low quality and work very slowly.

\$\endgroup\$
3
  • 1
    \$\begingroup\$ "and work very slowly." Since you’re calling an external site, it can't work faster than your internet connection speed… \$\endgroup\$ Jul 3 '18 at 14:45
  • \$\begingroup\$ How to avoid it? Maybe there is some timeout or I should write it to a file and than read it? \$\endgroup\$
    – topkurs
    Jul 3 '18 at 15:01
  • \$\begingroup\$ Hey, welcome to Code Review! I changed your title to what your code achieves, as we all want to improve our code here :) \$\endgroup\$
    – Graipher
    Jul 3 '18 at 15:13
1
\$\begingroup\$

Id make your code more functional (if your not going to use OOP) the fact that this code is slow is probably your internet connection (as mentioned in the comments)

First as the api is returning common keys I would just make the rounding & ternary operator as one function you can pass a response to,

Also because you aren't using full names and just short codes the whole code works in a loop, this way you can just add more coin short codes into the $coinsToFetch array

<?php
function fetchAndDecodeData($url)
{
  return json_decode(@file_get_contents($url), true);
}

function getValuesFromBinanceData($data)
{
  return [
    "priceChange"=>$data["priceChange"] > 0 ? "up" : "down",
    "lastPrice"=>round($data["lastPrice"], 1)
  ];
}

$coinsToFetch = ["BTC", "LTC"];

foreach ($coinsToFetch as $coin) {
  $coinData = fetchAndDecodeData('https://api.binance.com/api/v1/ticker/24hr?symbol=' . $coin . 'USDT');
  $data = getValuesFromBinanceData($coinData);
  echo "<h2> $coin " . $data['lastPrice'] . $data['priceChange'] . "</h2>";
}

To address the issue of the speed of your script in relation to your internet you could cache the results (redis) or store them in database (mysql) this would allow you to do some sort of code like this (psudeo code)

If its less than 5 minutes sinse the cache
  Load the data from the cache
Else 
  Load from binance api

If you are going to use the caching / storage I would convert this into two scripts;

  1. That runs as cron job to populate cache / storage every x number of minutes
  2. To display the data
\$\endgroup\$
2
  • \$\begingroup\$ Thanks a lot, but script is loading with 500 error :( \$\endgroup\$
    – topkurs
    Jul 3 '18 at 19:00
  • \$\begingroup\$ Ive updated the code it should be fine now \$\endgroup\$
    – Dan
    Jul 4 '18 at 5:39

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.