I have a script which is doing too much stuff when it gets executed, and as a result the user has to wait for a response. I am doing some heavy things inside like :

  1. Inserting record
  2. Check for settings
  3. Sending sms based on settings (using smsgateway)
  4. generating notification (locally and using FCM)

these are the minimal task which needs to execute for invoice so I can't actually reduce it. PHP also doesn't support threading the which I can divide task and run asynchronously.

I am posting my code for review so that any one from here will provide some better solution.

php script invoice.php

require_once '../controllers/member.php';
    require_once '../controllers/paymentAcc.php';
    require_once '../classes/check_user_subscription.class.php';
    require_once "../controllers/sms_via_module.php";
    class invoice extends member{
        private $response = array();

        public function insert_invoice($request){
            $dbname = "";
                $session = new session;
                $dbname = $session->start_session(get_device(),get_token());
                    $dbname = $_SESSION['db_name'];
                $dbname = $_SESSION['db_name'];

                return json_encode(['error'=>2,'error_msg'=>'could not find db','message'=>'check session id or token']);

                $conn = $this->connect($dbname);
                $pay_link = array();


                    $member_response = $this->insert_member($request);
                    $member_response = json_decode($member_response);
                    $request['contact_id'] = $member_response->data->contact_id;
                    $request['code'] = $member_response->data->code;

                    $item_cnt = 0;
                    $pay_cnt = 0;

                    $invoice_row =$conn->query("SELECT MAX(`invoice`) AS `invoice` FROM invoice WHERE `bid` = '{$request['bid']}'")->fetch_assoc();
                        $request['invoice'] = $invoice_row['invoice'] + 1;
                        $sett = $conn->query("SELECT start_invoice FROM invoice_settings WHERE `bid` = '{$request['bid']}'")->fetch_assoc();
                        $request['invoice'] = $sett['start_invoice'];

                    $request['date'] = date('Y-m-d',strtotime($request['date']));
                    $noti = new notification; // notification class object.
                    $sms = new sms_via_module; // sms_via_module class object.

                    $insert_invoice = $conn->query("INSERT INTO invoice(`code`,`invoice`,`contact_id`,`date`,`amount`,`tax`,`tax_amount`,`total`,`employee_id`,`balance`,`bid`,`created_at`,`created_by`) VALUES('{$request['code']}','{$request['invoice']}','{$request['contact_id']}','{$request['date']}','{$request['amount']}','{$request['tax']}','{$request['tax_amount']}','{$request['total']}','{$request['employee_id']}','{$request['balance']}','{$request['bid']}','".date('Y-m-d H:i:s')."','".get_uid()."')");

                    $invoice_id = $conn->insert_id;
                    if($invoice_id != 0){
                        $today = date('Y-m-d');
                        $update = 0;
                        $item_info = array();
                        foreach ($request['invoice_item'] as $key => $item) {
                            $start_date = date('Y-m-d',strtotime($item['start_date']));
                            $end_date = date('Y-m-d',strtotime($item['end_date']));
                            if($end_date > $today && $update == 0){
                                $update_member = $conn->query("UPDATE member SET status = '1' WHERE code = '{$request['code']}' AND contact_id = '{$request['contact_id']}'");
                                $update = 1; 

                            $insert_invoice_item = $conn->query("INSERT INTO invoice_item(`invoice_id`,`category_id`,`service_id`,`start_date`,`end_date`,`batch_id`,`amount`,`discount`,`sessions`,`sessions_used`,`created_at`,`created_by`) VALUES('{$invoice_id}','{$item['category_id']}','{$item['service_id']}','{$start_date}','{$end_date}','{$item['batch_id']}','{$item['amount']}','{$item['discount']}','{$item['sessions']}','{$item['session_used']}','".date('Y-m-d H:i:s')."','".get_uid()."')");

                                $item_id = $conn->insert_id;
                                /* Generate notification 
                                $service = $conn->query("SELECT service,duration,duration_type FROM service WHERE id = '{$item['service_id']}'")->fetch_assoc();
                                $noti->create_noti($conn,'Invoiced','Package ',get_uid(),$item_id,$service['service'],'Duration '.$service['duration'].$service['duration_type'].' AND Amount '.$item['amount'].' INR',get_bid(),$request['contact_id'],$item['service_id']); */

                                /* Creating followup for package expiry */

                        $payment =$conn->query("SELECT MAX(`receipt`) AS `receipt` FROM payment WHERE `bid` = '{$request['bid']}'")->fetch_assoc();

                        $request['receipt'] = !empty($payment['receipt']) ? ($payment['receipt'] + 1) : 1;

                        $row = $conn->query("SELECT invoice,fullname,mobile FROM cont_invoice_view WHERE id = '{$invoice_id}' ")->fetch_assoc();

                        /* Saving expiry sms according to action_request settings */
                        foreach ($item_info as $key => $item_obj){
                            $category = $conn->query("SELECT category FROM category WHERE id = '{$item_obj['category_id']}'")->fetch_assoc();
                            $sms->save($conn,$dbname,1,4,$item_obj['end_date'],'expiry_sms',['fname'=>$row['fullname'],'category'=>$category['category'],'end_date'=> $item_obj['end_date']],$row['mobile'],'expiry sms','invoice_invoiceitem_view',$invoice_id);

                        // saving payments
                        foreach ($request['payment'] as $key => $payment) {
                            $payment['reminder'] = date('Y-m-d',strtotime($payment['reminder']));
                            $payment['date'] = date('Y-m-d',strtotime($payment['date']));
                            if($payment['pay_mode'] == 4){
                                $conn1 = $this->connect("pf_central");
                                $select_client = $conn1->query("SELECT `u`.`id` FROM client_db AS `c` INNER JOIN user AS `u` ON `c`.`id` = `u`.`client_id` WHERE `db_name` = '{$dbname}'");
                                $client = $select_client->fetch_assoc();

                                $select_cont = $conn->query("SELECT `mobile`,`fullname`,`email` FROM contact_contactgroup_view WHERE `id` = '{$request['contact_id']}'");
                                $contact = $select_cont->fetch_assoc();

                                $opg = new paymentAcc;
                                $pay_res = json_decode($opg->create_payment_link(array('userid'=>$client['id'],'bid'=>$request['bid'],'purpose'=>$payment['comment'],'amount'=>$payment['amount'],'buyer_name'=>$contact['fullname'],'email'=>$contact['email'],'mobile'=>$contact['mobile'])));
                                    $request['pay_link'] = $pay_res->data->pay_link[0];
                                    array_push($pay_link, $pay_res->data->pay_link[0]);

                            $insert_payment = $conn->query("INSERT INTO payment(`invoice_id`,`date`,`amount`,`reminder`,`balance`,`receipt`,`pay_mode`,`comment`,`clear`,`bid`,`pay_link`,`created_at`,`created_by`) VALUES('{$invoice_id}','{$payment['date']}','{$payment['amount']}','{$payment['reminder']}','{$payment['balance']}','{$request['receipt']}','{$payment['pay_mode']}','{$payment['comment']}','{$payment['clear']}','{$request['bid']}','{$request['pay_link']}','".date('Y-m-d H:i:s')."','".get_uid()."')");


                                $payment_id = $conn->insert_id;
                                if($payment['clear'] && $payment['amount'] > 0){
                                    /* Generate notification */
                                    $noti->create_noti($conn,'Received','Payment',get_uid(),$payment_id,$payment['amount'].' INR','From '.$row['fullname'].' against invoice '.$row['invoice'],get_bid(),$request['contact_id'],$invoice_id);

                                    /* Send sms for payment clearence */
                                else if($payment['balance'] > 0 && !empty($payment['reminder']) ){

                                    $sms->save($conn,$dbname,1,3,$payment['reminder'],'payment_reminder',['fname'=>$row['fullname'],'balance'=>$payment['balance'],'reminder'=>$payment['reminder']],$row['mobile'],'payment reminder','invoice_payment_view',$payment_id);


                                if($payment['pay_mode'] == 2)
                                    $sms->save($conn,$dbname,1,19,$payment['reminder'],'payment_cheque_sms',['balance'=>$payment['balance'],'reminder'=>$payment['reminder']],$row['mobile'],'payment cheque sms','invoice_payment_view',$payment_id);
                                if($payment['pay_mode'] == 4 )


                    if($insert_invoice && (count($request['invoice_item']) == $item_cnt) && (count($request['payment']) == $payment_cnt)){

                        // Update to_date of member bio.
                        $default_extn = 0; //read from client conf.
                        $user = new check_user_subscription;
                        $to_date = $user->get_date($conn,$request['contact_id'],$request['code'],$default_extn);
                        $update_member = $conn->query("UPDATE member_bio SET to_date = '{$to_date}' WHERE code = '{$request['code']}' AND contact_id = '{$request['contact_id']}' ");

                        /* Send sms */
                        if($conn->query("SELECT tnc_accept FROM invoice_accept WHERE (tnc_accept = '1' OR tnc_accept = '2') AND bid = '".get_bid()."' ")->num_rows)

                        /* Generate notification */
                        $noti->create_noti($conn,'Created','Invoice',get_uid(),$invoice_id,$row['invoice'],$row['fullname'].' with Total amount '.$request['total'].' INR and Balance amount '.$request['balance'].' INR',get_bid(),$request['contact_id'],$invoice_id);

                        $response['data']['pay_link'] = $pay_link;
                        $response['data']['invoice_id'] = $invoice_id;
                        $response['data']['invoice'] = $request['invoice'];
                        $response['error'] = 0;
                        $response['error_msg'] = '';
                        $response['message'] = 'Invoice added';
                        $response['data'] = array();
                        $response['error'] = 1;
                        $response['error_msg'] = 'something went wrong';
                        $response['message'] = 'Please try again';
                    $response['data'] = array();
                    $response['error'] = 1;
                    $response['error_msg'] = 'Contact id is must';
                    $response['message'] = 'Internal execution error';  
                return json_encode($response);

I imagine the biggest slowdown in your code is sending the SMS messages.

As an alternative to spinning them off an a new thread, you could use a worker architecture where you push your SMS messages to a table in the database (with all the info needed to actually send it). Then you have a cron job run every minute that pulls a record from that table and sends the messages.

The additional benefit of such an architecture is:

  1. If the SMS service is down or something else temporarily goes wrong, you can just mark that record as failed and retry it again later. If you have a thread trying to do that, it'll either get stuck for a very long time (maybe being effectively zombified) or will exit without ever sending the SMS and you'll have lost that data.

  2. You can expand it to include all of the tasks you wish to run asynchronously. For instance, if you want to send web socket notifications or emails. These items may or may not come from your website, as well. So you may not have an opportunity to use async code hosted within the site. But an external worker querying a shared database can consume data from anywhere.

As with the response above mine, you should seriously consider breaking your insert_invoice function into multiple functions that can each handle unique bits of logic.

Having everything in one giant function means you or anyone else maintaining the code has to keep all of it in their head at once to navigate it. It quickly becomes unwieldy.

Using different functions for each discrete step allows someone to focus on just what is happening in that step. The codebase becomes far more maintainable and lucid. It also makes it much easier to see where you can refactor to reduce redundant or unnecessary code.

For instance, you should consider making a class for handling your database interactions. Each function shouldn't need to grab its own connection to the database from the raw settings. You could, instead, have a class with a Connect() function that deals with all that implementation complexity. Then have a function like Query(string) that can run queries against that connection and do all the error handling relevant to that connection and query.

Then your functions outside of the database layer can just call that Query function without worrying about the implementation details they're not supposed to know about to begin with.

At that point, you'll have an easier time implementing best practices like using PDO and prepared statements, as you'll only have to make those changes to your DB utility class and not everywhere in your codebase.

| improve this answer | |
  • \$\begingroup\$ +1 ... this approach is preferred over the async approach i suggested in my answer. it is very hard to debug code that doesn't output anything. \$\endgroup\$ – I wrestled a bear once. Jun 28 '17 at 20:07
  • \$\begingroup\$ I agree with your answer but can you give me an example of database class that will handle interaction. @Siegen \$\endgroup\$ – SaMeEr Jun 29 '17 at 8:33
  • \$\begingroup\$ Well, here's an example of one of my first attempts at such, a long time ago: bitbucket.org/sc-api/star-citizen-api/src/… From that, you can see what I mean about the DB class pulling its own settings (from a settings.php file, in this case) and using Connect() and Disconnect() functions to manage its own state. The query functions are overly specific to my use case but you can easily generalize them. \$\endgroup\$ – Siegen Jun 29 '17 at 14:19

There is no built-in way to do threads in PHP but that doesn't mean they're not doable. Here's a class I use frequently to execute long running tasks after the server has responded to the request. Typical use looks like this:

// Do stuff here to echo response to the browser
// Connection is now closed, browser is no longer waiting, 
// safe to perform long-running tasks

Your code is difficult to follow as you have a class with one single, very long method. Break that method up into several methods, you will thank me later.

Your $insert_payment query definitely needs to be done with a prepared statement.

| improve this answer | |

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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