Ensuring valid origin and destination countries

Question

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.

Answer 1

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.

Answer 2

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