Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Background:

I'm creating a website that parses in currency rates of the three major cities listed below in the $cities array.

I'll explain my code as I go along.

<?php

// Feed URL's //
$theMoneyConverter = 'http://themoneyconverter.com/rss-feed/';

// Define arrays // 
$cities = array(
    'London',
    'New York',
    'Paris'
);

$currencyCode = array(
    'GBP',
    'USD',
    'EUR'
);

// Arguments parsed onto theMoneyConverter URL to set position of the <item> tags declared in the XML document
$currencySource = array(
    $theMoneyConverter . $currencyCode[0] . '/rss.xml?x=15',
    $theMoneyConverter . $currencyCode[1] . '/rss.xml?x=16',
    $theMoneyConverter . $currencyCode[2] . '/rss.xml?x=56'
);
?>

Above is my configuration script that a developer will use if they wish to drop a city or add a new one. Notice in my $currencySource array I add arguments to the end of each URL. For example, at index 0 in the array I add ?x=15. This argument corresponds to the <item> element from the original source feed from themoneyconvert.com. In the case of ?x=15, this would be the 15th <item> element from the XML document.

The XML document to confirm this. The 15th element contains the following element just to clarify:

<item>
<title>EUR/GBP</title>

The following is stored in a separate script called getCurrency.php:

function get_currency_rate($currencySource) {

      try {
        $xml = new SimpleXmlElement(file_get_contents($currencySource));
    } catch (Exception $e) {
        echo 'Caught exception: An error has occured. Unable to get file contents';
    }

    $vars = parse_url($currencySource, PHP_URL_QUERY);
    parse_str($vars);
    switch ($x) {
        case 15:
            get_rate($xml, 15); //EUR 15
            get_rate($xml, 56); //USD 56
            break;
        case 16:
            get_rate($xml, 16); //GBP 16
            get_rate($xml, 15); //EUR 15
            break;
        case 56: default :
            get_rate($xml, 15); // EUR 15
            get_rate($xml, 56); // USD 56
            break;
    }
}

// Get and return currency rate
function get_rate(SimpleXMLElement $xml, $x) {

    $currency['rate'] = $xml->channel->item[$x]->description;

    echo $currency['rate'], '<br />';
}

I call the get_currency_rate() function from my user interface like such:

echo get_currency_rate($currencySource[$index])

After getting the file contents I extract the argument that I added to the URL from the configuration file listed above and store it as a variable $x. The XML feed and variable $x are parsed into the get_rate function().

The whole process doesn't seem very maintainable and particular readable. Adding a new city into the equation would become a nightmare for another developer. Can anyone think of any other alternatives?

share|improve this question
    
I just ran your url with ?x=15 and 16 - it does not grab the 15th element. it returns the entire XML. –  azngunit81 Apr 10 at 4:05

1 Answer 1

I see some inconsistencies in your getCurrency.php code. There are many variables that are never declared within the functions, so I don't think that's going to work as expected.

I think the best approach would be to make a parser which looks for whatever currency code you need, regardless of the item order because that might change in the future. I mean, the most performance-expensive part of the job is to parse the whole XML into a SimpleXML structure which you're doing anyway, so looping through the items seaching for the right one wouldn't be much worse and your code will be more straight-forward and escalable.

On the other hand, you might also want to have some cache of the parsed XML to avoid re-parsing the same file on each request. But that depends on the variability of the XML feed. Maybe you can check with The Money Converter how often the feed is updated.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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