Is this proper object oriented PHP?

[DeX]

I seem to have slowed down my system dramatically here by attempting to move everything into objects. Previously I had a setup where the view would create the quote layout and would request any data it needed from the model. So any time it needed a quantity or cost of an item to show on the quote, it would go to the model and the model would actively go to the database and fetch it whenever asked.

 

I have attempted to move away from this on-the-fly requesting from the database and figured it would be better to have the model fetch all of that information in the constructor and make it available whenever a "quote" object is created. I would then create the "quote" object at the beginning of the quote generation process in the view and any time I needed information relating to the quote, I would simply access it by:

$this->quoteDetails = $this->quoteModel->getDetails(); // gets large array which was built in constructor

echo $this->quoteDetails[$productId]['quantity'];
----- or -----
echo $this->quoteDetails[$productId]['unit_price'];

From there I can access any of the product attributes (price, quantity, length....) or I can access any of the information the salesman entered when doing the quote:

echo $this->quoteDetails['inputs']['customer-email'];

The quote is refreshed through Ajax any time a field is changed and it used to take 1 second to fetch the new quote, now it's taking about 6 seconds for obvious reasons. Is this the wrong way to go about this?

[mac_gyver]

are the objects created in session variable(s)? if they are not, they are are all destroyed when the server-side code execution ends, which is at the end of each http request, and they are all re-created for every http request.

[DeX]

They are not stored in the session. Their scope is the page they're created on and that's it.

Is this a good thing or bad thing?

[mac_gyver]

web servers are stateless. all the code and query(ies) you have described start over and run again for every request you are making to the server. an ajax request is still a request to the server. the only thing that may be helping in this case is if the queried for data is still in the database server's query cache, it will be returned from the cache rather than actually executing the query against any database tables.

 

for you to cache the queried for data in the php code and eliminate touching the database server for it, would require that you store the data in a session variable.

 

edit: also, for it to take multiple seconds for this code/query to run once indicates there is some problem, either in the db table design, the queries, or in the php code.

Edited October 1, 2016 by mac_gyver

[maxxd]

I have to agree with mac_gyver - a 6-second query is suspect. Post more details (table structure, query, etc.).

 

Also, just because you're moving to an object-oriented approach doesn't mean you need to pull all the information every time you instantiate the object. You can always initially populate the quote object with the data you know you're going to need (customer details, total, and so on) and then grab the more esoteric or specific data when requested or needed. Granted, if the only purpose of the object is to build a quote view or report, then you will actually need all that data at once. However, I fail to see how - in that instance - the object would have to persist across page loads.

 

Admittedly, I'm not a big fan of stuffing objects into session, but that's just me. I've seen some systems in the past where it just blew up everything one way or another. I can't say it wasn't programmer error (I wasn't working closely with that section of the code) so I may be unjustly biased, but there it is.

[DeX]

Yes, the quote object is needed in entirety only on the Ajax call when the quote is being generated, which happens any time an input field is modified on the quote page. So reading through the answers is it correct to say that I can keep the large object creation in the constructor and I need to look into maybe indexing more tables in the database to speed up the query? I do need all the object variables somewhere on the quote page.

[Jacques1]

The only way to answer the question is to actually measure the time and find the bottleneck.

 

Run the query directly in the database prompt and check the exact time. The execution plan (EXPLAIN SELECT ...) tells you how the query is processed and whether it should be optimized. If that isn't the reason, there may be something wrong with the way you issue queries (e. g. nested loops with individual queries instead of joins). If that still isn't the problem, reduce the code until you've identified the part which is slow. Profilling also helps.

[Stefany93]

OOP requires writing more code, which in turn slows down your code execution/website. Hence why it is used only if you are building big websites (e-shops, forums, etc)

Same with Java, one of the reasons it is so extremely bulky is because you can only write it in OOP.

 

But if your DB queries are slow, it is probably the DB's design fault or the way you are fetching the data needed. Make sure you use indexes where possible, avoid duplicate rows at all costs and use junction tables for many-to-many relationships.  

[DeX]

I took a closer look at how the constructor is building the array of information and it doesn't appear to be the database calls, there are actually very few here. Here is the flow of how the array is built:

- a database call is made to get all product prices and stored for future use (see farther down)

- a database call is made to get all the product names and ID

- a foreach loop is run on each product ID and inside the loop.....

-- the product ID is passed to a function which has a large switch statement with a quantity calculation for each product ID. It finds the matching product ID, runs through the code to get the quantity (based on what the DOM element inputs are) and returns the quantity in an array (see why below)

-- the same for the product widths (separate function)

-- the same for lengths

-- the same for heights

-- then it gets the total quantity of the product. This is because the user could have entered 2 10x12 doors and 5 10x10 doors. It will get the 2 quantities individually in the previous step and the total quantity will be 7.

-- then it gets the price of the product from the price array above

-- then it gets the total price for the product, based on the other attributes and a multiplier by the unit price

 

That's it, I've run the SQL queries individually and they all return instantly, but since it's running all of this individually on 360 products, it takes 4 seconds to complete. I have put an echo statement with a timer after each cycle and it gradually gets up to 4 seconds after running for all 360 products in the database.

 

If you're asking why I can't get all of this information with a single call from the database, it's because the information isn't in the database. The user is entering data on the page to generate the quote and the quote is built entirely from the inputs the user has entered, with minimum database calls to get things like the unit prices of each product.

 

I plan on using a rules system to build the quantity calculations for the products as some of you have helped me with in another thread but that change is being saved for after I complete this object oriented conversion I have going here.

[mac_gyver]

That's it, I've run the SQL queries individually and they all return instantly, but since it's running all of this individually on 360 products, it takes 4 seconds to complete. I have put an echo statement with a timer after each cycle and it gradually gets up to 4 seconds after running for all 360 products in the database.

 

 

 

it's running exactly what part of what you described 'individually' on 360 products?

 

if you mean the switch/case statement, this is exactly where you were at a little over a year ago - https://forums.phpfreaks.com/topic/297053-switch-statement-taking-a-long-time-to-execute/ 

 

each case xyz: statement must be evaluated to find the one that matches the current value and i'm not sure how php finds the next case statement in the tokenized bytecode. if i remember correctly, at the time of that linked to thread, a switch/case statement made up of empty case xyz: break; statements, or at best just an echo or an assignment statement in each case, didn't take that long to execute for all 300 values. however, if php must scan through all the tokenized bytecode to find the token that identifies the start of each/next case : statement, for your actual code, with several statements inside each case, this would take a noticeable and progressively longer time the further down in the list of case statements php must search to find a match.

 

converting to the rules based code that i have suggested will eliminate the switch/case statement. you will still be looping over a set of values, but you will be directly using those values in a function/method call. the time taken for the code execution for each set of values will be the same.

Edited October 4, 2016 by mac_gyver