5
\$\begingroup\$

I have created a class that gets data from an ODBC connection. The code works but it is really slow - I'm talking up to 1.20ish minutes to run. I know my code is inefficient but I'm really not sure why it's so slow. Tips or advice on speeding it up would be much appreciated.

<?php

class machine {

    public $name = "";

    public function __construct($name, $Id) {
        $this->name = $name;
        $this->Id = $Id;
    }

    Public function Data() {
        $conn = odbc_connect('monitor', '', '');
        if (!$conn) {
            exit("Connection Failed: " . $conn);
        }

        $sql = "SELECT TOP 1 ReaderData.ReaderIndex, ReaderData.CardID, ReaderData.ReaderDate,     ReaderData.ReaderTime, ReaderData.controllerID, Left([dtReading],10) AS [date], ReaderData.dtReading FROM      ReaderData
                           WHERE ReaderData.controllerID=$this->Id AND ReaderData.CardID = 'FFFFFFF0  '
                       ORDER BY ReaderData.ReaderIndex DESC;";
        $rs = odbc_exec($conn, $sql);
        if (!$rs) {
            exit("Error in SQL");
        }
        while (odbc_fetch_row($rs)) {

            $this->DtReading = odbc_result($rs, "dtReading");

            $result = strtotime($this->DtReading) + 2 * 60 * 60;

            $this->time = time() + 60 * 60 - $result;
            return round($this->time / 60, 2);
        }
    }

    public function Cycle() {
        $this->Arr = array();
        $conn = odbc_connect('monitor', '', '');
        if (!$conn) {
            exit("Connection Failed: " . $conn);
        }

        $sql = "SELECT TOP 2 ReaderData.ReaderIndex, ReaderData.CardID, ReaderData.ReaderDate, ReaderData.ReaderTime, ReaderData.controllerID, Left([dtReading],10) AS [date], ReaderData.dtReading FROM ReaderData WHERE ReaderData.controllerID=$this->Id AND ReaderData.CardID = 'FFFFFFF0  '  ORDER BY ReaderData.ReaderIndex DESC;";
        $rs = odbc_exec($conn, $sql);
        if (!$rs) {
            exit("Error in SQL");
        }

        $data = array();

        while (odbc_fetch_row($rs)) {
            $data[] = $this->DtReading = odbc_result($rs, "dtReading");
        }
        $this->Time1 = ($data[0]);
        $this->Time2 = ($data[1]);
        $Time1E = strtotime($this->Time1) + 2 * 60 * 60;
        $Time2E = strtotime($this->Time2) + 2 * 60 * 60;
        $this->cycle = $Time1E - $Time2E;

        return round($this->cycle, 2);
    }

    public function SageData() {

        $conn = odbc_connect('Data hub', '', '');
        if (!$conn) {
            exit("Connection Failed: " . $conn);
        }
        $sql = "SELECT [SHOP FLOOR PRODUCTION PLAN].[MACHINE], [SHOP FLOOR PRODUCTION PLAN].[cycletime]
    FROM [SHOP FLOOR PRODUCTION PLAN]
    WHERE ((([SHOP FLOOR PRODUCTION PLAN].[MACHINE])='$this->name'));

    ";

        $rs = odbc_exec($conn, $sql);
        if (!$rs) {
            exit("Error in SQL");
        }

        while (odbc_fetch_row($rs)) {

          $SageCycle = odbc_result($rs, "cycletime");
        }
        //  var_dump($this->SageCycle);
        odbc_close($conn);
        return @$SageCycle;
    }

    public function GetM() {

        $q = $this->Cycle();
        $qq = $this->SageData();

        $M = $q - $qq;
        //  $this->P = $this->M / $this->sageData();

        if ($qq == 0) {
            $this->P = 0;
        } else {
            $this->P = $M / $this->sageData();
        }

        return round($this->P, 2) * 100;
    }

    public function name() {

        echo $this->name;
    }

    }

    $machine1 = new machine('ZW01001', 41);
    $machine4 = new machine('ZW01004', 37);
    $machine5 = new machine('ZW01005', 28);
    $machine6 = new machine('ZW01006', 38);
    $machine7 = new machine('ZW01007', 30);
    $machine8 = new machine('ZW01008', 31); 
    $machine9 = new machine('ZW01009', 32);
    $machine10 = new machine('ZW01010', 40);
    $machine21 = new machine('ZW01021', 13);
    $machine22 = new machine('ZW01022', 2);
    $machine23 = new machine('ZW01023', 33);
    $machine24 = new machine('ZW01024', 34);
    $machine25 = new machine('ZW01025', 35);
    $machine26 = new machine('ZW01026', 36);

    $Cycle = (object) array(
            'Machines' => array(
                array(
                    'cycle' => $machine1->Cycle(),
                    'percent' => $machine1->GetM(),
                    'Data' => $machine1->Data()
                ),
                array(
                    'cycle' => $machine4->Cycle(),
                    'percent' => $machine4->GetM(),
                    'Data' => $machine4->Data(),
                ),
                array(
                    'cycle' => $machine5->Cycle(),
                    'percent' => $machine5->GetM(),
                    'Data' => $machine5->Data()
                ),
                array(
                    'cycle' => $machine6->Cycle(),
                    'percent' => $machine6->GetM(),
                    'Data' => $machine6->Data()
                ),
                array(
                    'cycle' => $machine7->Cycle(),
                    'percent' => $machine7->GetM(),
                    'Data' => $machine7->Data()
                ),
                array(
                    'cycle' => $machine8->Cycle(),
                    'percent' => $machine8->GetM(),
                    'Data' => $machine8->Data()
                ),
                array(
                    'cycle' => $machine9->Cycle(),
                    'percent' => $machine9->GetM(),
                    'Data' => $machine9->Data()
                ),
                array(
                    'cycle' => $machine10->Cycle(),
                    'percent' => $machine10->GetM(),
                    'Data' => $machine10->Data()
                ),
                array(
                    'cycle' => $machine21->Cycle(),
                    'percent' => $machine21->GetM(),
                    'Data' => $machine21->Data()
                ),
                array(
                    'cycle' => $machine22->Cycle(),
                    'percent' => $machine22->GetM(),
                    'Data' => $machine22->Data()
                ),
                array(
                    'cycle' => $machine23->Cycle(),
                    'percent' => $machine23->GetM(),
                    'Data' => $machine23->Data()
                ),
                array(
                    'cycle' => $machine24->Cycle(),
                    'percent' => $machine24->GetM(),
                    'Data' => $machine24->Data()
                ),
                array(
                    'cycle' => $machine25->Cycle(),
                    'percent' => $machine25->GetM(),
                    'Data' => $machine25->Data()
                ),
                array(
                    'cycle' => $machine26->Cycle(),
                    'percent' => $machine26->GetM(),
                    'Data' => $machine26->Data()
                ),
            )
    );


    $fp = fopen('Rag.json', 'w');
    fwrite($fp, json_encode($Cycle));
    fclose($fp);
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Please note that by convention, classes should start with an uppercase letter and variables and methods with a lowercase one. This helps keeping apart what is what. \$\endgroup\$
    – ljacqu
    Commented Jul 18, 2014 at 11:22

1 Answer 1

3
\$\begingroup\$

Recycling is the solution

For every Machine, you are performing 3 queries and opening/closing 3 connections.

So for 10 machines, you are performing 30 queries (no problemo) but opening a connection 30 times (w t f?) and closing it again.

There is your problem. Instead of having one connection that you query, you open a connection everytime you need to query something. this is really bad for performance and kills your application.

However, there are multiple things wrong with your code. Not only is it poorly written (it's just a bunch of functions put into a class), the design is simply wrong / there is no design.

Application Design

When writing software, we always solve problems. If the problem can't be solved in a few lines of code, we split the problem into smaller problems that can be solved. If a lot of small problems exist, it is best to create some structure in those solutions. You could for instance bundle them in a class, or namespace or file.

Defining the problem

Getting Data from an ODBC DataSource.

For this, we have a set of functions provided to us by php. the odbc_* functions. Solved. NEXT!

the real problem is that it's a real pain in tha ass writing the exact same odbc_query all over the place. We need an interface to comunicate with that helps us managing MachineObjects that are stored in an odbc MachineDataSource.

Defining the solution

Now we know the problem, we can start splitting it into smaller ones:

  • We need a Machine Objects that holds all the Machine data and adds some extra meaning + calculations for us. (e.g. the GetM() method)
  • I need a MachineDataSource that when I pass in an ID and a Name, it returns me the correct MachineObject

So you need a class that can represent a Machine and you need an interface that creates these Machines for you. I would go for the Repository Pattern here (look it up).

Fine tuning

A good way to fine tune this is to use Eager loading. Instead of performing multiple selects:

SELECT something FROM somewhere WHERE id = 1;
SELECT something FROM somewhere WHERE id = 2;
SELECT something FROM somewhere WHERE id = 3;
SELECT something FROM somewhere WHERE id = 4;
SELECT something FROM somewhere WHERE id = 5;

We could use eager loading and fetch them at the same time:

SELECT something FROM somewhere WHERE id IN(1,2,3,4,5);
//not sure or this query is correct, but you get the point

This however adds a lot of complex stuff to your code, Laravel Eloquent does a good job using a Collection of Objects (Machine's in your story) and then load certain data for all Objects in the Collection.

\$\endgroup\$
2
  • \$\begingroup\$ This response, is fairly amazing, thank you very much, so do you think the biggest performance boost will be from say opening 3 connection one for each function, and then getting all of the data at the same time. \$\endgroup\$ Commented Jul 18, 2014 at 12:23
  • 1
    \$\begingroup\$ The biggest performance boost will come from creating only 1 connection. For that you will have to create 1 class that handles the querying (and a private var $this->conn) that returns the Machine objects. This will reduce the connection-creation problem. If the app is still slow, you can start experimenting with the eagerloading. this will give big performance boosts for big Collections, but will slow dan small collections. So this should be testes, tested and tested \$\endgroup\$
    – Pinoniq
    Commented Jul 18, 2014 at 12:48

Your Answer

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

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