Skip to main content
Code Review

Return to Answer

replaced http://se2.php.net with https://www.php.net
Source Link

First of all, regarding your use of array_push(), I quote the documentation 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."

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."

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."

Added link to php docs about the array_push function. Improved the grammar. Mentioned use of databases.
Source Link
Letharion
  • 618
  • 1
  • 4
  • 14

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 could mainly beis just a personal preference, but I like this much better:

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. :)

First of all, your code is a bit to repetitive, this could mainly be a preference, but I like this much better:

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

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:

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. :)

Source Link
Letharion
  • 618
  • 1
  • 4
  • 14

First of all, your code is a bit to repetitive, this could mainly be a 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 closures, factories, dependency injection, and all kinds of other awesomeness. I disgress however, and will leave you to discover those subjects later. :)

lang-php

AltStyle によって変換されたページ (->オリジナル) /