2
\$\begingroup\$

Having the following code in PHP 5.2, where:

  1. $row: Is an array that contains some things, but the ones that are interesting for the post are country_from and country_to, which are country names.
  2. $pBCountries: Array containing strings with country names. Here's the content of the array: http://pastebin.com/f241JeqV

The function is very easy. It checks if the countries (country_from and country_to) are included on the array $pBCountries. If the countries are not found, it appends the "not found country name" to the array $notFoundArray, which is returned at the end of the function.

function countryChecker($row, $pBCountries)
{
    $notFoundArray = array();
    $c_fromFound = false;
    $c_toFound = false;
    foreach ($pBCountries as $country) {
        if (in_array($row["country_from"], $country)) {
            $c_fromFound = true;
        } elseif (in_array($row["country_to"], $country)) {
            $c_toFound = true;
        }
    }
    if ($c_fromFound == false) {
        array_push($notFoundArray, $row["country_from"]);
    }
    if ($c_toFound == false) {
        array_push($notFoundArray, $row["country_to"]);
    }
    return $notFoundArray;
}

The function works perfectly, but I'm pretty sure I can make it shorter and prettier.

PS: I can't update the PHP version as it's not my own machine.

\$\endgroup\$
6
  • 1
    \$\begingroup\$ Are you sure this code actually works ? you said $pBCountries is array of strings, so $country inside the foreach loop is a string while the in_array function accepts an array as second argument ! \$\endgroup\$
    – webNeat
    Commented Jun 16, 2016 at 9:42
  • 1
    \$\begingroup\$ Could you clarify what the array $pBCountries looks like? Cause you say that it is an array of countries (which I assume would be strings), yet you loop through this array with foreach ($pBCountries as $country) and then do in_array($row['country_from'], $country), which doesn't make sense because then $country would just be a string. If $pBCountries truly was an array of countries, why not to in_array($row['country_from'], $pBCountries? \$\endgroup\$ Commented Jun 16, 2016 at 9:44
  • 1
    \$\begingroup\$ Then I am not sure your code even works. The second parameter to in_array() should be an array, not a string. Your code most is probably throwing a WARNING when passing a string as the second argument and in_array() will then return NULL. \$\endgroup\$ Commented Jun 16, 2016 at 9:47
  • \$\begingroup\$ Yes, my fault. $pbCountries is an array of arrays that contains the country name. Here's the content of the file $pbCountries: pastebin.com/f241JeqV \$\endgroup\$
    – Avión
    Commented Jun 16, 2016 at 9:52
  • \$\begingroup\$ You should upgrade your PHP version to PHP 5.6, since even 5.4 has been EOL (End Of Life) for almost a year and PHP 5.2 for over 5 years. php.net/eol.php \$\endgroup\$ Commented Jun 16, 2016 at 10:54

2 Answers 2

2
\$\begingroup\$

Representing a country (OOP)

So it seems that your array of countries ($pBCountries) has the following structure:

[
    [
        "id" => "0",
        "name" => "Netherlands"
        "country_code" => "NL"
    ],
    [
        "id" => "1",
        "name" => "United Kingdom"
        "country_code" => "UK"
    ],
]

Now that we have that figured out, lets take a look at how we can improve this. Generally we do not like to work with arrays like this, as it is extremely unclear how they are structured to outside users (as you have seen from the comments to your question).

What we do like working with are objects. Objects are a much better way of representing stuff than arrays are. We can create a simple object that represents a country like so:

class Country {
    private $id;
    private $name;
    private $code;

    /**
     * @param string|int $id
     * @param string $name
     * @param string $code
     */
    public function __construct($id, $name, $code) {
        $this->id = (string) $id;
        $this->name = (string) $name;
        $this->code = (string) $code;
    }

    /**
     * @return string
     */
    public function getId() {
        return $this->id;
    }

    /**
     * @return string
     */
    public function getName() {
        return $this->name;
    }

    /**
     * @return string
     */
    public function getCode() {
        return $this->code;
    }
}

Now we have a simple, immutable country object. The private fields cannot be changed from the outside.

Creating the Country objects

Now we still need a function which creates a bunch of these Country objects from your array. For this we are going to create a createMultipleFromArray() function on the Country object:

class Country {
    /* private fields */

    /**
     * @param array countries
     * @return Country[]
     */
    public static function createMultipleFromArray(array $countries) {
        $countries = array();

        foreach ($countries as $country) {
            array_push($countries, self::createFromArray($country));
        }

        return $countries;
    }

    /**
     * @param array $country
     * @return Country
     */
    public static function createFromArray(array $country) {
        return new Country($country["id"], $country["name"], $country["country_code"]);
    }

    /* constructor and getters */
}

I have omitted the private fields, constructor and getters from this snippet and replaced them with a comment, so that you can understand where I would place the methods but without me having to copy them over to this snippet as well.

You have probably noticed that I have annotated most functions with some @return and @param annotations. These annotations are extremely useful as they tell you what type is expected for certain arguments and what type will be returned. If you are using an IDE like PhpStorm this is even more helpful, as PhpStorm will know which methods are available on a Country object and can auto-complete them for you. This is a lot less error prone than fiddling with array indices yourself.

Now we can create an array of Country objects by simple doing:

// p.s. $pBCountries is a pretty bad name for an array of countries
$countries = Country::createMultipleFromArray($pBCountries);

Array Intersection

Now that we have cleaned up that mess of an array, lets get back to the actual function. First of all, your checkCountry() function would be better off taking two explicit $country_from and $country_to parameters, instead of relying on an array to be passed which may or may not contain these indices. This makes the function more clean.

The functionality of your function can actually be extracted to a much more general concept called array intersection. Array intersection is a function that takes two arrays and returns a new array containing only the elements that are in both arrays.

To achieve this, we first need a function which can determine if two Country objects are the same. We do this by creating an equals() method on the Country object:

class Country {
    /* private fields and constructor */

    /**
     * @param Country $other_country
     * @return bool
     */
    public function equals(Country $other_country) {
        return $this->id == $other_country->getId() &&
            $this->name == $other_country->getName() &&
            $this->code == $other_country->getCode();
    }

    /* Getter functions */
}

Now we can create a separate class that can perform an intersection of two arrays of Country objects:

class CountryFilter {
    /**
     * @param Country[] $countries_to_filter
     * @param Country[] $countries
     */
    public static function intersect(array $countries_to_filter, array $countries) {
        $intersected_countries = array();

        foreach ($countries as $country) {
            if (self::contains($countries_to_filter, $country)) {
                array_push($intersected_countries, $country);
            }
        }

        return $intersected_countries;
    }

    /*
     * @param Country[] $country_array
     * @param Country $country
     */
    public static function contains(array $country_array, Country $country) {
        foreach ($country_array as $country_from_array) {
            if ($country->equals($country_from_array)) {
                return true;
            }
        }

        return false;
    }
}

The contains() method I think doesn't really belong on the CountryFilter class, but actually on a class like CountryList, which is a class that holds an array of countries and provides the contains() functionality. But I figured that would probably take it too far, so I kept it at the CountryFilter class.

Assuming that you have changed $row['country_from'] and $row['country_to'] to be actual Country objects and $pBCountries to be an array of Country objects, we can now perform this function call to achieve the same results as the countryChecker() method:

$intersection = CountryFilter::intersect($pBCountries, [$row['country_from'], $row['country_to']]);

This refactor resulted in much more general classes and functionality with a lot of reusability. The intersect() method can be used for any two arrays of countries and is therefore very versatile. Each of these small pieces of code are very readable and easy to understand, which will in turn make other code that utilizes these two classes more easy to understand as well.

Thus code readability and reusability have greatly been improved.

\$\endgroup\$
2
  • \$\begingroup\$ Thanks a lot for the explanation. I know I can reorder the code much better using classes and stuff, but I just need a refactor of my simple function without going any further. I was just trying to avoid to use so many ifs in my code. Anyways, thanks for the huge text. \$\endgroup\$
    – Avión
    Commented Jun 16, 2016 at 11:12
  • 1
    \$\begingroup\$ I appreciate the effort and upvoted, but isn't this a bit over-engineered? \$\endgroup\$ Commented Jun 16, 2016 at 11:26
1
\$\begingroup\$

Remarks

  • When you do in_array($row["country_from"], $country) you are comparing a country name with all fields on the $country array, which makes no sense. Doing $row["country_from"] == $country['name'] makes more sens.

  • Use simple and significant names for function arguments: $countries is clearer then $pBCountries.

  • Add comments to your code.

Code for PHP >= 5.3

Here is a correct function using a functional approch instead of loops.

/**
 * Checks the country_from and country_to of a row and returns an array of not found countries.
 * @param  {array} $row
 * @param  {array} $countries
 * @return {array}
 */
function countryChecker($row, $countries)
{
    $countryNames = array_map(function($country) {
        return $country['name'];
    }, $countries);

    $countriesToCheck = array($row['country_from'], $row['country_to']);

    $notFoundCountries = array_filter($countriesToCheck, function($country) use ($countryNames) {
        return in_array($country, $countryNames)
    });

    return $notFoundCountries;
}

Code for PHP 5.2

In your case as you use PHP 5.2, This is the code which will work for you

/**
 * Get the name of a country
 * @param  {array} $country
 * @return {string}
 */
function getName($country) {
    return $country['name'];
}

/**
 * Checks the country_from and country_to of a row and returns an array of not found countries.
 * @param  {array} $row
 * @param  {array} $countries
 * @return {array}
 */
function countryChecker($row, $countries)
{
    $countryNames = array_map('getName', $countries);

    $notFoundCountries = array();
    if(!in_array($row['country_from'], $countryNames))
        $notFoundCountries[] = $row['country_from'];
    if(!in_array($row['country_to'], $countryNames))
        $notFoundCountries[] = $row['country_to'];

    return $notFoundCountries;
}

Hope this helps :)

\$\endgroup\$
7
  • \$\begingroup\$ You must be using an old version of PHP, I updated the code to use array(...) instead of [...] \$\endgroup\$
    – webNeat
    Commented Jun 16, 2016 at 10:32
  • \$\begingroup\$ Thanks and yes, I'm using an old version of PHP (5.2) and I can't update it. Now I'm getting Parse error: syntax error, unexpected T_FUNCTION, expecting ')'on this line $countryNames = array_map(function($country) { \$\endgroup\$
    – Avión
    Commented Jun 16, 2016 at 10:33
  • \$\begingroup\$ You should mention your PHP version in the question, as PHP 5.2 doesn't support closures (anonymous functions). \$\endgroup\$
    – webNeat
    Commented Jun 16, 2016 at 10:48
  • \$\begingroup\$ I just added it. I'm quite new in PHP and I didn't know they were so many changes. So, any other further help? \$\endgroup\$
    – Avión
    Commented Jun 16, 2016 at 10:50
  • \$\begingroup\$ I edited the answer to add code for PHP 5.2 \$\endgroup\$
    – webNeat
    Commented Jun 16, 2016 at 11:00

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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