2
\$\begingroup\$

I've created a website that displays weather and currency rates for London and New York. The following is an abstract from my cityConfig.php script. Is this a good approach for adding variables from each city into an array?

// Define arrays // 
$cities = array();
$currencyRateHeadings = array(); 
$currencySource = array(); 
$weatherSource = array(); 

// Feed URL's //
$yahooWeather = 'http://weather.yahooapis.com/forecastrss?p=';
$theMoneyConverter = 'http://themoneyconverter.com/rss-feed/';

// City 1 //
$city = 'London';
$currencyRateHeading = '1 British Pound Sterling';
$cityWeatherSourceCode = 'UKXX0085';
$cityWeatherSource = $yahooWeather . $cityWeatherSourceCode . '&u=c';
$currencyRateFeedCode = 'GBP';
$cityCurrencySource = $theMoneyConverter . $currencyRateFeedCode . '/rss.xml';

array_push($currencySource, $cityCurrencySource);
array_push($weatherSource, $cityWeatherSource);
array_push($cities, $city);
array_push($currencyRateHeadings, $currencyRateHeading);

// City 2 //
$city = 'New York';
$currencyRateHeading = '1 US Dollar';
$cityWeatherSourceCode = 'USNY0996';
$cityWeatherSource = $yahooWeather . $cityWeatherSourceCode . '&u=c';
$currencyRateFeedCode = 'USD';
$cityCurrencySource = $theMoneyConverter . $currencyRateFeedCode . '/rss.xml';

array_push($cities, $city);
array_push($currencyRateHeadings, $currencyRateHeading);
array_push($currencySource, $cityCurrencySource);
array_push($weatherSource, $cityWeatherSource);

Another alternative I considered was to create extra variables by adding either a 1 or a 2 to the end of the variable name (depending whether they are related to city1 or city2) and push everything onto the array at the end.

// Define arrays // 
$cities = array();
$currencyRateHeadings = array(); 
$currencySource = array(); 
$weatherSource = array(); 

// Feed URL's //
$yahooWeather = 'http://weather.yahooapis.com/forecastrss?p=';
$theMoneyConverter = 'http://themoneyconverter.com/rss-feed/';

// City 1 //
$city1 = 'London';
$currencyRateHeading1 = '1 British Pound Sterling';
$cityWeatherSourceCode1 = 'UKXX0085';
$cityWeatherSource1 = $yahooWeather . $cityWeatherSourceCode . '&u=c';
$currencyRateFeedCode1 = 'GBP';
$cityCurrencySource1 = $theMoneyConverter . $currencyRateFeedCode . '/rss.xml';

// City 2 //
$city2 = 'New York';
$currencyRateHeading2 = '1 US Dollar';
$cityWeatherSourceCode2 = 'USNY0996';
$cityWeatherSource2 = $yahooWeather . $cityWeatherSourceCode . '&u=c';
$currencyRateFeedCode2 = 'USD';
$cityCurrencySource2 = $theMoneyConverter . $currencyRateFeedCode . '/rss.xml';

array_push($cities, $city1, $city2);
array_push($currencyRateHeadings, $currencyRateHeading1, $currencyRateHeading2);
array_push($currencySource, $cityCurrencySource1, $cityCurrencySource2);
array_push($weatherSource, $cityWeatherSource1, $cityWeatherSource2);

Can anyone provide an example of a better way of doing this?

The arrays and variables are called later in my threeColumnContainer.php script after passing through the switch statement to determine which page they have come from. I'm happy with what's happening below at the moment. It's just setting the array's that I'd like feedback for?

<li class="leftCol">
    <p>
        <?php
        switch ($currentPage) {
            case 'index.php':
                echo displayCityContent($weatherSource[0], $columnSubheading);
                break;
            case 'currencyRate.php':
                echo displayCurrencyRateContent($currencySource[0]);
                break;
            default:
                content_unavailable();
                break;
        }
        ?>
    </p>
</li>
<li class="rightCol">
    <p>
        <?php
        switch ($currentPage) {
            case 'index.php':
                echo displayCityContent($weatherSource[1], $columnSubheading);
                break;
            case 'currencyRate.php':
                echo displayCurrencyRateContent($currencySource[1]);
                break;
            default:
                content_unavailable();
                break;
        }
        ?>                        
    </p>
</li>
\$\endgroup\$
  • 1
    \$\begingroup\$ Another alternative I considered was to create extra variables by adding either a 1 or a 2 to the end of the variable name - No, do not do this; it's naive (no offense). Can you store your data in a database (of some kind, not necessarily MySQL but that would work)? If it were an relational database like MySQL, you could simply query for the cities. Alternatively, you could create an array "normally" ($cities = array(array('key'=>'first value'),array('key'=>'another value'));) in an included file that setup your array's structure. \$\endgroup\$ – Jared Farrish Feb 4 '12 at 12:03
  • 1
    \$\begingroup\$ Anywhere where you're repeating code, though, should be optimized so you don't do it (practicality sometimes dictates that you will copy some code). The general rule is Don't Repeat Yourself (DRY). \$\endgroup\$ – Jared Farrish Feb 4 '12 at 12:07
1
\$\begingroup\$

First of all, regarding your use of array_push(), I quote the documentation: "Note: If you use array_push() to add one element to the array it's better to use $array[] = because in that way there is no overhead of calling a function."

With that out of the way, on to the code!

With or without array_push, I find that the code is a bit to repetitive, this is just a personal preference, but I like this much better:

$yahooWeather = 'http://weather.yahooapis.com/forecastrss?p=';
$theMoneyConverter = 'http://themoneyconverter.com/rss-feed/';

$currencySource = array(
  0 => $theMoneyConverter . 'GBP/rss.xml',
  1 => $theMoneyConverter . 'USD/rss.xml',
);

$cities = array(
  0 => 'London',
  1 => 'New York',
);

$currencyRateHeadings = array(
  0 => '1 British Pound Sterling',
  1 => '1 US Dollar',
); 

$weatherSource = array(
  0 => $yahooWeather . 'UKXX0085&u=c',
  1 => $yahooWeather . 'USNY0996&u=c',
);

This however still repeats a few things, and will become worse if you add additional cities, so at the very least, I would then change it into this:

$yahooWeather = 'http://weather.yahooapis.com/forecastrss?p=';
$theMoneyConverter = 'http://themoneyconverter.com/rss-feed/';

$cities = array(
  0 => 'London',
  1 => 'New York',
);

$currencyRateHeadings = array(
  0 => '1 British Pound Sterling',
  1 => '1 US Dollar',
); 

$weather_codes = array('UKXX0085', 'USNY0996');
$currency_codes = array('GBP', 'USD');
$weatherSource = array();
$currenySource = array();
foreach ($weather_codes as $weather_code) {
  $weatherSource[] = $yahooWeather . $weather_code . '&u=c';
}
foreach ($currency_codes as $currency_code) {
  $currencySource[] = $theMoneyConverter . $currency_code . '/rss.xml';
}

This second example, makes it very easy to add new elements, especially the weather and currency codes, as all you need to do is attach a new element at the end of the two arrays that hold them.

So far the structure of your arrays have been kept the same, to make sure that they still work with your current scripts. If I were you though, I would restructure it.

If you really only need two cities, just do:

$city_data = array(
  'London' => array('1 British Pound Sterling', $yahooWeather . 'UKXX0085' . '&u=c', $theMoneyConverter . 'GBP' . '/rss.xml'),
  'New York' => array('1 US Dollar',  $yahooWeather . 'USNY0996' . '&u=c', $theMoneyConverter . 'USD' . '/rss.xml'),
);

If you want it to be extensible to more cities on the other hand:

$city_data = array(
  'London' => array('1 British Pound Sterling', 'UKXX0085', 'GBP'),
  'New York' => array('1 US Dollar', 'USNY0996', 'USD'),
);

$cities = array();
foreach($city_names as $city) {
  $cities[$city] = city($city[0], $city[1], $city[2]);
}

function city($currency_heading = '', $weather_code, $currency_code) {
  $yahooWeather = 'http://weather.yahooapis.com/forecastrss?p=';
  $theMoneyConverter = 'http://themoneyconverter.com/rss-feed/';

  return array(
    'currency_heading' => $currency_heading,
    'weather' => $yahooWeather . $weather_code . '&u=c';
    'currency' => $theMoneyConverter . $currency_code . '/rss.xml';
  );
}

From there on, we could start doing really cool stuff with databases, closures, factories, dependency injection, and all kinds of other awesomeness. I disgress however, and will leave you to discover those subjects later. :)

\$\endgroup\$

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.