2
\$\begingroup\$

The stored procedures must return their first resultset in the form:

(
    [index] int,
    [id] varchar(50)
)

where

  • [index] is 0,1,2,3,... and
  • [id] is the name of the struct.

As long as the first resultset returns the names of the resultsets in the same order they are returned by the stored procedure the php struct will store each struct in the struct by it's name.

Example ResultSet 0:

0,'resultsets'
1,'codes'
2,'listings'
3,'users'

Example Function Call:

$struct = genericMultiResults('exec dbo.genericProcedure');

Example Results:

$struct['resultsets'][0]; // 'resultsets'
$struct['resultsets'][1]; // 'codes'
foreach($struct['codes'] as $code) {  // loop over the records of the codes resultset
    ...
}
foreach($struct['listings'] as $listing) { /* loop over the records of the listings resultset */
    ...
}

Implementation:

function genericMultiResultSet($sql) {
    global $conn;

    // resultset holder
    $resultSets = array();
    // resultsets key name, can be changed by record 0 of resultset 0
    $resultSetsKey = 'resultsets';
    // current resultset name, only used by resultsets 1 and above, not by 0, defaults to name of 0
    $resultSetKey  = 'resultsets';

    // execute multi-resultset procedure or sql statement
    $results = sqlsrv_query($conn,$sql);
    // assume we have the first resultset, loop over resultsets
    do {
        // loop over rows in resultset
        while($row = sqlsrv_fetch_array($results,SQLSRV_FETCH_ASSOC)) {
            // if the structure is $row['index'], $row['id'] then assume it is a resultsets key
            if(isset($row['index']) && isset($row['id']) && count($row) == 2) {
                // if resultsets key not created yet, create it
                if(count($resultSets) == 0) {
                    $resultSets[$row['id']] = array();
                    // assume first record in resultset describes itself (the resultset, not the row)
                    $resultSetsKey = $row['id'];                    
                }
                // add entry to resultsets key
                $resultSets[$resultSetsKey][$row['index']] = $row['id'];
            // otherwise, not a resultsets key
            } else {
                // if no array for current $resultset create it
                if(!isset($resultSets[$resultSetKey])) {
                    $resultSets[$resultSetKey] = array();
                }
                // add row to it's resultset within $resultSets
                array_push($resultSets[$resultSetKey],$row);
            }
        }
        // get next key for next resultset
        if(isset($resultSets[$resultSetsKey][count($resultSets)])) {
            $resultSetKey = $resultSets[$resultSetsKey][count($resultSets)];
        }
    // get next resultset
    } while(!is_null(sqlsrv_next_result($results)));

    sqlsrv_free_stmt($results);
    return $resultSets;
}

So my implementation works perfectly, does exactly what I want, lets me edit the procedures (add remove resultsets at will) and I only have to modify my php that uses the function, I never have to touch the function.

So my question: Is there a more elegant implementation to get the same result, ie, reference the resultsets by name set in first resultset, rather than referencing them by the order they are returned?

\$\endgroup\$
  • \$\begingroup\$ I'm not really sure what the code is trying to do, or what you are asking. That's probably not your fault, my brain hurts so its being rather dense. Anyways, here are some suggestions. Use empty() instead of comparing count() to zero. Declare $resultSetsKey before initializing $resultSets, then use $resultSetsKey instead of $row[ 'id' ] so that you use the same key throughout the application and don't cause confusion. Don't use array_push(), just append like so $resultSets[ $resultSetKey ] [] = $row. Assuming no other answers are forth coming I'll give this another crack later. \$\endgroup\$ – mseancole Jul 19 '12 at 16:27
  • \$\begingroup\$ @showerhead - thanks for the feedback. I'm not really asking a question as much as I'm wanting my code reviewed as that is what I thought was meant by codereview.stackexchange.com since there are always so many different ways to do the same thing, peer review can make code that works either easier to maintain or more reliable. \$\endgroup\$ – Robert Louis Murphy Jul 20 '12 at 18:23
2
\$\begingroup\$

In the way you formed your post it looks like you were asking for a better way to do what you are already doing, which it still apears that you are. That's fine, I just had no idea what you wanted from this code. I had to walk myself through your code step by step, so some of this is just going to be repeating what you've already said in an attempt to affirm it into my mind. I don't know why I was having difficulties before, I guess, as I said, I was just being dense.

First of all, your example function call does not call the same function as the one you provided, though this is most likely just a typo.

Alright, with you so far. You seem to be saying you don't like your current output and that you desire a ResultSet that uses associative indexing rather than numerical indexing, similar to:

array( [ "resultsets" ] => array(), [ "codes" ] => array(), [ "listings" ] => array(), [ "users" ] => array() )

However, the ResultSet I'm seeing does not have this problem. I do see you are using the $row[ 'index' ] as a key for some reason, maybe that is what you mean? You can remove those and just use automatically generated incrementals, which I'll show you below. Let's take a look at that function.

Immediately I see a global. These are bad. Avoid globals at all costs. Globals don't exist. Forget you ever heard of them. If you need a variable to exist outside of the function in which you are using it, have it be the return value. If you need it inside a new function, have it be one of the parameters required to run it. In this case, you probably defined $conn just before this function is called, just pass it as a second parameter. Or rather, pass it as the first and the SQL as the second, that way it makes sense when compared to the other functions. The best way to do this would actually be to use a class and class properties, however this is beyond the scope of this review. I'd recommend becoming a little more familar with the "Don't Repeat Yourself" and "Single Responsibility" Principles, DRY and SR respectively, before attempting to learn OOP. Though SR is not usually abbreviated I will do so to save time.

Your loop seems odd. I'm not familiar enough with SQLSRV to be 100% sure, but it looks like your code is somewhat redundant. The only reason you haven't noticed it is because the internal iterator is smart enough to know better than to reset itself. Your first loop says it will continue looping through the results while there is a next result in the array. Your second, internal loop to the first, says the same. So, because you set up your first loop as a do/while loop, it will immediately perform the first iteration without checking the condition in the first while. So, as the second while loop iterates over the same array its as if that first loop were never there. Once you exit the do statement the internal pointer on the $results array is already pointing to the end of the array, this means when it checks that while statement at the end of the do/while block it will see that its job is already done and proceed with the code. Unless I miss my guess completely, you should be able to remove the do/while loop and this should perform the same.

Now, inside your loop. This is where the DRY and SR Principles begin. SR states that a function/method/class should only be concerned with a SINGLE RESPONSIBILITY. In other words its specialized to perform one task. Usually this task is repeatable, which is where the the DRY principle comes in. DRY states that you should never repeat yourself. If you find that you are performing the same task, create a function/method/class to do that task for you. So, if you want to "fetch" or "get" something, a repeatable task, you create a function that will do just that. And if there are any other smaller tasks associated with doing this one task it should be delegated to another specialized function that does just that. So this one big function you provided could actually be broken up into a few separate, more specialized, functions. So how do we separate it?

Believe it or not, you've already got the ground work laid out for it. Because we are in a loop, anything we do can be considered a repeatable task. While this isn't the only type of "repeatable" task, it is the easiest to spot. Tasks are any group of related actions done to accomplish an end goal. In plain english we refer to a task such as, "do this". In programming we refer to them much the same, but with slightly differnt termanology, such as "if, else, for, while, do, etc...". Sounds pretty daunting, but it isn't. Not everything in an if statement needs to be separated into its own custom function. Usually its already done for us. For example, isset(), count(), and empty() are all predefined tasks that do just one thing. But sometimes we can create a larger task that uses these smaller tasks.

Hopefully with those hints you will be able to figure out how to break this up. I don't want to do all the work for you, else you'll never learn. Now, I will show you a modified if/else statement that shows how to remove those $row[ 'index' ] indices and replace them with auto-incrementing indices. I'll also make a few other improvements and try explaining them below. I don't know enough about your data structure to determine if the initial if/else statement needs to be changed, but I can work with the rest.

if( isset( $row[ 'id' ] ) {
    $resultSetsKey = $row[ 'id' ];
}
if( ! isset( $resultSets[ $resultSetsKey ] ) ) {
    $resultSets[ $resultSetsKey ] = array();
}

if(isset($row['index']) && isset($row['id']) && count($row) == 2) {
    $resultSets[ $resultSetsKey ] [] = $row['id'];
} else {
    $resultSets[ $resultSetKey ] [] = $row;
}

So, first thing you will notice, I moved the code used to set the $resultSetsKey and initial value of that array index, outside of the initial if/else statement. This is to reduce the amount of DRY violations. The new if/else statement is much smaller. Now all its concerned about is which part of the arrow to set to the $resultSets array.

I hope these points helped.

\$\endgroup\$
  • \$\begingroup\$ I wish I could +1 this more than once just for "Immediately I see a global. These are bad. Avoid globals at all costs. Globals don't exist. Forget you ever heard of them." :) \$\endgroup\$ – Corbin Jul 21 '12 at 4:21

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.