SitePoint Sponsor

User Tag List

Results 1 to 7 of 7
  1. #1
    SitePoint Addict Divisive Cotton's Avatar
    Join Date
    Jun 2008
    Location
    Andy lives in London, UK
    Posts
    393
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)

    Taking a value from a loop in a class method

    Here's my problem that I've been stumped on for the last few hours

    I'm creating a class to create html forms with: https://gist.github.com/1966505

    In the create_fields() method I loop through a multidimensional array to create the form fields

    This is what I end up with:

    PHP Code:
     $input '<tr>';
     
    $input .= "<th><label for=\"$id\">$desc</label></th>";
     
    $input .= '<td><'.$form.' ></td>';
     
    $input .= '</tr>';

    var_dump($input);

    string '<tr><th><label for="feed-name">The name of the feed:</label></th><td><input type="text"  name="affiliate_hoover_plugin_options[feedName]"  id="feed-name"  class="regular-text code "  maxlength="200"  value="" ></td></tr>' (length=220)

    string '<tr><th><label for="url-name">The URL of the feed:</label></th><td><input type="text"  name="affiliate_hoover_plugin_options[urlName]"  id="url-name"  class="regular-text code "  maxlength="300"  value="" ></td></tr>' (length=216
    Great

    So if I was to echo it, no problems

    However, when I try to take the returned values and use them in another method they disappear.

    It's not a scope issue it is how I am collecting the data from the loop that is the root of the problem

    I'm sure there must be an easy solution that has passed me by
    Let everyday be Christmas

  2. #2
    Avid Logophile silver trophy
    ParkinT's Avatar
    Join Date
    May 2006
    Location
    Central Florida
    Posts
    2,337
    Mentioned
    192 Post(s)
    Tagged
    4 Thread(s)
    I see that in your major switch statement (in create_fields()) you are using "echo". That will output the value but not persist it in any way.
    So there is nothing to return; which I can't see a return statement anywhere in that method.
    Don't be yourself. Be someone a little nicer. -Mignon McLaughlin, journalist and author (1913-1983)


    Git is for EVERYONE
    Literally, the best app for readers.
    Make Your P@ssw0rd Secure
    Leveraging SubDomains

  3. #3
    SitePoint Addict Divisive Cotton's Avatar
    Join Date
    Jun 2008
    Location
    Andy lives in London, UK
    Posts
    393
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)

    Unhappy

    If I was to use return instead of echo then I would get is null:

    PHP Code:
    var_dump($cont->individual_fields("text""feedName""feed-name"null"The name of the feed:""200""YES"));

    <
    pre xmlns="http://www.w3.org/1999/xhtml" dir="ltr" class="xdebug-var-dump"><font color="#3465a4">null</font>
    </
    pre


    Using echo returns the HTML

    There is clearly something fundamentally stupidly wrong with how I am looping through the values

    Damn it

    This is the first method:

    PHP Code:
     public function individual_fields($type$name$id$class null$desc$maxlength null$value null,
            
    $select null) {

    // validation remved

            // create an array out of the parameter values
            
    $fields_essentials = array(
                
    'type' => $type,
                
    'name' => $name,
                
    'id' => $id,
                
    'desc' => $desc,
                
    'class' => $class,
                
    'maxlength' => $maxlength,
                
    'value' => $value,
                
    'select' => $select);

            
    // call the create_fields() methods
            
    $this->create_fields($fields_essentials);

        } 
    Which then passes the array to the second method which creates the individual HTML fields

    PHP Code:
    private function create_fields($fields_essentials) {

            
    extract($this->config_settings());

            foreach (
    $fields_essentials as $key => $value) {

                if (
    $key === "type") {

                    switch (
    $value) {

                        case 
    'text':
                            
    // text area

                            
    $form 'input type="text" ';

                            foreach (
    $fields_essentials as $key => $value) {

                                if (
    $key === "name") {
                                    
    $form .= " name=\"{$option_name}[{$value}]\" ";
                                    
    $name $value;
                                }
                                if (
    $key === "id") {
                                    (
    $value !== null) ? $form .= " id=\"{$value}\" " null;
                                    
    $id $value;
                                }
                                if (
    $key === "desc") {
                                    
    $desc $value;
                                }
                                if (
    $key === "class") {

                                    if (
    $value !== null) {
                                        
    $form .= " class=\"regular-text code {$value}\" ";
                                    } else {
                                        
    $form .= " class=\"regular-text code \" ";
                                    }

                                }
                                if (
    $key === "maxlength") {
                                    (
    $value !== null) ? $form .= " maxlength=\"{$value}\" " null;
                                }
                                if (
    $key === "value") {

                                    if (
    $value !== null && $value != "YES") {
                                        
    $form .= " value=\"{$value}\" ";
                                    }
                                    if (
    $value == "YES") {

                                        
    $form .= ' value="';
                                        
    $form .= isset($_POST[$option_name[$name]]) ? print $_POST[$option_name[$name]] : null;
                                        
    $form .= '"';

                                    }

                                } 
    // end if $key

                            
    // end foreach

                            
    $input '<tr>';
                            
    $input .= "<th><label for=\"$id\">$desc</label></th>";
                            
    $input .= '<td><'.$form.' ></td>';
                            
    $input .= '</tr>';


                       
                          return 
    $input;   break;
                          
    // I'M THE RETURN VALUE DO SOMETHING WITH ME!


                          
                        
    case 'hidden':
                        case 
    'button':
                        case 
    'image':
                        case 
    'password':
                        case 
    'reset':
                        case 
    'submit':
                            
    // error here - there form inputs are not cattered for
                            
    die("You cannot use the individual_fields() method to create inputs for $key");
                            break;
                        default:
                            
    // error message here
                            
    die("Make sure the input type in the individual_fields() method is correct");
                            break;

                    } 
    // end switch statement

                
    // end fi

            
    // end foreach

        

    Let everyday be Christmas

  4. #4
    Non-Member bronze trophy
    Join Date
    Nov 2009
    Location
    Keene, NH
    Posts
    3,760
    Mentioned
    23 Post(s)
    Tagged
    0 Thread(s)
    Well, your first code seems gibberish to me, not sure what that even is... your later post and what's on github?

    if you have to === a string compare, there's something horrifically wrong with your strings... == is just fine.

    But more important, you're looping for no good reason -- if all you want to operate on is 'type', and type is a associative key, what are you even looping for?!?

    Code:
    	private function create_fields($fields_essentials) { 
    	
    		extract($this->config_settings());
    		
    		switch ($fields_essentials['type']) {
    			
    			case 'text':
    There's no reason to be looping through values you aren't even using! Your inner loop makes even LESS sense... wait, what's the contents of $field_essentials again?

    You've got other oddball logic failings and strange code in there.... like the multiple echo statements to do the job of one, slow double quotes doing the job of singles, (which are those $vars inside $form supposed to be evaluated or not?!? Looks like you've got some form of templating stuff on top with those curly brackets in there)... You also seem to be declaring a whole slew of local variables for no good reason -- and I'm not even convinced you need a loop.

    Also since extract is a COPY, and not a reference, assigning values to the vars extracted by it really isn't gonna do a whole lot for you.

    Even the 'null' checks are odd, since you should probably be using isempty since it will actually return set and non-null from foreach.

    ... and that's before we even talk about 'what the devil is this table code doing in here?'.

    Are you planning on doing anything with those extracted vars? Just seems really wierd... if you want to actually save values to them, you'll need them as a reference, NOT as extracted copies.

    I'll look deeper at the code, but I suspect you've got over-thought things a bit, and at the same time left out some important safety checks (like isset and empty).

  5. #5
    Non-Member bronze trophy
    Join Date
    Nov 2009
    Location
    Keene, NH
    Posts
    3,760
    Mentioned
    23 Post(s)
    Tagged
    0 Thread(s)
    Reading deeper, I think I see what it is you were trying to do... I might be way off base in following it, but I suspect this is what you meant to do:

    Code:
    	private function create_fields($fields_essentials) { 
    	
    		extract($this->config_settings());
    		
    		if (isset($fields_essentials['type']) {
    		
    			switch ($fields_essentials['type']) {
    			
    				case 'text': 
    				
    					return '
    						<label for="'.$field_essentials['id'].'">
    							',$field_essentials['desc'],'
    						</label>
    						<input type="text"',(
    							isset($field_essentials['name']) && !empty($field_essentials['name'])) ?
    							' name="'.$option_name.'['.$field_essentials['name'].']"' :
    							''
    						),(
    							(isset($field_essentials['id']) && !empty($field_essentials['id'])) ?
    							' id="'.$field_essentials['id'].'"' :
    							''
    						),' class="regular-text code',(
    							(isset($field_essentials['class']) && !empty($field_essentials['class'])) ?
    							' '.$field_essentials['class'] :
    							''
    						),'"'(
    							(isset($field_essentials['maxlength']) && !empty($field_essentials['maxlength'])) ?
    							' maxlength= '.$field_essentials['maxlength'] :
    							''
    						),(
    							(isset($field_essentials['value']) && !empty($field_essentials['value'])) ?
    							' value="'.(
    								$field_essentials['value']=='YES' ?
    								(isset($_POST[$option_name[$name]]) ? $_POST[$option_name[$name]] : '') :
    								$field_essentials['value']
    							) :
    							''
    						),' />
    						<br />';
    
    					case 'hidden': 
    					case 'button': 
    					case 'image': 
    					case 'password': 
    					case 'reset': 
    					case 'submit': 
    						// error here - there form inputs are not cattered for 
    						die("You cannot use the individual_fields() method to create inputs for $key"); 
    					break; 
    					
    					default: 
    						// error message here 
    						die("Make sure the input type in the individual_fields() method is correct"); 
    					break; 
    
    			} // END switch ($fields_essentials['type'])
    
    		} // END if isset -- you may want to add an ELSE here!
    
    	} // END method create_fields
    Naturally swinging an axe at the table for nothing stuff. Untested, probably a typo or two, but reduces the code logic and should run many times faster.

  6. #6
    SitePoint Addict Divisive Cotton's Avatar
    Join Date
    Jun 2008
    Location
    Andy lives in London, UK
    Posts
    393
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Thanks a lot for your "help"

    I'll come back and deal with your "gibberish" insult later but I haven't got time now

    But I will state is using double quotes instead of single quotes or using echo does not impact on the performance of a PHP script in any significant way, and anyway that was NOT what I was asking about OKAY!!
    Let everyday be Christmas

  7. #7
    Non-Member bronze trophy
    Join Date
    Nov 2009
    Location
    Keene, NH
    Posts
    3,760
    Mentioned
    23 Post(s)
    Tagged
    0 Thread(s)
    Don't take it personally man -- I really wasn't able to follow your original posts meaning; call it a comprehension issue, call it an incomplete picture -- but for me it was gibberish -- and besides, you have to break things down so you can build them up; there are several logic failings in the code and sloppy code practices that could be cleaned up; any ONE of them could be causing issues... You don't want help fixing them, what are you even asking for help FOR?!?

    What do I mean by logic failings?

    Code:
            foreach ($fields_essentials as $key => $value) { 
    
                if ($key === "type") { 
    
                    switch ($value) { 
    
                        case 'text': 
                            // text area 
    
                            $form = 'input type="text" '; 
    
                            foreach ($fields_essentials as $key => $value) {
    First foreach is for 'nothing' -- since you're searching for only ONE valid condition on a KEY. Iterating through an associative array for a specific KEY doesn't make any sense -- waste of code, waste of processing time. If you know the key you want to process, don't loop blindly through an associative array -- that's what associative arrays are there to stop you from having to do -- Just check if it's set, then use it!

    Second foreach would be causing 'current pointer' issues. Arrays have a 'pointer' of sorts to the 'current' record.... reset puts it at the start, end puts it at the end, next moves to the next one... and foreach... uses it... so by doing a foreach on the same array twice, you're short-circuiting the outer one.... Now you're looking for a key value, so that's 'ok' in a manner of speaking, but it's wasted logic...

    Because again, it's an associative array -- you don't loop through an associative array and run case on it's key -- you just access it by the key.

    Which is how my example code turned your two loop and case statement with multiple variable overheads, into two IF and a return... dropping it from 3.37k of code with multiple variables you didn't even need - to 1.9k

    As to the other things like quotes -- maybe it's the machine language coder in me, maybe it's having learned coding back when stuff like that mattered -- but to me that's just sloppy coding. It might only be 1-3% if that -- but when it's the difference between hitting the shift key or not hitting the shift key... why waste the extra keystroke AND make sloppy code?

    But from your reaction, apparently you don't actually want help or to make the code better or even functional. Gotcha.


Bookmarks

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •