2
\$\begingroup\$

I wrote a PHP connector that will allow me to communicate with a REST API. This is the first time I am trying to make my code as clean as possible. I want to help myself learning how to better code.

I have done everything I can think of to make my code clean and easy to read, but I would like to get feedback from the experts on how to make my code even better. Is there a way for me to break this code into multiple files instead of one big file? I was hoping I could at least put my current public method into a separate file but not sure how I would do that.

Due the characters limitation I removed some of the methods out of my code for this question, but the full code can be found here.

<?php namespace API;

/**
 * ICWS API
 *
 * @package ICWS
 */
class ICWS {

    private $_myAppName = 'ICWS API connector';
    private $_languageID = 'en-US';
    private $_protocol = 'http';
    private $_sessionIdKey = 'sessionId';
    private $_icwsCallQueueKey = 'icwsCallQueue';
    private $_icwsCurrentUserStatusKey = 'icwsCurrentUserStatusKey';
    private $_icwsUserStatusQueueKey = 'icwsUserStatusQueue';
    private $_maxLoginAttempts = 3;
    private $_loginAttempts = 0;
    private $_debug = false;

    //No need to edit beyond this line
    private $_isSubscribledToQueue = false;
    private $_alternateHostList = array();
    private $_defaultAttributes = array();

    private $_queueType = 1;
    private $_userID;
    private $_password;
    private $_workstation;
    private $_queueName;
    private $_cainfo;
    private $_baseURL;
    private $_csrfToken;
    private $_sessionId;
    private $_ININ_ICWS_CSRF_Token;
    private $_Location;
    private $_subscriptionId;
    private $_curlHeader;
    private $_requestFile;
    private $_activeInteractionId = 0;
    private $_onPhone = false;

    public function __construct($config)
    {

        //Make sure all required config are set
        if(    !isset($config['host']) || empty($config['host'])
            || !isset($config['port']) || empty($config['port'])
            || !isset($config['userID']) || empty($config['userID'])
            || !isset($config['password']) || empty($config['password'])
            || !isset($config['workstation']) || empty($config['workstation'])
        ){
            throw new ApiException('Host, Port, userID, password, and workstation are required in the configuration!');
        }

        $this->_userID = $config['userID'];
        $this->_password = $config['password'];
        $this->_workstation = $config['workstation'];


        //set the sessionId if it already exists
        if( isset( $_COOKIE[$this->_sessionIdKey] ) && !empty( $_COOKIE[$this->_sessionIdKey] )){
            $this->_sessionId = $_COOKIE[$this->_sessionIdKey];
        }

        if(isset($_COOKIE['ININ-ICWS-CSRF-Token']) && !empty($_COOKIE['ININ-ICWS-CSRF-Token'])){
            $this->_ININ_ICWS_CSRF_Token = $_COOKIE['ININ-ICWS-CSRF-Token'];
        }
        $this->_baseURL = $this->_protocol . '://' . $config['host'] . ':' . $config['port'] . '/icws/';
        $this->_subscriptionId = $this->_userID;

    }

    /**
    * Authentication the user and generated a sessionId
    *
    * @param string $userID
    * @param string $password
    * @param boolean $forceNewSession
    * @catch exception
    * @return void
    */  
    public function createSession($forceNewSession = false){

        if( !empty($this->_sessionId) && ! $forceNewSession ){
            return;
        }

        if($forceNewSession){
            $this->destroySession();
        }

        $this->_requestFile = 'connection';
        $type = 'urn:inin.com:connection:icAuthConnectionRequestSettings';

        $data = array('__type' => $type,
                      'applicationName' => $this->_myAppName,
                      'userID' => $this->_userID,
                      'password' => $this->_password);

        $this->_curlHeader = array('Accept-Language: ' . $this->_languageID,
                                   'Content-Type: application/json');
        $httpCode = 0;

        try {
            ++$this->_loginAttempts;
            $data = $this->_processRequest('POST', 'connection', $data, $httpCode, false);

            if($this->_debug){
                new showVar($data, false, 'HTTP Code: ' . $httpCode);
            }

            if( ($httpCode == 200 || $httpCode = 201)
                && $this->_setCheck($data['sessionId'])
                && $this->_setCheck($data['csrfToken'])
            ){

                $this->_sessionId = $data['sessionId'];
                $this->_csrfToken = $data['csrfToken'];

                $this->_alternateHostList = $this->_setVar($data['alternateHostList'], array());

                setCookie($this->_sessionIdKey, $data['sessionId']);
                $this->_loginAttempts = 0;

                //Login and assign the workstation
                $this->_setWorkStation();

                //subscribe to a the user's Queue
                $this->_subscribeToQueue();

                //subscribe to the user status
                $this->_updateMessageSubscription();

                //clear out the user's queue cache
                $this->flushMessages();

            } else {
                $this->_reconnect();
            }

        } catch (\Exception  $e){
            $this->_displayError($e);
        }
    }

    /**
    * Destroy the IC session
    *
    * @return void
    */      
    public function destroySession(){

        //destroy the sessionId
        $this->_sessionId = NULL;
        $this->_destroy($this->_sessionIdKey);

        //destroy the CSRF-Token
        $this->_ININ_ICWS_CSRF_Token = NULL;
        $this->_destroy('ININ-ICWS-CSRF-Token');    

        //destroy the CSRF-Token
        $this->_ININ_ICWS_CSRF_ID = NULL;
        $this->_destroy('ININ-ICWS-CSRF-ID');   

    }   

    /**
    * Reads the messages inside the user queue
    *
    * @return boolean
    */      
    public function _viewMessagingChannel(){

        $this->_requestFile = 'messages';
        $httpCode = 0;
        $data = $this->_sendRequest('GET', 'messaging/messages', false, $httpCode);


        if( $this->_debug){
            new showVar($data, false, 'HTTP Code: ' . $httpCode);
        }


        if(   ( $httpCode == 200 || $httpCode == 201)
            && $data && !empty($data)){
            return $data;
        } elseif( $httpCode == 401 ){
            $this->_reconnect();
            $this->_viewMessagingChannel();
        }

        return false;
    }

    /**
    * Process messages from the user Queue
    *
    * @return void
    */  
    public function processMessages(){

        //read the messages
        $msg = $this->_viewMessagingChannel();


        if($msg === false){
            return false;
        }

        foreach($msg as $m){

            //handle Phone calls
            if($m['__type'] == 'urn:inin.com:queues:queueContentsMessage'){
                $hand = $this->_handleQueueMessageCall($m);
                if($hand !== false){

                    $calls[] = $hand;
                }
            } 
        }

    }

    /**
    * Looks at the existing cache and check the current status and it either updates it or remove it
    *
    * @return void
    */  
    public function flushMessages(){

        $msg = $this->getCallsQueue();

        if( empty($msg) ){
            return false;
        }
        $data = $this->getDefaultAttributes();
        foreach($msg as $k=>$v){

            $newData = $this->getInteractionAttributes($data, $k);
            if(empty($newData)){
                $this->_removeCache($this->_icwsCallQueueKey, $k);
            } else {
                $this->_updateCache($this->_icwsCallQueueKey, $k, $newData);
            }
        }

    }

    /**
    * sets the default attributes to be used
    *
    * @params array $fields
    * @params boolean $ignoreDefaults
    * @return void
    */
    public function setDefaultAttributes($fields = array(), $ignoreDefaults = false ){

        $data = array('Eic_RemoteName',
                      'Eic_RemoteAddress',
                      'Eic_State',
                      'Eic_CallDirection',
                      'Eic_ObjectType',
                      'Eic_LocalAddress',
                      'Eic_CallState',
                      'Eic_Muted');

        $final = $data;

        if($ignoreDefaults && !empty($fields) ){
            $final = $fields;
        } elseif( !empty($fields) ){
            $final = array_unique(array_merge($data, $fields));
        }

        $this->_defaultAttributes = $final;
    }


    /**
    * return the default attributes to be used
    *
    * @return array
    */
    public function getDefaultAttributes(){
        return $this->_defaultAttributes;
    }

    /**
    * Convert The Phone Call Message Received into short format
    *
    * @params array $msg
    * @return array
    */      
    private function _handleQueueMessageCall($msg){

        //new showVar($msg);

        if(isset($msg['isDelta']) && empty($msg['isDelta'])){
            return false;
        }

        if(!isset($msg['interactionsAdded']) && !isset($msg['interactionsChanged']) && !isset($msg['interactionsRemoved'])){            
            return false;
        }


        $final = array();
        $myCall = array();
        $status = '';

        if( isset($msg['interactionsAdded']) ){
            $status = 'Added';
        }

        if( isset($msg['interactionsChanged']) ){
            $status = 'Changed';
        }

        if( isset($msg['interactionsRemoved']) ){
            $status = 'Removed';
        }

        $myCall = $msg['interactions' . $status];

        if( empty($myCall) ){
            return false;
        }

        foreach($myCall as $call){

            if($status == 'Removed'){
                $this->_removeCache($this->_icwsCallQueueKey, $call);
            }

            $c = array();
            $attribute = array();

            if( !isset($call['attributes']) || !isset($call['interactionId'])|| empty($status) ){
                return false;
            } 

            $interactionId = $call['interactionId'];
            $attribute = $call['attributes'];

            //attribute
            if( !in_array('Eic_State', $this->_defaultAttributes) ){
                $this->setDefaultAttributes( array('Eic_State') );
            }

            $fields = $this->getDefaultAttributes();

            foreach($fields as $kf){
                $c[$kf] = isset($attribute[$kf]) ? $attribute[$kf] : '';
            }

            if($c['Eic_State'] == 'C'){
                $this->_activeInteractionId = $interactionId;
            }

            if(!empty($c)){
                $final[] = $c;

                $userStatus = array('activeInteractionId' => $this->_activeInteractionId, 'onPhone' => $this->_onPhone);
                $this->_updateCache($this->_icwsCallQueueKey, $interactionId, $attribute);

            }
        }

        if(empty($final)){
            return false;   
        }

        return $final;
    }

    /**
    * Add/Update the cached array in the session
    *
    * @param bigint $id
    * @param array $field
    * @return void
    */  
    private function _updateCache($key, $id, $field){

        //create a new if the $id does not exists in the cache
        if( !isset($_SESSION[$key][$id]) ){
            $_SESSION[$key][$id] = $field;
        } else {

            if(is_array($_SESSION[$key][$id])){
                $existingValues = $_SESSION[$key][$id];

                foreach($field as $k=>$v){
                    $existingValues[$k] = $v;
                }

                $_SESSION[$key][$id] = $existingValues;
            }
        }
    }

    /**
    * removes the $id from the cache/session
    *
    * @param bigint $id
    * @return boolean
    */      
    private function _removeCache($key, $id){

        if( !isset($_SESSION[$key][$id])){
            return false;
        }

        unset($_SESSION[$key][$id]);

        return true;

    }


    /**
    * Reads the current user's status cache session
    *
    * @return array
    */      
    public function getCurrentUserStatusQueue(){

        $data = array();

        if(isset($_SESSION[$this->_icwsCurrentUserStatusKey])){
            $data = $_SESSION[$this->_icwsCurrentUserStatusKey];
        }

        return $data;
    }

    /**
    * Reads the current called cached session
    *
    * @return array
    */      
    public function getCallsQueue(){

        $data = array();

        if(isset($_SESSION[$this->_icwsCallQueueKey])){
            $data = $_SESSION[$this->_icwsCallQueueKey];
        }

        return $data;
    }

    /**
    * Get Interaction Attributes a phone call
    *
    * @param array $data
    * @return array with the attributes or false for error
    */      
    public function getInteractionAttributes($fields, $id){

        if(empty($id)){
            return false;
        }

        $this->_requestFile = 'getInteractionAttributes';

        if(!$fields || !$id){
            return false;
        }       

        $httpCode = 0;
        $data = array('select' => implode(',', $fields) );

        $data = $this->_sendRequest('GET', 'interactions/' . $id, $data, $httpCode);

        if($httpCode == 200 || $httpCode == 201){
            return $data;
        } elseif($httpCode == 401){
            $this->_reconnect();
            $this->getInteractionAttributes($fields, $id);
        }

        return false;
    }

    /**
    * Calls any Method that does not require a sessionId using a GET method
    *
    * @param string $uri 
    * @param array $data
    * @catch exception
    * @return array or false
    */
    public function getData($uri, $data = false){
        $this->_requestFile = 'getData';
        $return = false;
        $httpCode = 0;
        try {
            $return = $this->_processRequest('GET', $uri, $data, $httpCode);

            if( $this->_debug){
                new showVar($return, false, 'HTTP Code: ' . $httpCode);
            }

        } catch (\Exception  $e){
            $this->_displayError($e);
        } finally {
            return $return;
        }       
    }

    /**
    * Transfer a Phone Call - NOT TESTED
    *
    * @param integer $id
    * @param string $queueName
    * @param integer $queueType
    * @return boolean
    */      
    public function blindTransferCall($id, $queueName, $queueType = 1){

        if(empty($id) || empty($queueName) || !$queueType){
            return false;
        }

        $this->_requestFile = 'blind-transfer';
        $data = array('queueName' => $queueName,
                      'queueType' => $queueType);
        $httpCode = 0;
        $data = $this->_sendRequest('POST', 'interactions/' .  $id .'/blind-transfer', $data, $httpCode);

        if( $this->_debug){
            new showVar($data, false, 'HTTP Code: ' . $httpCode);
        }

        if($httpCode == 200 || $httpCode == 201){
            return true;
        } elseif($httpCode == 401){
            $this->_reconnect();
            $this->blindTransferCall($id, $queueName, $queueType);
        }

        return false;
    }   

    /**
    * Disconnect Phone Call
    *
    * @param integer $id
    * @return boolean
    */      
    public function disconnectCall($id){

        if(empty($id)){
            return false;
        }

        $this->_requestFile = 'disconnect';
        $httpCode = 0;
        $data = $this->_sendRequest('POST', 'interactions/' .  $id .'/disconnect', false, $httpCode);

        if( $this->_debug){
            new showVar($data, false, 'HTTP Code: ' . $httpCode);
        }

        if($httpCode == 200 || $httpCode == 201){

            return true;

        } elseif($httpCode == 401){
            $this->_reconnect();
            $this->disconnectCall($id);
        }

        return false;
    }

    /**
    * UnMute a phone call
    *
    * @param integer $id
    * @return boolean
    */      
    public function unMuteCall($id){

        if(empty($id)){
            return false;
        }

        $this->_requestFile = 'unmute';
        $httpCode = 0;
        $data = $this->_sendRequest('POST', 'interactions/' .  $id .'/mute', array('on' => false), $httpCode);

        if( $this->_debug){
            new showVar($data, false, 'HTTP Code: ' . $httpCode);
        }

        if($httpCode == 200 || $httpCode == 201){
            return true;
        } elseif( $httpCode == 401){
            $this->unMuteCall($id);
        }

        return false;
    }

    /**
    * Pick up a phone call
    *
    * @param integer $id
    * @return boolean
    */      
    public function answerCall($id){

        if(empty($id)){
            return false;
        }

        $this->_requestFile = 'pickup';
        $httpCode = 0;
        $data = $this->_sendRequest('POST', 'interactions/' .  $id .'/pickup',false, $httpCode);
        if( $this->_debug){
            new showVar($data, false, 'HTTP Code: ' . $httpCode);
        }

        if($httpCode == 200 || $httpCode == 201){
            return true;
        } elseif( $httpCode == 401 ){
            $this->_reconnect();
            $this->answerCall($id);
        }

        return false;
    }   

    /**
    * Places the caller on hold
    *
    * @param integer $id
    * @return boolean
    */      
    public function holdCall($id){

        if(empty($id)){
            return false;
        }

        $this->_requestFile = 'hold';
        $httpCode = 0;
        $data = $this->_sendRequest('POST', 'interactions/' .  $id .'/hold', array('on' => true), $httpCode);

        if( $this->_debug){
            new showVar($data, false, 'HTTP Code: ' . $httpCode);
        }

        if($httpCode == 200 || $httpCode == 201){
            return true;
        } elseif($httpCode == 401){
            $this->_reconnect();
            $this->holdCall($id);
        }

        return false;
    }

    /**
    * Places a call
    *
    * @param string $phoneNumber
    * @param string $workstation
    * @return bigint interactionId or false
    * @Documentation
    */  
    public function placeCall($phoneNumber){

        $toCall = $this->_returnPhoneNumber($phoneNumber);

        if(!$this->_isValidPhoneNumber($toCall)){
            return false;
        }

        $this->_requestFile = 'call';
        $params = array('__type' => 'urn:inin.com:interactions:createCallParameters',
                        'target' => $phoneNumber);
        $httpCode = 0;                 
        $data = $this->_sendRequest('POST', 'interactions', $params, $httpCode);

        if( $this->_debug){
            new showVar($data, false, 'HTTP Code: ' . $httpCode);
        }

        if( ($httpCode == 200 || $httpCode == 201 ) 
            && isset($data['interactionId']) && !empty($data['interactionId']) ){

            return $data['interactionId'];
        } else if($httpCode == 401){
            $this->_reconnect();
            $this->placeCall($phoneNumber);
        }   

        return false;
    }

    /**
    * Validates if the number is a valid us number
    *
    * @param string $num
    * @return bigint interactionId or false
    */  
    private function _isValidPhoneNumber($num){

        if(isset($num[9]) && !isset($num[10]) && is_numeric($num)){
            return true;
        }

        return false;
    }

    /**
    * Places a call
    *
    * @param string $phoneNumber
    * @param string $workstation
    * @return bigint interactionId or false
    */  
    private function _setWorkStation(){

        $this->_requestFile = 'setWorkStation';
        $httpCode = 0;
        $callParms = array('__type' => 'urn:inin.com:connection:workstationSettings',
                           'workstation' =>  $this->_workstation);

        $data = $this->_sendRequest('PUT', 'connection/station', $callParms, $httpCode);

        if( $this->_debug){
            new showVar($data, false, 'HTTP Code: ' . $httpCode);
        }

        if($httpCode == 200 || $httpCode == 201){
            return true;
        } else if($httpCode == 401){
            $this->_reconnect();
            $this->_setWorkStation();
        }

        return false;
    }   

    /**
    * Creates or updates a subscription to an interaction queue. 
    *
    * @param integer $id
    * @return boolean
    */      
    private function _subscribeToQueue(){

        $this->_requestFile = 'subscribeToQueue';
        $httpCode = 0;
        $attributes = $this->getDefaultAttributes();

        if(empty($attributes)){
            $attributes[] = 'Eic_State';
        }

        $data = array ('queueIds' =>  array(
                                            array('queueType' => $this->_queueType,
                                                  'queueName' => $this->_queueName)
                                        ),

                       'attributeNames' => $attributes
                        );

        $data = $this->_sendRequest('PUT', 'messaging/subscriptions/queues/' . $this->_subscriptionId , $data, $httpCode);

        if( $this->_debug){
            new showVar($data, false, 'HTTP Code: ' . $httpCode);
        }

        if($httpCode == 200 || $httpCode == 201){
            $this->_isSubscribledToQueue = true;
            return true;
        } elseif($httpCode == 401){
            $this->_reconnect();
            $this->_subscribeToQueue($id);
        }

        return false;
    }

    /**
    * Delete subscription to an interaction queue. 
    *
    * @param integer $id
    * @return boolean
    */      
    private function _deleteSubscription($id){

        if(empty($id)){
            return false;
        }

        $this->_requestFile = 'deleteSubscription';
        $httpCode = 0;
        $data = $this->_sendRequest('DELETE', 'messaging/subscriptions/queues/' .  $id , false, $httpCode);

        if( $this->_debug){
            new showVar($data, false, 'HTTP Code: ' . $httpCode);
        }

        if($httpCode == 200 || $httpCode == 201){
            $this->_isSubscribledToQueue = false;
            return true;
        } elseif($httpCode == 401){
            $this->_reconnect();
            $this->_deleteSubscription($id);
        }

        return false;
    }


    /**
    * Adds user status to the message queue
    *
    * @return void
    */          
    private function _updateMessageSubscription(){
        $this->_requestFile = 'updateMessageSubscription';
        $httpCode = 0;
        $data = $this->_sendRequest('PUT', 'messaging/subscriptions/status/user-statuses' , array('userIds' => array($this->_userID)) , $httpCode);

        if( $this->_debug){
            new showVar($data, false, 'HTTP Code: ' . $httpCode);
        }
    }

    /**
    * Calls any Method after a session is created
    *
    * @param string $method 
    * @param string $uri 
    * @param array $data
    * @catch exception
    * @return array or false
    */
    private function _sendRequest($method, $uri, $data = false, &$httpCode = 0){

        if( !$this->_sessionId ){
            return false;
        }

        $uri = $this->_sessionId . '/' . $uri;
        $return = false;
        //,'Cookie: ' . $this->_ININ_ICWS_Cookie
        $this->_curlHeader = array('ININ-ICWS-CSRF-Token: ' . $this->_ININ_ICWS_CSRF_Token,
                                   'ININ-ICWS-Session-ID: ' . $this->_sessionId,
                                   'Content-Type: application/json');

        try {
            $return = $this->_processRequest($method, $uri, $data, $httpCode);
        } catch (\Exception  $e){
            $this->_displayError($e);
        } finally {
            return $return;
        }       
    }

    /**
    * Initialize the proper cURL call to be executed
    *
    * @param string $method 
    * @param string $uri 
    * @param array $data
    * @return curl_init() $ch
    */
    private function _curlInitialize($method, $uri, $data = false){
        $ch = curl_init();
        $url = $this->_baseURL . $uri;

        if( 
               ($method == 'POST' || $method == 'PUT') 
            && $data
        ){
            $jsonString = json_encode($data);
            curl_setopt( $ch, CURLOPT_POSTFIELDS, $jsonString );

        }

        if($method == 'POST'){
            curl_setopt($ch, CURLOPT_POST, true);
        } elseif( $method == 'PUT'){
            curl_setopt($ch, CURLOPT_CUSTOMREQUEST, 'PUT');
        } else {
            if ($data){
                $url = sprintf("%s?%s", $url, http_build_query($data, '', '&'));
            }
        }   

        //set the URL
        curl_setopt($ch, CURLOPT_URL, $url);

        //disable the use of cached connection
        curl_setopt($ch, CURLOPT_FRESH_CONNECT, true);

        //return the respond from the API
        curl_setopt($ch, CURLOPT_RETURNTRANSFER, true);

        //return the HEADER respond from the API
        curl_setopt($ch, CURLOPT_HEADER, true);

        //add custom headers
        if(!empty($this->_curlHeader)){
            curl_setopt($ch, CURLOPT_HTTPHEADER, $this->_curlHeader);
        }

        //add the cookie value

        $cookiesFile = 'icwsCookies';
        curl_setopt($ch, CURLOPT_COOKIEJAR, $cookiesFile); // write
        curl_setopt($ch, CURLOPT_COOKIEFILE, $cookiesFile); // read


        //enable SSL
        if( $this->_protocol == 'https' ){
            curl_setopt($ch, CURLOPT_CAINFO, $this->_cainfo);
            curl_setopt($ch, CURLOPT_SSL_VERIFYPEER, true);
            curl_setopt($ch, CURLOPT_SSL_VERIFYHOST, true);
        }

        return $ch;
    }

    /**
    * Handle the cURL call to the API
    *
    * @throws ApiException
    * @param string $method
    * @param string $uri 
    * @param array $data
    * @param array &$httpCode
    * @return array
    */  
    private function _processRequest($method, $uri, $data = false, &$httpCode = NULL, $allowReconnect = true)
    {

        //initialize the cURL request
        $ch = $this->_curlInitialize($method, $uri, $data);

        //send the request to the API
        $respond = curl_exec($ch);

        //read the http code returned from ICWS
        $httpCode = curl_getinfo($ch, CURLINFO_HTTP_CODE);

        if($this->_debug){
            new showVar(curl_getinfo($ch));
        }
        //throw cURL exception
        if($respond === false){
            $errorNo = curl_errno($ch);
            $errorMessage = curl_error($ch);

            throw new ApiException($errorMessage, $errorNo);
        }   

        list($header, $body) = explode("\r\n\r\n", $respond, 2);

        if($uri == 'connection'){
            $this->_handleReceivedHeaders($header);
        }

        //convert respond to an array
        $result = json_decode($body, true);

        //throw API exception
        if(  $this->_hasAPIError($result) ){
            $errorCode = 0;
            if(isset($result['errorCode'])){
                $errorCode = $result['errorCode'];
            }
            throw new ApiException($result['message'], $errorCode);
        }

        return $result;
    }

    /**
    * Reconnect to the Api and generate a new sessionId
    *
    * @return boolean
    */              
    private function _reconnect(){

        if( $this->_loginAttempts > $this->_maxLoginAttempts){
            throw new ApiException('All Attempts to create a session have been used! Please check your credentials and try again');
            return false;
        } else {
            $this->createSession(true);
        }

    }

    /**
    * Get the cookie HTTP headers and set them as cookie
    *
    * @param array $httpRespond
    * @return void
    */  
    private function _handleReceivedHeaders($httpRespond){

        $header = $this->_http_parse_headers($httpRespond);

        if( isset($header['ININ-ICWS-CSRF-Token']) ){
            $this->_ININ_ICWS_CSRF_Token = $header['ININ-ICWS-CSRF-Token'];
            setCookie('ININ-ICWS-CSRF-Token', $this->_ININ_ICWS_CSRF_Token);
        }       

    }




}

?>
\$\endgroup\$
0

1 Answer 1

2
\$\begingroup\$

There's a couple of things you ought to take into account if you want to write clean, reusable and possibly "shareable" code (ie code that you can put up on github as a package):

  • Please choose a coding standard, and stick to it. The most commonly adopted standard is PHP-FIG. Your coding style seems consistent, but it doesn't quite conform to the recommendations of PHP-FIG.
  • It may seem silly, but ICWS doesn't mean anything to me. It's probably the name of the webservice you're calling, but it could be any number of things, really. It's defined within the API namespace, but that's about all I have to go on, name-wise...
  • The fact that you've defined this class within an API namespace, would lead me to believe that this class might be used in different places. Perhaps some CLI script might use it, in which case echoing markup from the _displayError method is just plain wrong.
  • The Single Responsibility Principle is an important concept to grasp, and apply: A class should have one, and only one reason to change means that a class can have just one task. communicating with an API is a task, displaying errors is another. The _displayError method, therefore does not belong in your class. Get rid of it. Unless the class knows how to handle an exception, it shouldn't catch it, that's the responsibility of the caller.
  • Your code could be more self-documenting by if you'd use type-hints. Not only will your IDE use the doc-blocks, PHP will raise fatal errors if a variable of the wrong type is passed to any given method. This will make debugging and maintaining your code a lot easier.
  • Looking at the full source, there are a couple of things you really need to fix: a class that stretches out over 1500 lines should be revisited. It's very likely to be a module in class-drag. A module is an entity of code (generally a collection of classes) that depend on eachother to perform a task. Each class has its own, distinct role (responsibility) within that module, but it's the module that forms the functional component. Here's an example: it's a REST API client module. It has a client-like class that calls the API (basically, it's a cURL wrapper), but it uses multiple classes to do its job.
  • You seem to be "programming by array". This is generally regarded as bad practice. It makes code bulky quite quickly and rather tedious to maintain. It also makes your class a lot less nice to use: to know what to pass to the constructor, you'd have to look at the source, and read through the entire constructor to know what the array should look like. That's not the way to go
  • The full code also contains a closing ?> tag. If a file contains nothing but PHP code the documentation recommends to omit the closing tag:

If a file is pure PHP code, it is preferable to omit the PHP closing tag at the end of the file. This prevents accidental whitespace or new lines being added after the PHP closing tag, which may cause unwanted effects because PHP will start output buffering when there is no intention from the programmer to send any output at that point in the script.

That's a short list of things that, to me at least, stood out after a quick canter through your code. I'll be revisiting the answer, adding recommendations, some examples and other background information. For now, I'd start looking into separating out the various tasks you're attempting to perform in this single class, and actually turn it into a clean module. Always ask yourself the same thing when adding functionality: Is this part of the "core business" of the class I'm adding it to, or do I just need the functionality this class already provides to add the new feature? If it indeed extends the core functionality (ie: adding support for PUT and DELETE requests to a cURL wrapper that only supports GET), then you should work on the existing class. If you're just using what is already in that class, then you should turn to other classes (existing ones, or create a new one) (ie: Performing a specific GET request with a specific set of parameters does not belong in a cURL wrapper class).


So, what I'd recommend:

Though it's a matter of personal preference, I do believe most people will find the following type of code easier to read:

class ICWS
{
    public function __construct(array $config)
    {
        //this is the constructor
    }

    private function updateCache($key, $id, $field)
    {
        //method body
    }
}

Note the placement of the brackets, the lack of underscores in private method names (the use of _ to denote protected/private members stems from the old PHP4 days, where the public, private and protected keywords didn't exist, and everything was public).
Also note the use of an array type-hint in the constructor, it's a possible type hint, but I wouldn't use array here. What I'd do is something I'll get to in a moment.

But hinting an array still is so vague, it's error prone and it doesn't get rid of those awful isset checks in your constructor. I'd suggest (and you can refer to the github repo I linked to earlier) to create a class that contains all of the arguments required by the constructor. You could provide this class with a single, validate method, that your constructor calls (if you end up using the Config class in various places, have the validate method take an argument that determines what it actually validates, in function of the caller):

public function __construct(Config $config)
{
    //ensure $config is valild
    if ($config->validage(Config::VALIDATE_ICWS)) {
        throw new \InvalidArgumentException(
            sprintf(
                'The config passed to %s is invalid (used %s rules)',
                __CLASS__,
                Config::VALIDATE_ICWS
            )
        );
    }
    $this->userId = $config->getUserId();
    //and so on
}

You can enrich this config object with getters that return the formatted $baseURL value for you, so you don't have to have that messy concatenation statement in your constructor. In fact, Take a look at the Config\Connection class here, it should be a usable example for you.

more updates later

\$\endgroup\$
4
  • \$\begingroup\$ Wow!!! this is great... I appreciate your answer so much +1. I want to start working on the "Single Responsibility Principle" and though the the cache mechanize would be a perfect candidate to handle the cache as it does a single job. I rewrote that class and posted it codeshare.io/AAE2V. is this a good class and is the correct path? then in my original class I would use it like this CacheHandler::updateCache($this->_icwsCallQueueKey, $k, $newData) \$\endgroup\$
    – Mike
    May 26, 2015 at 22:17
  • \$\begingroup\$ @EliasVanOotegen, I took your advice and now my 1 huge class is broken down into 7 different classes. I would like to share the code here so you can review it and give me even more advise if possible, How can I share the new code? should it be a new question? \$\endgroup\$
    – Mike
    May 28, 2015 at 19:51
  • \$\begingroup\$ @Mike: I can't find the post itself, but this kind of situation has been discussed on the meta site. If you've taken some of my advice to heart, and want the refactored code to be reviewed, then yes you should post it as a new question, perhaps link to the follow-up in a comment here, so I can find it easily \$\endgroup\$ May 29, 2015 at 6:46
  • \$\begingroup\$ @EliasVanOotegen I posted my question part to here codereview.stackexchange.com/questions/92257/… Thank you for your time \$\endgroup\$
    – Mike
    May 30, 2015 at 23:10

Your Answer

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

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