Is there a better way cycle through these PHP codes, maybe with foreach loop?

Question

I use file_get_contents to grab a remote pricing (updated daily), use substr to keep only the portions I want (stripping out the currency symbols and other data from the output and only keeping the numbers) and use file_put_contents to store it into a cache directory which I refer to later.

This is what I have now:-

<?php

$cacheDirectory = $_SERVER['DOCUMENT_ROOT'] . '/cache/';

// Small Plan - US
$cachefile_SM_US = $cacheDirectory . 'SM_US.cache';

if(file_exists($cachefile_SM_US)) {
    if(time() - filemtime($cachefile_SM_US) > 1600) {
        // too old , re-fetch
        $cache_SM_US = file_get_contents('https://remotedomain.com/?get=price&product=10&currency=1');
        $substr_SM_US = substr($cache_SM_US,17,2);
        file_put_contents($cachefile_SM_US, $substr_SM_US);
        } else {
            // cache is still fresh
    }
} else {
    // no cache, create one
    $cache_SM_US = file_get_contents('https://remotedomain.com/?get=price&product=10&currency=1');
    $substr_SM_US = substr($cache_SM_US,17,2);
    file_put_contents($cachefile_SM_US, $substr_SM_US);
}

// Large Plan - US
$cachefile_LG_US = $cacheDirectory . 'LG_US.cache';

if(file_exists($cachefile_LG_US)) {
    if(time() - filemtime($cachefile_LG_US) > 1600) {
        // too old , re-fetch
        $cache_LG_US = file_get_contents('https://remotedomain.com/?get=price&product=20&currency=1');
        $substr_LG_US = substr($cache_LG_US,17,2);
        file_put_contents($cachefile_LG_US, $substr_LG_US);
    } else {
        // cache is still fresh
    }
} else {
    // no cache, create one
    $cache_LG_US = file_get_contents('https://remotedomain.com/?get=price&product=20&currency=1');
    $substr_LG_US = substr($cache_LG_US,17,2);
    file_put_contents($cachefile_LG_US, $substr_LG_US);
}

// Small Plan - EU
$cachefile_SM_EU = $cacheDirectory . 'SM_EU.cache';

if(file_exists($cachefile_SM_EU)) {
    if(time() - filemtime($cachefile_SM_EU) > 1600) {
        // too old , re-fetch
        $cache_SM_EU = file_get_contents('https://remotedomain.com/?get=price&product=10&currency=2');
        $substr_SM_EU = substr($cache_SM_EU,17,2);
        file_put_contents($cachefile_SM_EU, $substr_SM_EU);
        } else {
            // cache is still fresh
    }
} else {
    // no cache, create one
    $cache_SM_EU = file_get_contents('https://remotedomain.com/?get=price&product=10&currency=2');
    $substr_SM_EU = substr($cache_SM_EU,17,2);
    file_put_contents($cachefile_SM_EU, $substr_SM_EU);
}

// Large Plan - EU
$cachefile_LG_EU = $cacheDirectory . 'LG_EU.cache';

if(file_exists($cachefile_LG_EU)) {
    if(time() - filemtime($cachefile_LG_EU) > 1600) {
        // too old , re-fetch
        $cache_LG_EU = file_get_contents('https://remotedomain.com/?get=price&product=20&currency=2');
        $substr_LG_EU = substr($cache_LG_EU,17,2);
        file_put_contents($cachefile_LG_EU, $substr_LG_EU);
    } else {
        // cache is still fresh
    }
} else {
    // no cache, create one
    $cache_LG_EU = file_get_contents('https://remotedomain.com/?get=price&product=20&currency=2');
    $substr_LG_EU = substr($cache_LG_EU,17,2);
    file_put_contents($cachefile_LG_EU, $substr_LG_EU);
}

?>

This manual way works when there are only two products (10 and 20) and two currencies (1 and 2) as I only need to do it 4 times to get all the pricing I need.

However, I am going to significantly expand the number of products to at least 12 products and 9 currencies so it is not realistic to do them manually.

I believe this can be done more efficiently with PHP foreach loop but I tried a few days and didn't manage to get it to work, maybe because of my weaker understanding of the concept.

I managed to split it up into this:-

<?php

$cacheDirectory = $_SERVER['DOCUMENT_ROOT'] . '/cache/';

$url = 'https://remotedomain.com/?get=price';
$productA = 10;
$productB = 20;
$USD = 1;
$EUR = 2;

// Small Plan - US
$cachefile_SM_US = $cacheDirectory . 'SM_US.cache';

if(file_exists($cachefile_SM_US)) {
    if(time() - filemtime($cachefile_SM_US) > 1600) {
        // too old , re-fetch
        $cache_SM_US = file_get_contents($url . '&product=' . $productA . '&currency=' . $USD);
        $substr_SM_US = substr($cache_SM_US,17,2);
        file_put_contents($cachefile_SM_US, $substr_SM_US);
        } else {
            // cache is still fresh
    }
} else {
    // no cache, create one
    $cache_SM_US = file_get_contents($url . '&product=' . $productA . '&currency=' . $USD);
    $substr_SM_US = substr($cache_SM_US,17,2);
    file_put_contents($cachefile_SM_US, $substr_SM_US);
}

// Large Plan - US
$cachefile_LG_US = $cacheDirectory . 'LG_US.cache';

if(file_exists($cachefile_LG_US)) {
    if(time() - filemtime($cachefile_LG_US) > 1600) {
        // too old , re-fetch
        $cache_LG_US = file_get_contents($url . '&product=' . $productB . '&currency=' . $USD);
        $substr_LG_US = substr($cache_LG_US,17,2);
        file_put_contents($cachefile_LG_US, $substr_LG_US);
    } else {
        // cache is still fresh
    }
} else {
    // no cache, create one
    $cache_LG_US = file_get_contents($url . '&product=' . $productB . '&currency=' . $USD);
    $substr_LG_US = substr($cache_LG_US,17,2);
    file_put_contents($cachefile_LG_US, $substr_LG_US);
}

// Small Plan - EU
$cachefile_SM_EU = $cacheDirectory . 'SM_EU.cache';

if(file_exists($cachefile_SM_EU)) {
    if(time() - filemtime($cachefile_SM_EU) > 1600) {
        // too old , re-fetch
        $cache_SM_EU = file_get_contents($url . '&product=' . $productA . '&currency=' . $EUR);
        $substr_SM_EU = substr($cache_SM_EU,17,2);
        file_put_contents($cachefile_SM_EU, $substr_SM_EU);
        } else {
            // cache is still fresh
    }
} else {
    // no cache, create one
    $cache_SM_EU = file_get_contents($url . '&product=' . $productA . '&currency=' . $EUR);
    $substr_SM_EU = substr($cache_SM_EU,17,2);
    file_put_contents($cachefile_SM_EU, $substr_SM_EU);
}

// Large Plan - EU
$cachefile_LG_EU = $cacheDirectory . 'LG_EU.cache';

if(file_exists($cachefile_LG_EU)) {
    if(time() - filemtime($cachefile_LG_EU) > 1600) {
        // too old , re-fetch
        $cache_LG_EU = file_get_contents($url . '&product=' . $productB . '&currency=' . $EUR);
        $substr_LG_EU = substr($cache_LG_EU,17,2);
        file_put_contents($cachefile_LG_EU, $substr_LG_EU);
    } else {
        // cache is still fresh
    }
} else {
    // no cache, create one
    $cache_LG_EU = file_get_contents($url . '&product=' . $productB . '&currency=' . $EUR);
    $substr_LG_EU = substr($cache_LG_EU,17,2);
    file_put_contents($cachefile_LG_EU, $substr_LG_EU);
}

?>

The challenge I have now is how to turn this into a foreach loop which will cycle through each product and each currencies.

Appreciate pointers in the right direction.

Thank you!

[Why is “Can someone help me?” not an actual question?](http://meta.stackoverflow.com/q/284236/1011527) — Jay Blanchard, Apr 15 '20 at 13:15
The main thing is to look at what common code there is and what parameters are needed to make the code work with the different possibilities. Then make a function with the common code and call this with the individual settings. So look at how `$cachefile_SM_US`, the product id and currency are the main things that vary in each pass. — Nigel Ren, Apr 15 '20 at 13:21
Thanks @NigelRen! I've updated the post and split them up a bit. Appreciate pointers in the right direction! — James, Apr 15 '20 at 13:30
See if you can make the whole `if(file_exists($cachefile_SM_US)) { ... }` into a function with the three parameters. — Nigel Ren, Apr 15 '20 at 13:35
Can you elaborate a little bit about loops you want to optimize, please? I am not sure what do you need to loop. Definitely you need to refactor and apply some code style rules for easier reading. As @NigelRen mention, extract repeating parts to functions. `function createCache()`, then extract check for `hasCache` and `cacheExpired`. — Vladimir Vukanac, Apr 15 '20 at 13:59
Check also: https://www.php-fig.org/psr/psr-16/ and https://symfony.com/doc/current/components/cache.html — Vladimir Vukanac, Apr 15 '20 at 14:39

Chemaclass · Accepted Answer · 2020-04-17T08:03:43.027

Absolutely. Take a look at this example :)

<?php declare(strict_types=1);

interface CacheNormalizer
{
    public function normalize(string $text): string;
}

interface PlanDomainToCache
{
    public function buildUrl(Plan $plan): string;
}

final class CachedRemoteSiteManager
{
    /** @var int Time To Live Cache */
    private $timeToLive;

    /** @var CacheNormalizer */
    private $cacheNormalizer;

    /** @var PlanDomainToCache */
    private $planDomainToCache;

    public function __construct(
        int $timeToLive,
        CacheNormalizer $cacheNormalizer,
        PlanDomainToCache $planDomainToCache
    ) {
        $this->timeToLive = $timeToLive;
        $this->cacheNormalizer = $cacheNormalizer;
        $this->planDomainToCache = $planDomainToCache;
    }

    public function updateIfNecessary(Plan $plan): void
    {
        if ($this->shouldCreateOrUpdateCache($plan)) {
            $this->createOrUpdateCache($plan);
        }
    }

    private function shouldCreateOrUpdateCache(Plan $plan): bool
    {
        return !file_exists($plan->cacheDirectory())
            || time() - filemtime($plan->cacheDirectory()) > $this->timeToLive;
    }

    private function createOrUpdateCache(Plan $plan): void
    {
        $urlToCache = $this->planDomainToCache->buildUrl($plan);
        $textToCache = file_get_contents($urlToCache);

        file_put_contents(
            $plan->cacheDirectory(),
            $this->cacheNormalizer->normalize($textToCache)
        );
    }
}

final class Plan
{
    /** @var string */
    private $cacheDirectory;

    /** @var int */
    private $product;

    /** @var int */
    private $currency;

    public function __construct(string $cacheDir, int $product, int $currency)
    {
        $this->cacheDirectory = $cacheDir;
        $this->product = $product;
        $this->currency = $currency;
    }

    public function cacheDirectory(): string
    {
        return $this->cacheDirectory;
    }

    public function product(): int
    {
        return $this->product;
    }

    public function currency(): int
    {
        return $this->currency;
    }
}

// Usage example:

$cacheDirectory = $_SERVER['DOCUMENT_ROOT'] . '/cache/';
$productA = 10;
$productB = 20;
$USD = 1;
$EUR = 2;

/** @var Plan[] */
$plansToCache = [
    new Plan($cacheDirectory . 'SM_US.cache', $productA, $USD),
    new Plan($cacheDirectory . 'LG_US.cache', $productB, $USD),
    new Plan($cacheDirectory . 'SM_EU.cache', $productA, $EUR),
    new Plan($cacheDirectory . 'LG_EU.cache', $productB, $EUR),
];

$cacheManager = new CachedRemoteSiteManager(
    $cacheTtl = 1600,
    new class implements CacheNormalizer {
        public function normalize(string $text): string
        {
            return substr($text, 17, 2);
        }
    },
    new class implements PlanDomainToCache {
        public function buildUrl(Plan $plan): string
        {
            return sprintf(
                'https://remotedomain.com/?get=price&product=%d&currency=%d',
                $plan->product(),
                $plan->currency()
            );
        }
    }
);

foreach ($plansToCache as $plan) {
    $cacheManager->updateIfNecessary($plan);
}

As you can see at the bottom, in the "usage example", I extracted all details (almost all of them) so we can easily define:

how we want to normalize the cached data (using the CacheNormalizer)
how we want to build the URL that we want to cache (using PlanDomainToCache).

UPDATED:

If you want to see how could you extract/decouple every detail from the ending code, inverting the dependencies upwards even for the "Persistence" layer: https://gist.github.com/Chemaclass/01d3f42685ff69f6897192202a32014d

That doesn't contain the different product urls but it shouldn't be hard to include those as params or something — JensV, Apr 15 '20 at 13:33
I updated my example. Now the product_id and currency_id are customizable :) — Chemaclass, Apr 15 '20 at 13:54
Nice OO solution :). In my opinion I'd name the `RemoteDomainCacheManager` something like `CachedRemotePriceFetcher` or something though. Makes it clearer on what it's capabilities are. — JensV, Apr 15 '20 at 13:57
And I guess a typo on the phpdoc for `$plansToCache` I think it should be `@var Plan[]` since it's a collection of objects — JensV, Apr 15 '20 at 13:59
You are right @JensV. I updated that part. And also I extract the "domain URL builder" to be callable. Doing this, we can define the way we want to build the URL at the same instance we are instantiating the object. This way the class `CachedRemoteSiteManager` is completely decouple for any internal detail about the web site that is caching ;) — Chemaclass, Apr 15 '20 at 14:13
While you're at it, why not extract the data parsing functionality (substr(..) stuff). Oh, why not make the class abstract and write an implementation for that :D — JensV, Apr 15 '20 at 14:16
Absolutely. I created an interface `CacheNormalizer` which decouple the responsibility of the normalization of the text that is cached ;) — Chemaclass, Apr 15 '20 at 14:23
It works perfectly! I found out about `preg_replace` so I changed `substr($text, 17, 2);` to `preg_replace("/[^0-9\.,]/", "", substr($text, 17));` which suits my use case better. (of stripping off everything except numbers and decimals. — James, Apr 16 '20 at 05:38

score 0 · Answer 2 · answered Apr 15 '20 at 13:53

If I interpreted the code correctly you want to look up both products for both currencies. This can be done with a nested foreach loop after you define your products and currencies.

$cacheDirectory = $_SERVER['DOCUMENT_ROOT'] . '/cache/';
$url = 'https://remotedomain.com/?get=price';

const MAX_CACHE_TIME = 1600;

// Optional
$output = [];

$productList = [
    [
        'id'   => 10,
        'name' => 'SM',
    ],
    [
        'id'   => 20,
        'name' => 'LG',
    ]
];

$currencies = [
    'US' => 1,
    'EU' => 2,
];

foreach ($productList as $product) {
    foreach ($currencies as $currencyName => $currencyId) {
        $cacheFile = $cacheDirectory . $product['name'] . '_' . $currencyName . '.cache';

        if (!file_exists($cacheFile) || filemtime($cacheFile) > MAX_CACHE_TIME) {
            // No cache or too old
            $data = file_get_contents($url . '&product=' . $product['id'] . '&currency=' . $currencyId);
            $relevantData = substr($data, 17, 2);
            file_put_contents($cacheFile, $relevantData);
            // Optional, put the data in an array
            $output[$product['id']][$currencyId] = $relevantData;
        } else {
            $output[$product['id']][$currencyId] = file_get_contents($cacheFile);
        }

    }
}

// Read output with $output[10]['US'] for example

Is there a better way cycle through these PHP codes, maybe with foreach loop?

2 Answers2