[RESOLVED] Is this the right approach to query functions?

gwerner

Okay, I've asked in a previous post how best to handle organizing queries. Should I just include in the HTML as needed. This seems wrong, because all I'm getting is a page full of queries littered throughout the HTML.

Or, build functions and call them as needed, etc. Basically, I was told that it kind of depends on the situation and to take it on a case by case basis. Functions being the best solution if they are utilized multiple times.

Below is what I've come up with for a couple of common queries that seem to be popping up throughout my site. The first function contains a query to find out how many of a particular product has been sold. The second function determines the average monthly usage of this product.

Any thought on what I'm doing wrong or how to make it better is appreciated.

<?php
function soldSum($pId) {
	$query = "SELECT SUM(orderitems_quantity) AS 'totalSold'
				FROM orderitems
				WHERE orderitems_productid = '$pId'";
	$result = mysql_query($query);
	if (!$result) {
		error_log(mysql_error()."\n$query");
		return false;
	}
	$totalSold = array();
	while($row = mysql_fetch_assoc($result)) {
		$totalSold[] = $row['totalSold'];
	}
	return $totalSold;
}

function monthUsage($create, $sold) {
	$diff =  (strtotime(date("Y-m-d")) - strtotime($create));
	$dayAge = ($diff / 86400) / (30.41666666667);
	$productUsage = number_format(($sold / $dayAge), 2);

return $productUsage;
}
?>

I then pop this into a table with other product info to return the results. These functions are being called inside of another query that grabs the other product information.

<?php
foreach(soldSum($products_id) as $key => $option) {
	$usage = monthUsage($products_create, $option);
	if ($option > '0') {
		echo '<td>'.$usage.'</td>';
		echo '<td>'.$option.'</td>';
	}
	else {
		echo '<td>0</td>';
		echo '<td>0</td>';
	}
}
?>

droms

Hi, you didn't state the issue you were getting with it? Is it returning an error?

one note on first look - if ($option > '0') { the 0 should be treated as an int, you're treating it as a string, hence the ' ' , so remove those.

gwerner

I'm not receiving any errors. Just wanted to get assurance that I'm going in the right direction in terms of code structure and development.

The note you gave regarding quotes around integers is exactly the feedback I'm looking for. That seems like a no brainer after you pointed it out. Thanks.

Weedpacket

Moving this to the Code Critique forum.

NogDog

Unless I'm missing something, it looks like the soldSum() function will only ever retrieve, at most, on result row with one column, so having it return an array seems superfluous, as does having a loop to retrieve the results. Also, there would then be no reason to process its result via a foreach loop in the main script.

gwerner

I'm following what you are saying, and you are correct in your assumption that only one row would be returned. I've modified my code a bit. Do you think this is any better?

I removed the while loop and did an extract on the returned row.

<?php
function soldSum($pId) {
	$query = "SELECT SUM(orderitems_quantity) AS totalSold
				FROM orderitems
				WHERE orderitems_productid = '$pId'";
	$result = mysql_query($query);
	if (!$result) {
		error_log(mysql_error()."\n$query");
		return false;
	}
	$row = mysql_fetch_assoc($result);
	extract($row);
	return $totalSold;
}
?>

I'm now calling the function through a variable. Then, I check to see if it's set and display the results on the condition of the if statement.

<?php
$totalSold = soldSum($products_id);
if (isset($totalSold)) {
$usage = monthUsage($products_create, $totalSold);
	echo '<td>'.$usage.'</td>';
	echo '<td>'.$totalSold.'</td>';
}
else {
	echo '<td>0</td>';
	echo '<td>0</td>';
}
?>

I really appreciate your help on this!

bradgrafelman

Using [man]isset/man there is pointless - the variable will always be set (it's defined on the preceding line). It would make more sense to test if the returned value was false (since that's what you've designed as the error condition).

gwerner

What you said makes sense. I used isset because I've been getting a lot of variable undefined errors and I just used this out of habit. The variable is defined. It just may not contain a value.

I'm going to assume this will do the trick.

<?php
if ($totalSold != FALSE) {
?>

This raises another question for me. I'm checking for the result with the code below.

<?php
if (!$result) {
	error_log(mysql_error()."\n$query");
	return false;
}
?>

Does this check for a row returned or if the connection to the database was made? The reason I ask, I'm using a mysql SELECT for a log in form that verifies a user name and password. It always returns true unless I use mysql_num_rows. What am I not understanding here?

<?php
$query = "SELECT users_id, users_perms
		 FROM users
		 WHERE users_name = '$userName' AND users_password = '$mdpass'
		 LIMIT 1";
$result = mysql_query($query);
if (!$result) {
   error_log(mysql_error()."\n$query");
   return false;
}
if (mysql_num_rows($result) > 0) {
?>

Again, I really appreciate the help and am trying hard to gain a better understanding on how all this works.

bradgrafelman

Read the manual when in doubt. For example, the manual page for [man]mysql_query/man says:

For SELECT, SHOW, DESCRIBE, EXPLAIN and other statements returning resultset, mysql_query() returns a resource on success, or FALSE on error.

A SELECT query that doesn't match any rows isn't an error. A malformed query, insufficient permissions, connection failure, etc. etc. are errors, and those conditions will make mysql_query() return false.

gwerner

Thanks, that makes sense. I definitely read the manual as much as I visit these boards. Sometimes what I read doesn't make sense or just doesn't stick. So, I appreciate you walking me through it a bit.