Logic Flow

SFDonovan

I have a script that I am trying to implement some logic. One table is newly created and populated from a csv import. They are exam grades and need to be checked and "cleansed". Basically four things I have to check before I write to the database.

Check if already imported
Validate UID and compare to Student_ID in the imported table.
Check if all Students are assigned to a group.
Check if any students are missing from import.

Here is what I am doing.

//Check if IRAT already imported
$HasError = checkSession($Session_ID);
if ($HasError==TRUE) {
  OpenTable();
echo"<tr><td>IRAT grades for this Session have already been imported.</td></tr>";
echo"<tr><td>You can edit individual IRAT grades <a href='".$admin_file.".php?op=TLViewIratGrades&Session_ID=$Session_ID'>Here!</a></td></tr>";
Closetable();
}
//Validate UID and compare to Student_ID in the imported table.
$HasError = checkUID();
if ($HasError==TRUE) {
OpenTable();
echo"<tr><td>There are errors in the IRAT imported table.  These need to be fixed before writing to the grades table.  Ensure all student UID's match for all current students already in the system.</td></tr>";
echo"<tr><td><input type=\"button\" value=\"Back\" onClick=\"history.go(-1)\"></td></tr>";
Closetable();
}
//Check if all Students are assigned to a group.
$HasError = checkGroup();
if ($HasError==TRUE) {
OpenTable();

etc
etc

Some of my functions look like this:

function checkUID() {
global $prefix, $db;
//Check valid Students UID's and compare to the imported table
$checkUID = $db->sql_query("SELECT a.UID FROM ".$prefix."_tl_students a
LEFT JOIN ".$prefix."_tl_session_grade_import b ON a.UID = b.Student_ID
WHERE a.UID IS NULL");
$error_total = $db->sql_numrows($checkUID);
//We have unmatched UID's!
if ($error_total > 0) {
$HasError=TRUE;
}else{
$HasError=FALSE;
return $HasError;
}
}


function checkSession($Session_ID) {
global $prefix, $db;
$sessionQuery = $db->sql_query("SELECT * FROM ".$prefix."_tl_session_grades WHERE Session_ID = '$Session_ID'");
$Session_total = $db->sql_numrows($sessionQuery);
if ($Session_total > 0) {
$HasError=TRUE;
}else{
$HasError=FALSE;
return $HasError;
}
}


function checkGroup() {
global $prefix, $db;
//Check if all Students are assigned to a group.
$checkGroup = $db->sql_query("SELECT b.*, a.Name_First, a.Name_Last FROM ".$prefix."_tl_session_grade_import a
LEFT JOIN ".$prefix."_tl_students b ON (b.UID = a.Student_ID)
JOIN ".$prefix."_tl_group_students c ON (b.SOMS_KEY = c.SOMS_KEY)
WHERE c.SOMS_KEY IS NULL;");
$group_total = $db->sql_numrows($checkGroup);
if ( $group_total > 0 ) {
$HasError=TRUE;
}else{
$HasError=FALSE;
return $HasError;
}
}

Is this the correct way of handling the flow of logic? Am I returning from the functions correctly?

SFDonovan

Trying to troubleshoot. I don't know why this gets passed as FALSE. I imported this and have a Session_ID =$Session_ID.

$Session_ID = $_POST['Session_ID'];


//Check if IRAT already imported
$HasError = checkSession($Session_ID);
if ($HasError==TRUE) {
  OpenTable();
echo"<tr><td>IRAT grades for this Session have already been imported.</td></tr>";
echo"<tr><td>You can edit individual IRAT grades <a href='".$admin_file.".php?op=TLViewIratGrades&Session_ID=$Session_ID'>Here!</a></td></tr>";
Closetable();
}

The function is below

function checkSession($Session_ID) {
global $prefix, $db;
$sessionQuery = $db->sql_query("SELECT * FROM ".$prefix."_tl_session_grades WHERE Session_ID = '$Session_ID'");
$Session_total = $db->sql_numrows($sessionQuery);
if ($Session_total > 0) {
$HasError=TRUE;
}else{
$HasError=FALSE;
return $HasError;
}
}

If I pass $Session_ID can I return $HasError? Do I have to pass them both?

NogDog

Move the return statement to below the ELSE block. Otherwise, nothing gets returned if the preceding IF condition evaluates as true.

PS: Using

 instead of [code] tags will help make your code easier to read here.

SFDonovan

Thanks NogDog,

Should I also use an if {
}else{

}

when returning from a function? I want to make sure this $return value gets set correctly. My first test is the following:

//Validate UID and compare to Student_ID in the imported table.
$HasError = checkUID();
if ($HasError==TRUE) {
OpenTable();
echo"<tr><td>There are errors in the IRAT imported table.  These need to be fixed before writing to the grades table.  Ensure all student UID's match for all current students already in the system.</td></tr>";
echo"<tr><td><input type=\"button\" value=\"Back\" onClick=\"history.go(-1)\"></td></tr>";
Closetable();
die();
}

function checkUID() {
global $prefix, $db;
//Check valid Students UID's and compare to the imported table
$checkUID = $db->sql_query("SELECT a.UID FROM ".$prefix."_tl_students a
LEFT JOIN ".$prefix."_tl_session_grade_import b ON a.UID = b.Student_ID
WHERE a.UID IS NULL");
$error_total = $db->sql_numrows($checkUID);
//We have unmatched UID's!
if ($error_total > 0) {
$HasError=TRUE;
return $HasError;
}else{
$HasError=FALSE;
return $HasError;
}
}

NogDog

That should be OK, though under the concept of avoiding duplication of code I might do:

<?php
function checkUID() {
   global $prefix, $db;
   //Check valid Students UID's and compare to the imported table
   $checkUID = $db->sql_query("SELECT a.UID FROM ".$prefix."_tl_students a
   LEFT JOIN ".$prefix."_tl_session_grade_import b ON a.UID = b.Student_ID
   WHERE a.UID IS NULL");
   $error_total = $db->sql_numrows($checkUID);
   //We have unmatched UID's!
   if ($error_total > 0) {
      $HasError=TRUE;
   }else{
      $HasError=FALSE;
   }
   return $HasError;
}

Horizon88

You could also use a ternary operator:

return ($error_total > 0) ? TRUE : FALSE;

laserlight

Or even more succinctly:

return $db->sql_numrows($checkUID) > 0;

Horizon88

Always! Always just a LITTLE bit more. 😛 Can't you let me win, just once, LL?

laserlight

Always! Always just a LITTLE bit more. 😛 Can't you let me win, just once, LL?

I would :p But I get irritated when I see such code: I mean, you already have a boolean value, so why do you want to return or assign true/false values? It seems like a little code obfuscation to me.

NogDog

return (bool) $db->sql_numrows($checkUID);

Horizon88

hahaha. now SF just needs to pick one 😛

laserlight

The casting to bool version is less clear than comparison with 0. Instead of thinking "check if the number of rows returned is greater than zero (or non-zero)", you think "what happens when the return value is converted to a boolean value?"

NogDog

laserlight wrote:
The casting to bool version is less clear than comparison with 0. Instead of thinking "check if the number of rows returned is greater than zero (or non-zero)", you think "what happens when the return value is converted to a boolean value?"

I doubt I'd actually use the idiom I suggested (for fun) in my previous reply, but if you think about it, it's every bit as "clear" as the very typical idiom you see people use all the time:

if(count($some_array))
{
   // do something
}

It's exactly the same thing, really: you have to understand that count() will return an integer from 0 - n, and that number will be automatically cast by the IF's condition expression to a boolean false if the result is zero, else it will be true. What you are really doing with the above statement is...

if( (bool) count($some_array) ) {

...but PHP does the type conversion automatically in such a situation, so the explicit casting via "(bool)" would be redundant.

Now for the novice programmer who doesn't even know what "type casting" is, that may be a different story as to what is clear and what is not. But if we've caused one or two of those novices to dig into the manual a bit to figure out wtf I'm talking about, then "mission accomplished!" 🙂

PS: Guess it would be polite of me to include this link for any who want to do some digging: http://www.php.net/manual/en/language.types.type-juggling.php

Weedpacket

NogDog wrote:
...but PHP does the type conversion automatically in such a situation, so the explicit casting via "(bool)" would be redundant.

In which case

return $db->sql_numrows($checkUID);

would be simpler still: if the return value is used in an if() statement to test for nonzeroness then it will be cast to a boolean false/true; in other situations it will be the actual number of rows matched.

In other words, more information is returned to the caller and let it decide what to do with what it gets. In the test code:

echo"<tr><td>There are errors in the IRAT imported table.  These need to be fixed before writing to the grades table.  Ensure all student UID's match for all current students already in the system.</td></tr>";

All we're told is that there are errors, not how many (which might be helpful to know).

NogDog

All depends on what the requirement is for that function and what its API documentation says it will return, in case any clients call it using === or !== comparisons. 🙂

SFDonovan

NogDog wrote:
Now for the novice programmer who doesn't even know what "type casting" is, that may be a different story as to what is clear and what is not. But if we've caused one or two of those novices to dig into the manual a bit to figure out wtf I'm talking about, then "mission accomplished!" 🙂

I really would not need to change datatypes just for testing if I have errors with an imported table. I just need to make sure all records match the unique field I am matching on. If not I have errors that need fixed before writing to the table.

I want to thanks everybody for their detailed input. It was outstanding.

laserlight

I doubt I'd actually use the idiom I suggested (for fun) in my previous reply, but if you think about it, it's every bit as "clear" as the very typical idiom you see people use all the time:

I agree. Such use of count() has the same weakness.

It's exactly the same thing, really: you have to understand that count() will return an integer from 0 - n, and that number will be automatically cast by the IF's condition expression to a boolean false if the result is zero, else it will be true. What you are really doing with the above statement is...

That's the thing: it relies on an implicit conversion instead of being explicit about what you really mean, even though being explicit costs nothing while improving clarity.

All depends on what the requirement is for that function and what its API documentation says it will return, in case any clients call it using === or !== comparisons.

I agree.