How to improve this script

I have been 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

They want 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:

      <?php
  /**
 * Created by PhpStorm.
 * User: Andy
 * Date: 20/05/2019
 * Time: 23:38
*/
require 'vendor/autoload.php';
require_once "class/validator.php";

$options = getopt('f:l:n:');

if(!isset($options['f']) && !isset($options['n'])){
echo "No files or number have been specified \n";
exit();
}

if(isset($options['f'])){
if(!is_array($options['f'])){
    $files[] = $options['f'];
}
else {
    $files = $options['f'];
}

foreach ($files as $file){
    if(!file_exists($file)) {
        echo "File not found ".$file. "\n";
        exit();
    }
}
}

if(isset($options['n'])){
$numbers = explode(',', $options['n']);
}

$logResults = false;

if(isset($options['l']) && $options['l'] == 'true')
$logResults = true;
//Uk, Guernsey, Jersey and Isle of Man
$regionCodes = array('GB', 'GG', 'JE', 'IM');

echo "Validation Process Starting \r\n";

$validator = new validator($files, $numbers, $regionCodes, $logResults);
$results = $validator->getResults();

// 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;

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

fclose($fp);

The class:

        <?php
/**
 * Used to validate uk mobile numbers including the channel islands
 * Created by PhpStorm.
 * User: Andy
 */
class validator
{
    private $files;
    private $manualNumbers;
    private $logResults;
    private $phoneNumbers;
    private $regionCodes;
    private const VALID = 'Valid';
    private const INVALID = 'Invalid';

    function __construct($files = null, $manualNumbers = null, $regionCodes, $logResults)
    {
        $this->files = $files;
        $this->manualNumbers = $manualNumbers;
        $this->logResults = $logResults;
        $this->regionCodes = $regionCodes;
        //parse passed in files
        if(!is_null($files)) $this->parseFiles();
        //parse numbers passed in as an option
        if(!is_null($manualNumbers)) $this->parseNumbers();

    }

    /**
     * Loops through the supplied txt files and extracts the phone numbers.
     * Phone numbers are validated as uk mobile numbers (including the channel islands).
     * Phone numbers should be separated by new lines.
     */

    public function parseFiles(){

        $phoneUtil = \libphonenumber\PhoneNumberUtil::getInstance();
        $carrierMapper = \libphonenumber\PhoneNumberToCarrierMapper::getInstance();

        foreach ($this->files as $file) {
            $phoneNumbers = file($file);
            foreach ($phoneNumbers as $phoneNumber) {
                $number = trim($phoneNumber);
                try {
                    $phoneNumberObject = $phoneUtil->parse($number, 'GB');
                }
                catch (\libphonenumber\NumberParseException $e) {

                    $this->phoneNumbers[] = array($number,'', self::INVALID);
                    $this->log($number.' '.$e);
                    error_log($e);
                    continue;
                }

                $regionCode = $phoneUtil->getRegionCodeForNumber($phoneNumberObject);
                $valid = false;

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

                $type = $phoneUtil->getNumberType($phoneNumberObject);
                $carrier = '';
                $validMobile = self::INVALID;
                if ($valid && $type === 1) {
                    //if it is a valid mobile number but carrier is unknown
                    $carrier = ($carrierMapper->getNameForNumber($phoneNumberObject, 'en') === '' ? 'unknown': $carrierMapper->getNameForNumber($phoneNumberObject, 'en')) ;
                    $validMobile = self::VALID;
                }
                $this->phoneNumbers[] = array($number,$carrier, $validMobile);
                if($this->logResults){
                    $this->log($number.' carrier:'.$carrier.' status:'.$validMobile);
                }
            }
        }
    }


    /**
     * Loops through the phone numbers supplied as an option .
     * Phone numbers are validated as uk mobile numbers (including the channel islands).
     * Phone numbers should be separated by new lines.
     */

    public function parseNumbers(){
        $phoneUtil = \libphonenumber\PhoneNumberUtil::getInstance();
        $carrierMapper = \libphonenumber\PhoneNumberToCarrierMapper::getInstance();

        foreach ($this->manualNumbers as $phoneNumber){
            $number = trim($phoneNumber);
            try {
                $phoneNumberObject = $phoneUtil->parse($number, 'GB');
            }
            catch (\libphonenumber\NumberParseException $e) {

                $this->phoneNumbers[] = array($number,'', self::INVALID);
                $this->log($number.' '.$e);
                error_log($e);
                continue;
            }

            $regionCode = $phoneUtil->getRegionCodeForNumber($phoneNumberObject);
            $valid = false;

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

            $type = $phoneUtil->getNumberType($phoneNumberObject);
            $carrier = '';
            $validMobile = self::INVALID;
            if ($valid && $type === 1) {
                //if it is a valid mobile number but carrier is unknown
                $carrier = ($carrierMapper->getNameForNumber($phoneNumberObject, 'en') === '' ? 'unknown': $carrierMapper->getNameForNumber($phoneNumberObject, 'en')) ;
                $validMobile = self::VALID;
            }
            $this->phoneNumbers[] = array($number,$carrier, $validMobile);
            if($this->logResults){
                $this->log($number.' carrier:'.$carrier.' status:'.$validMobile);
            }
        }
    }

    /**
     * Writes to custom log file
     */
    public function log($log_msg)
    {
        $log_filename = "logs";
        if (!file_exists($log_filename))
        {
            // create directory/folder uploads.
            mkdir($log_filename, 0777, true);
        }
        $log_file_data = $log_filename.'/log_' . date('d-M-Y') . '.log';
        file_put_contents($log_file_data, '['.date("Y-m-d H:i:s").'] '.$log_msg . "\n", FILE_APPEND);
    }


    /**
     * Returns validated results.
     * @return array
     */

    public function getResults(){
        return $this->phoneNumbers;
    }
}

You include an autoloader but do not utilize it yourself when including the file to get the validator class.

The validator class is a mish mash of functions that seemingly kinda belong together but really don’t. If you want to utilize oop then I’d split it up into different classes so you can have a parser, a validator, a logger, etc. It also create instances of other classes everywhere instead of using DI.

Are there any open source packages you can utilize instead of your own? Are there any open source packages to help out with the cli param process etc?

I’d also think about how your console logging (echoing stuff) and the logging to a file is both actually loggers, just with different output. This might be, combined, and might be helped by utilizing some open source stuff.

You also definitely need to add tests. For your own code, not the open source code you utilize, but you should make sure they have tests. Because tests good.

Thanks for your help! Can you suggest any open source packages i could use?

I found an open source logging package that will eliminate the need for a logger class.

Here is a much better version of the script I made. Apart from the tests can you spot anything wrong with it.

    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;

       
        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;
}

}

Sponsor our Newsletter | Privacy Policy | Terms of Service