Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I've setup up a nice, little parent class that I can extend easily to do Http request to servers offering some sort of data with JSON/XML/whatever output.

You can see the init() function that is defined in the child class. The request URl builder is defined in the parent class, as the callback is.

The desired result is like the following:

// THE RESULT: (just an example - the class takes *any* sort of input)
http://example.com/HTTPservice?format=json&q=give_me_data&api_key=A0123456789Z

http://  example.com/HTTPservice ?       format=json&q=give_me_data&api_key=A0123456789Z
SCHEME   REQUEST URL             PREFIX  PREFIX_EACH/KEY/SEPARATOR/VALUE (repeating)

// Scheme
http://
// Request URI
example.com/HTTPservice
// Query Parts: Prefix - gets prepended to the result of http_builder :: get_query_parts()
?                    // $this->query_parts['prefix']
// Request Parts: 
format=json          // $this->request_parts['format] + $this->query_parts['separator'] + $this->request_parts['json']
&                    // $this->query_parts['prefix_each']
q=give_me_data       // $this->request_parts['q] + $this->query_parts['separator'] + $this->request_parts['give_me_data']
&                    // $this->query_parts['prefix_each']
api_key=A0123456789Z // $this->request_parts['api_key] + $this->query_parts['separator'] + $this->request_parts['A0123456789Z']

So it defines a scheme (http/https), appends the main URl to the API and then appends the query string, which gets build from a key/value array.

It is capable of adding a prefix and suffix to the complete string, as well as adding single key/value pairs with prefix/suffix for each pair. It also adds a separator on demand.

// Setup: extending class
class http_factory extends http_builder
{

public function init()
{
    $this->scheme        = 'http://';
    $this->request_uri   = "example.com/HTTPservice";
    $this->query_parts   = array(
         'prefix'      => '?'
        ,'suffix'      => ''
        ,'separator'   => '='
        ,'prefix_each' => '&'
        ,'suffix_each' => ''
    );
    $this->request_parts = array(
         'format'  => 'json'
        ,'q'       => 'give_me_data'
        ,'api_key' => 'A0123456789Z'
    );
}
} // END http_factory 

// The core class
class http_builder
{

// Builds the complete string
public function get_request_uri()
{
    // Prepare
    $parts = array_map( 'urlencode', $this->request_parts );
    $parts = array_map( 'htmlentities', $parts );

    $uri = sprintf(
        "%s%s%s%s%s"
        ,$this->scheme
        ,$this->request_uri
        ,$this->query_parts['prefix']
        ,implode( '', array_map(
             array( $this, 'get_query_parts' )
            ,array_keys( $this->request_parts )
            ,$this->request_parts 
         ) )
        ,$this->query_parts['suffix']
    );
}

// Callback fn to build a string from the key/value pair
public function get_query_parts( $key, $value )
{
    static $counter, $parts_count = 0;

    ! $parts_count AND $parts_count = count( $this->query_parts ); 
    $counter++;

    extract( $this->query_parts );
    return implode(
         ''
         // Allow single (value only) query parts without separator and prefix
        ,array(
             ( ! empty( $key ) AND 1 < $counter ) ? $prefix_each : ''
            ,$key
            ,! empty( $key ) ? $separator : ''
            ,$value
            ,! ( count( $this->query_parts ) === $counter ) ? $suffix_each : ''
         )
    );
}
} // END http_builder

Question How could I avoid all that crap with counting in the cb fn, without killing what's left from readability in the main request builder. Important is, that I need to have the prefix/suffix for each key/value pair. The first one must not have a prefix_each and the last one must not have a suffix_each.

I also appreciate every other comment. Criticism will be taken as positive in any way.

EDIT: I have to note, that I'm not using http_build_query(), as some APIs I'm dealing with, are using completely different strings, that don't use = as separator.

share|improve this question
add comment

1 Answer

I'm not sure I entirely understand what this is doing. But I'll give you my two cents.

If the scheme were HTTPS, would that instead append the $request_uri with "HTTPSservice?"? In which case you could do something like the following.

$this->scheme       = 'http';//changed for ease
$this->request_uri  = '://example.com/' . strtoupper( $this->scheme ) . 'service?';

I don't know about everyone else, but to me it seems odd to see commas on the newline rather than the preceding one when you are defining multiline arrays or parameters, but that may just be a point of preference.

I really want to bash that "q" parameter, but I've done it before too so I can't say much except that it should probably be more specific.

The sprintf() function is usually pretty slow from my understanding, so it would probably be better to just concatenate the strings together manually. I'll leave this to you to profile and determine.

What? What is this?

! $parts_count AND $parts_count = count( $this->query_parts ); 

No! Don't do this! Its just confusing and abuses short circuiting in the worst way. Just use an if statement, yes its two more lines, but this is the only clean way to check if a variable is set then give it a default value. As it was I was left scratching my head for a moment or two trying to figure out what the hell was going on. Or, since you've already set it to static, you can just give it that count value upon initializing it.

if( ! isset( $parts_count ) ) {
    $parts_count = count( $this->query_parts );
}

BTW: Use && instead of AND. Short reason: They aren't the same thing. Long Reason: AND and && have different precedences, therefore one supersedes the other so it could have unforeseen results. I can't give you a good example as I've never run across an issue with this before, however it is standard practice not to use this format unless specifically meant.

Instead of using those ternary statements inline, as you are doing, I would assign a variable to them so that it is easier to determine their purpose.

Also, don't create an array just to implode it, that's redundant. Just concatenate it. That's like saying, "I'd like a burger, hold the patty and bun." Sure you will get a salad, but its an odd way to go about it.

Might want to work on that legibility thing a little more before worrying about that counter, as I'm not sure what's going on anyways. But if I'm understanding this correctly, you can just shift and pop the array outside of the function to remove the first and last elements respectively and then append them manually outside of the function. The $request_parts array will no longer contain these values and it can be implode()'d like you are currently doing without change.

$keys   = array_keys( $this->request_parts );
$values = array_values( $this->request_values );

$firstKey   = array_shift( $keys );
$firstValue = array_shift( $values );

$lastKey   = array_pop( $keys );
$lastValue = array_pop( $values );

$newRequestValues = array_combine( $keys, $values );
share|improve this answer
    
Thanks for your answer. I'm leaving all the preference stuff out, as it's ... preference and I'm aware of anything you wrote (incl. && over and/AND). The point with your last argument is, that I'm using array_map for a reason: There can be lots of key/value pairs and the order inside the assoc array. And the order is undefined - so using array_pop/shift may a) bring the wrong value and b) would pop/shift it permanently off the array and therefore wouldn't be available for the next key/val pair anymore. Then there's also the issue that I avoid assigning var, that are used only once... –  kaiser Aug 6 '12 at 21:02
    
(...continue) ...as this would slow down stuff for no reason. The only reason why I'm assigning $uri is that in my "real" class, I log the output and return $uri after that. Else I'd return immediately. The actual problem, that I've is that I want to avoid all the static counting and comparing inside the callback. I already tried several things incl. work arounds for http_build_query(). But http_build_query() lacks the possibility to replace the = connector and add leading/trailing chars like [] or similar (some APIs use such things). –  kaiser Aug 6 '12 at 21:05
    
(...continue) It also lacks the main features I'm after: Adding a prefix that starts after the first key/value pair and adding a suffix that ends before the first key/value pair. For other things like the leading ?, I got prefix and for stuff like .json (that some APIs got), I got suffix. My problems are only the prefix_each/suffix_each and the counting that comes with it. Thanks. –  kaiser Aug 6 '12 at 21:08
    
The difference between && and AND is not a matter of preference. I said precedence. Sorry about final point, forgot I was looking at an associative array. You can still use suggested solution with array_keys() and array_values(), then combining with array_combine(). If this is not a feasible solution for you, you will have to deal with the counter as there is no other way. Except maybe with iterators, but I can't help you there. As far as order, that shouldn't matter, they all go to the same place, it only matters which one is first and gets the "?" instead of the "&". –  mseancole Aug 6 '12 at 22:03
    
Assigning variables for one time use should not hinder your program. If it does, then there is definitely something wrong. This is a simple task and should not cause any issues. If you are trying to optimize for performance, unnecessarily, then don't that's called premature optimization and makes horrible code. –  mseancole Aug 6 '12 at 22:03
show 3 more comments

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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