PHP program to validate phone numbers from a file

Question

As part of job interview, I was asked to write a cli script that takes a list of phone numbers to validate from a file and as an option in the command line. It will then produce a CSV file with three columns “phone number”, “carrier”, “status”.

The status column should indicate if the phone number is valid or not valid. The carrier field should contain the carrier name if the phone number is valid.

I am using libphonenumber-for-php and https://github.com/splitbrain/php-cli

They asked me to show knowledge in PHP programming using the latest techniques and open source technologies available. Can anybody see obvious improvements where I am not doing this?

So far I have come up with the following which works:

require 'vendor/autoload.php';

use splitbrain\phpcli\CLI;
use splitbrain\phpcli\Options;
use libphonenumber\PhoneNumberUtil;
use libphonenumber\PhoneNumberToCarrierMapper;
use libphonenumber\PhoneNumberType;


class mobileValidator extends CLI
{
// override the default log level
protected $logdefault = 'info';

// register options and arguments
protected function setup(Options $options)
{
    $options->setHelp('This script takes a list of phone numbers from a file (or as an option) and validates them as UK mobile numbers.');
    $options->registerOption('file', 'The file containing a list of phone numbers to validate', 'f', 'filename');
    $options->registerOption('numbers', 'List of numbers passed in as an option (numbers should separated by a comma).', 'n', 'numbers');
}


/**
 * The main Program
 * Arguments and options have been parsed when this is run
 * @param Options $options
 * @return void
 */

protected function main(Options $options)
{
    if (!$options->getOpt('file') && !$options->getOpt('numbers')) {
        $this->error('No files or numbers have been supplied - Cannot Continue.');
        exit();
    }

    $this->notice('Validation Process Started');

    //Get options
    $opNum = $options->getOpt('numbers');
    $file = $options->getOpt('file');

    $numbersFromOp = array();
    $numbersFromFile = array();

    //Set country code uk
    $countryCode = 44;

    //Set regions codes to Uk, Guernsey, Jersey and Isle of Man
    $regionCodes = array('GB', 'GG', 'JE', 'IM');

    //get validator dependencies
    $phoneUtil = PhoneNumberUtil::getInstance();
    $carrierMapper = PhoneNumberToCarrierMapper::getInstance();
    $phoneNumberType = PhoneNumberType::MOBILE;

    //Get numbers from passed in option
    if ($opNum) {
        $numbersFromOp = explode(',', $opNum);
    }

    //Check if files is set and actually exists
    if ($file) {
        if(file_exists($file)) {
            $numbersFromFile = file($file);
        }
        else {
            $this->warning("File not found ".$file);
            if(!$opNum){
                $this->critical('File not found and no numbers have been supplied - Cannot Continue.');
                exit();
            }
        }
    }

    //marge all number arrays to be validated
    $phoneNumbers = array_merge($numbersFromFile, $numbersFromOp);

    //Validate the numbers
    $validator = new validator($phoneNumbers, $phoneUtil, $carrierMapper, $phoneNumberType, $countryCode, $regionCodes);
    $results = $validator->validateNumbers();

    //write numbers to the csv file
    $this->generateCSV($results);

    $this->success('Validation process completed successfully');
}

/**
 * generateCSV
 * Simple function to write a csv file
 * @param array $results
 * @return void
 */

protected function generateCSV(array $results){
    // Open a file to write to
    $fp = fopen('output/phone_numbers_'. date('d-M-Y-H-i').'.csv' , 'wb');

    $headerRow = array ('Phone Number', 'Carrier', 'Status');

    $i = 0;
    foreach( $results as $fields ){
        if( $i === 0 ){
            fputcsv($fp, $headerRow ); // First write the headers
        }
        fputcsv($fp, $fields); // Then write the fields
        $i++;
    }
    fclose($fp);
}


/**
 * Override log function
 * Added error_log
 * @param string $level
 * @param string $message
 * @param array $context
 */
public function log($level, $message, array $context = array())
{
    // is this log level wanted?
    if (!isset($this->loglevel[$level])) return;

    /** @var string $prefix */
    /** @var string $color */
    /** @var resource $channel */
    list($prefix, $color, $channel) = $this->loglevel[$level];
    if (!$this->colors->isEnabled()) $prefix = '';

    $message = $this->interpolate($message, $context);
    error_log($message);
    $this->colors->ptln($prefix . $message, $color, $channel);
}

}
// execute it
$cli = new mobileValidator();
$cli->run();

The validator class:

use libphonenumber\PhoneNumberUtil;
use libphonenumber\PhoneNumberToCarrierMapper;
use libphonenumber\NumberParseException;

class validator
{
private $phoneNumbers;
private $phoneUtil;
private $carrierMapper;
private $phoneNumberType;
private $countryCode;
private $regionCodes;
private const VALID = 'Valid';
private const INVALID = 'Invalid';

function __construct( array $phoneNumbers, PhoneNumberUtil $phoneUtil, PhoneNumberToCarrierMapper $carrierMapper, $phoneNumberType, $countryCode, $regionCodes)
{
    $this->phoneNumbers = $phoneNumbers;
    $this->phoneUtil = $phoneUtil;
    $this->carrierMapper = $carrierMapper;
    $this->phoneNumberType = $phoneNumberType;
    $this->countryCode = $countryCode;
    $this->regionCodes = $regionCodes;
}

/**
 * Returns validated results
 * Loops through the phone numbers and validates as uk mobile numbers (including the channel islands).
 * Results returned as an array( number, carrier, valid)
 * @return array $results
 */

public function validateNumbers(){
    $results = array();
    //loop through supplied phone numbers
    foreach ($this->phoneNumbers as $phoneNumber) {
        $number = trim($phoneNumber);
        try {
            $phoneNumberObject = $this->phoneUtil->parse($number, 'GB');
        }
        catch (NumberParseException $e) {
            $results[] = array($number,'', self::INVALID);
            error_log($e);
            continue;
        }
        //get country code and region code
        $countryCode = $phoneNumberObject->getCountryCode();
        $regionCode = $this->phoneUtil->getRegionCodeForNumber($phoneNumberObject);
        $valid = false;

        //Number is considered valid if the country code matches supplied country code
        // and the region code matches one of the supplied region codes

        if($countryCode === $this->countryCode && in_array($regionCode, $this->regionCodes)){
            $valid = $this->phoneUtil->isValidNumber($phoneNumberObject);
        }

        $type = $this->phoneUtil->getNumberType($phoneNumberObject);
        $carrier = '';
        $validMobile = self::INVALID;
        //if the number is valid and the type is mobile attempt to get the carrier
        if ($valid && $type === $this->phoneNumberType) {
            $carrier = $this->carrierMapper->getNameForNumber($phoneNumberObject, 'en');
            if ($carrier === '') {
                $carrier = 'unknown';
            }
            $validMobile = self::VALID;
        }
        //add to the results array
        $results[] = array($number,$carrier, $validMobile);
    }

    return $results;
}
}

Answer 1

I looked through your code, but to be honest, I don't see much to review. Believe it or not; That's actually a good thing. I think.

You're basically using two libraries in two classes. But you haven't paid much attention to structuring your own code.

You could, for instance, have take a bit more effort in the validator class, splitting it up in methods for validating the various aspects of a phone number. Now it is all inside one big validateNumbers() method. It works, but as a validator it is far less useful than it could be. I almost start to wonder why this is a class in the first place? The same is true for mobileValidator, almost everything is cramped inside the main() method.

In general a method should perform a single task in the context of its class.

That means it should concentrate on the details of that single task. In your validator class validateNumbers() deals with all the aspects of the phone numbers. Were you to split it up a bit more, you could end up with methods like this:

__construct($phoneUtil, $carrierMapper, $phoneNumberType)
restrictCountryAndRegionCodes($countryCode, $regionCodes)
validateNumbers($phoneNumbers)
validateNumber($phoneNo)
cleanNumber($phoneNo)
isMobileNumber($phoneNo)
getCarrier($phoneNo)

These are, of course, just examples. Notice how I detached the phone numbers completely from the class constructor. That way you can feed it one, or many, numbers, several times.

The point is: Your validator class is very specifically made for one job and only for that job. It cannot do anything else. It cannot easily be modified, expanded, or reused. As a class it is not flexible. By breaking up the code, into its functional parts, it becomes more flexible, and easier to read, refactor, debug and test.

This is basically the Single Responsibility principle, of the SOLID principles, applied to methods. Although these princicles are rather abstract, they do have real benefits when applied consistently.

If I were to write your code, I would first think about the main entities that it deals with; The files and the phone number.

This almost automatically suggests to me that there should be a class called 'PhoneNumber'. This class then gets various methods like getNumber() and setNumber(), and, of course, it could have a method called isValidMobileNumber(). It would deal with only one phone number.

The next class I can see is the file with the phone numbers. A class name for that could be simply: DataFile, and it would deal with the details of that file.

The last class is the CSV file.

These three classes are used by my extended CLI class like this:

class PhoneNoValidatorCLI extends CLI
{
    private $phoneData = [];

    private function __construct()
    {
        $this->phoneNo = new PhoneNumber(.....);
        $this->addPhoneData("Phone Number", "Carrier", "Status"); // header
        parent::__construct();
    }

    public function addPhoneData($number, $carrier, $status)
    {
        $this->phoneData[] = [$number, $carrier, $status];
    }

    public function writeData2CSV($filename)
    {
        $csv = new CSV($filename);
        $csv->writeData($this->phoneData);
    }

    protected function main(Options $options)
    {
        $file = new DataFile(.....);
        while ($number = $file->nextLine()) {
            $this->phoneNo->setNumber($number);
            $this->addPhoneData($this->phoneNo->getNumber(),
                                $this->phoneNo->getCarrier(),
                                $this->phoneNo->getStatus());
        }
        $file->close();
        $this->writeData2CSV("phone_numbers_" . date('d-M-Y-H-i'));
    }
}

This is just a very crude example, lacking many details, but I hope you get the idea. In OOP you first try to find the logical objects you're dealing with, and then write your code around them. You had two libraries and wrote your code around those, without thinking about the possible objects in your own task.

One last thing. You extend the CLI class. It is considered "bad practice" to extend classes you do not own. Your extension could break their class. However, I also see that the library itself tells you to use it like that. So forget about this.