How to shorten this code?

fishbaitfood · September 3, 2011

Hey all,

I really like programming in PHP, but I'm still in the learning process.

And mostly, I'm not that good in making my code short.

This is now the case again.

I have this code which gets a lot of checkboxes, only checkboxes, out of a mysql database, for a planning project.

Now the code I have so far, retrieves the checkboxes, which are booleans in the database, and checks if they are True or False and sets the checkboxes respectively to checked="yes" or not.

This code works.

But I'm here to get tips on how to shorten my code, since it's very repetitive and thus long, which isn't quite efficient.

As I said, I'm still in learning process, so I hope you guys understand that I'm looking for help with this.

Thanks.

EDIT: Okay, I now suddenly see, that I can use elsif for True and False. This would be logical to do, and I can accomplish this easily. But it would be still very repetitive.

Here's the code:

<?php
				while ($row = mysql_fetch_array($result)){

					// UNCHECKED

					if ($row['fact-sent'] == false){
						$fact_sent = '<input type="checkbox" />';
					}

					if ($row['fact-payed'] == false){
						$fact_payed = '<input type="checkbox" />';
					}

					if ($row['domain'] == false){
						$domain = '<input type="checkbox" />';
					}

					if ($row['content'] == false){
						$content = '<input type="checkbox" />';
					}

					if ($row['design-sent'] == false){
						$design_sent = '<input type="checkbox" />';
					}

					if ($row['design-approved'] == false){
						$design_approved = '<input type="checkbox" />';
					}

					if ($row['design-sliced'] == false){
						$design_sliced = '<input type="checkbox" />';
					}

					if ($row['temp-website'] == false){
						$temp_website = '<input type="checkbox" />';
					}

					if ($row['temp-website-sent'] == false){
						$temp_website_sent = '<input type="checkbox" />';
					}

					if ($row['temp-website-approved'] == false){
						$temp_website_approved = '<input type="checkbox" />';
					}

					if ($row['cms'] == false){
						$cms = '<input type="checkbox" />';
					}

					if ($row['seo'] == false){
						$seo = '<input type="checkbox" />';
					}

					if ($row['analytics'] == false){
						$analytics = '<input type="checkbox" />';
					}

					if ($row['webmaster-tools'] == false){
						$webmaster_tools = '<input type="checkbox" />';
					}

					if ($row['website'] == false){
						$website = '<input type="checkbox" />';
					}

					if ($row['website-online'] == false){
						$website_online = '<input type="checkbox" />';
					}


					// CHECKED

					if ($row['fact-sent'] == true){
						$fact_sent = '<input type="checkbox" checked="yes" />';
					}

					if ($row['fact-payed'] == true){
						$fact_payed = '<input type="checkbox" checked="yes" />';
					}

					if ($row['domain'] == true){
						$domain = '<input type="checkbox" checked="yes" />';
					}

					if ($row['content'] == true){
						$content = '<input type="checkbox" checked="yes" />';
					}

					if ($row['design-sent'] == true){
						$design_sent = '<input type="checkbox" checked="yes" />';
					}

					if ($row['design-approved'] == true){
						$design_approved = '<input type="checkbox" checked="yes" />';
					}

					if ($row['design-sliced'] == true){
						$design_sliced = '<input type="checkbox" checked="yes" />';
					}

					if ($row['temp-website'] == true){
						$temp_website = '<input type="checkbox" checked="yes" />';
					}

					if ($row['temp-website-sent'] == true){
						$temp_website_sent = '<input type="checkbox" checked="yes" />';
					}

					if ($row['temp-website-approved'] == true){
						$temp_website_approved = '<input type="checkbox" checked="yes" />';
					}

					if ($row['cms'] == true){
						$cms = '<input type="checkbox" checked="yes" />';
					}

					if ($row['seo'] == true){
						$seo = '<input type="checkbox" checked="yes" />';
					}

					if ($row['analytics'] == true){
						$analytics = '<input type="checkbox" checked="yes" />';
					}

					if ($row['webmaster-tools'] == true){
						$webmaster_tools = '<input type="checkbox" checked="yes" />';
					}

					if ($row['website'] == true){
						$website = '<input type="checkbox" checked="yes" />';
					}

					if ($row['website-online'] == true){
						$website_online = '<input type="checkbox" checked="yes" />';
					}


					// OUTPUT

					echo '<tr><td class="customer">'.$row['project'].'</td><td class="customer">'.$row['customer'].'</td><td class="data">'.$fact_sent.'</td><td class="data">'.$fact_payed.'</td><td class="data">'.$domain.'</td><td class="data">'.$content.'</td><td class="data">'.$design_sent.'</td><td class="data">'.$design_approved.'</td><td class="data">'.$design_sliced.'</td><td class="data">'.$temp_website.'</td><td class="data">'.$temp_website_sent.'</td><td class="data">'.$temp_website_approved.'</td><td class="data">'.$cms.'</td><td class="data">'.$seo.'</td><td class="data">'.$analytics.'</td><td class="data">'.$webmaster_tools.'</td><td class="data">'.$website.'</td><td class="data">'.$website_online.'</td></tr>';
				}
			?>

fishbaitfood · September 3, 2011

Hmm, I can't seem to edit my original post anymore...

So here's the updated code, with if elseif.

<?php
while ($row = mysql_fetch_array($result)){

	if ($row['fact-sent'] == false){
		$fact_sent = '<input type="checkbox" />';
	}
	elseif ($row['fact-sent'] == true){
		$fact_sent = '<input type="checkbox" checked="yes" />';
	}

	if ($row['fact-payed'] == false){
		$fact_payed = '<input type="checkbox" />';
	}
	elseif ($row['fact-payed'] == true){
		$fact_payed = '<input type="checkbox" checked="yes" />';
	}

	if ($row['domain'] == false){
		$domain = '<input type="checkbox" />';
	}
	elseif ($row['domain'] == true){
		$domain = '<input type="checkbox" checked="yes" />';
	}

	if ($row['content'] == false){
		$content = '<input type="checkbox" />';
	}
	elseif ($row['content'] == true){
		$content = '<input type="checkbox" checked="yes" />';
	}

	if ($row['design-sent'] == false){
		$design_sent = '<input type="checkbox" />';
	}
	elseif ($row['design-sent'] == true){
		$design_sent = '<input type="checkbox" checked="yes" />';
	}

	if ($row['design-approved'] == false){
		$design_approved = '<input type="checkbox" />';
	}
	elseif ($row['design-approved'] == true){
		$design_approved = '<input type="checkbox" checked="yes" />';
	}

	if ($row['design-sliced'] == false){
		$design_sliced = '<input type="checkbox" />';
	}
	elseif ($row['design-sliced'] == true){
		$design_sliced = '<input type="checkbox" checked="yes" />';
	}

	if ($row['temp-website'] == false){
		$temp_website = '<input type="checkbox" />';
	}
	elseif ($row['temp-website'] == true){
		$temp_website = '<input type="checkbox" checked="yes" />';
	}

	if ($row['temp-website-sent'] == false){
		$temp_website_sent = '<input type="checkbox" />';
	}
	elseif ($row['temp-website-sent'] == true){
		$temp_website_sent = '<input type="checkbox" checked="yes" />';
	}

	if ($row['temp-website-approved'] == false){
		$temp_website_approved = '<input type="checkbox" />';
	}
	elseif ($row['temp-website-approved'] == true){
		$temp_website_approved = '<input type="checkbox" checked="yes" />';
	}

	if ($row['cms'] == false){
		$cms = '<input type="checkbox" />';
	}
	elseif ($row['cms'] == true){
		$cms = '<input type="checkbox" checked="yes" />';
	}

	if ($row['seo'] == false){
		$seo = '<input type="checkbox" />';
	}
	elseif ($row['seo'] == true){
		$seo = '<input type="checkbox" checked="yes" />';
	}

	if ($row['analytics'] == false){
		$analytics = '<input type="checkbox" />';
	}
	elseif ($row['analytics'] == true){
		$analytics = '<input type="checkbox" checked="yes" />';
	}

	if ($row['webmaster-tools'] == false){
		$webmaster_tools = '<input type="checkbox" />';
	}
	elseif ($row['webmaster-tools'] == true){
		$webmaster_tools = '<input type="checkbox" checked="yes" />';
	}

	if ($row['website'] == false){
		$website = '<input type="checkbox" />';
	}
	elseif ($row['website'] == true){
		$website = '<input type="checkbox" checked="yes" />';
	}

	if ($row['website-online'] == false){
		$website_online = '<input type="checkbox" />';
	}
	elseif ($row['website-online'] == true){
		$website_online = '<input type="checkbox" checked="yes" />';
	}


	// OUTPUT

	echo '<form name="checkbox_form"><tr><td class="customer">'.$row['project'].'</td><td class="customer">'.$row['customer'].'</td><td class="data">'.$fact_sent.'</td><td class="data">'.$fact_payed.'</td><td class="data">'.$domain.'</td><td class="data">'.$content.'</td><td class="data">'.$design_sent.'</td><td class="data">'.$design_approved.'</td><td class="data">'.$design_sliced.'</td><td class="data">'.$temp_website.'</td><td class="data">'.$temp_website_sent.'</td><td class="data">'.$temp_website_approved.'</td><td class="data">'.$cms.'</td><td class="data">'.$seo.'</td><td class="data">'.$analytics.'</td><td class="data">'.$webmaster_tools.'</td><td class="data">'.$website.'</td><td class="data">'.$website_online.'</td></tr>';
}
?>

jcbones · September 3, 2011

How about:

[/code]

$fact_sent = '<input type="checkbox" ' . ($row['fact-sent'] == true ? 'checked="checked"' : NULL) . ' />';

[/code]

ETC...

Psycho · September 3, 2011

I can't see what the purpose of those checkboxes are. You aren't even giving them a name so,they can't be used for editing the data.

But, let's assume they are only for display. My first recommendation would be to write your code in a manner that is easily readable. Putting all that code into one echo statement is not easily readable and will be just as unreadable in the HTML source created.

Another tip[, it is not necessary to have a condition such as

if($foo == true) {

Just use

if($foo) {

This is just a rough example based upon what you are currently doing, but I have a feeling if I saw all of your code and had a better idea of what you are trying to accomplish I would probably do something very different.

<?php

    $checkboxFields = array(
        'fact-sent', 'fact-payed', 'domain', 'content','design-sent', 'design-approved',
        'design-sliced', 'temp-website', 'temp-website-sent', 'temp-website-approved',
        'cms', 'seo', 'analytics', 'webmaster-tools', 'website', 'website-online');
    
while ($row = mysql_fetch_array($result))
    {
        $formHTML  = "<td class=\"customer\">{$row['project']}</td>\n";
        $formHTML .= "<td class=\"customer\">{$row['customer']}</td>\n";
        
	foreach($checkboxFields as $fieldName)
        {
            $checked = ($row[$fieldName]) ? ' checked="checked"' : '';
            $formHTML .= "<td class=\"data\"><input type=\"checkbox\"{$checked} /></td>\n";
        }
        
        // OUTPUT
    echo "<form name=\"checkbox_form\">\n";
    echo "<tr>\n";
        echo $formHTML;
    	echo "</tr>\n";
    }

?>

PFMaBiSmAd · September 3, 2011

LOL, virtually identical to what mjdamato posted ^^^

<?php
$checkboxes = array('fact-sent','fact-payed','domain','content','design-sent','design-approved',
'design-sliced','temp-website','temp-website-sent','temp-website-approved','cms','seo','analytics',
'webmaster-tools','website','website-online'); // list of fields that are checkboxes and the order to present them in the form

while ($row = mysql_fetch_array($result)){
	echo "<form name='checkbox_form'><tr><td class='customer'>{$row['project']}</td><td class='customer'>{$row['customer']}</td>";
	foreach($checkboxes as $key){
		echo "<td class='data'><input type='checkbox'" . ($row[$key] == true ? " checked='checked'" : NULL) . " /></td>";
	}
	echo "</tr>";
}
?>

fishbaitfood · September 4, 2011

Thanks all, exactly what I needed!

This clears up a lot.

Also many thanks for a better markup. :shy: I'm actually very strict about that too.

One more question:

Are the curly brackets alternatives of '.' for putting variables in html code?

Nevermind, found it.

Thanks again for taking the time.

fishbaitfood · September 4, 2011

Hi all,

Sorry to bother again, but I thought making a new topic about this would be kinda silly.

So about the checkboxes.. As I've said, I want to update them through boolean values in the database.

Each record has 16 checkboxes/booleans.

Now you would have guessed, I'm having trouble updating them independantly.

What I've tried:

- One big form to update all records. (would be easy for posting the form to the process php page, but then I would have duplicate checkbox names)

- Different forms for each record, but then I would have trouble with posting/identifying each form.

What would be the easiest way to solve this?

This has to be the trickiest thing I ever tried to make with php...

I'd greatly appreciate if you'd help me on this. Thank you.

What I have so far, using one big form:

<form name="checkbox_form">
<?php
$checkboxFields = array(
	'fact-sent', 'fact-payed', 'domain', 'content','design-sent', 'design-approved',
	'design-sliced', 'temp-website', 'temp-website-sent', 'temp-website-approved',
	'cms', 'seo', 'analytics', 'webmaster-tools', 'website', 'website-online');

while ($row = mysql_fetch_array($result)) {
	$formHTML  = "<td class=\"customer\">{$row['project']}</td>\n";
	$formHTML .= "<td class=\"customer\">{$row['customer']}</td>\n";

	foreach($checkboxFields as $fieldName) {
		$checked = ($row[$fieldName]) ? ' checked="checked"' : '';
		$formHTML .= "<td class=\"data\"><input type=\"checkbox\" name=\"{$fieldName}\" value=\"{$row['project']}\"{$checked} /></td>\n";
	}

	// OUTPUT
	echo "<tr>\n";
	echo $formHTML;
	echo "</tr>\n";
}
?>
</form>

fishbaitfood · September 4, 2011

Maybe it's better to get a form for each row, and when a checkbox is clicked, use jQuery .parent to get the respective form, and post it using jQuery?

I think this would be the better solution, since it was my initial plan to post the form(s) with jQuery, so I won't have a page refresh.

This is possible, right?

Psycho · September 4, 2011

You should never make the functionality of your application rely upon JavaScript. JS should only be added to provide additional functionality on top of something that will work without it.

Since you plan to use these field to update records, then you need to give them names as I said before. You should give them the same name as the database field to be consistent.

If you want one form to update many records, the solution is simply. Name the fields so that they are arrays with the index of each field the record id.

<input type="checkbox" name="fieldname[recordID]" />

If you want a separate form for each record, then just add a hidden field into each for to identify the record id.

having said that, here are good reasons NOT to allow updating of all records in one form. Specifically performance and scalability concerns. Depending on the amount of records and the user base those concerns can be mitigated. My personally preference is to give a list of records with a button/link on each record to go to an edit page.

fishbaitfood · September 4, 2011

Thanks mjdamato, I'll look into that definately.

But I got this script now, which SHOULD work.

It gathers the value (id) of my checkboxes.

And checks if they are ticked or not, respectively 1 or 0.

This way I can update my boolean values in my database.

Weird thing is, I also get the values of the unchecked checkboxes, how's that possible? I was just echoing out all post data, to see this.

This is the error I get:

Query failed: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '-sent = 0 WHERE project =' at line 1

Code:

<?php

$checkboxFields = array(
	'fact-sent', 'fact-payed', 'domain', 'content','design-sent', 'design-approved',
	'design-sliced', 'temp-website', 'temp-website-sent', 'temp-website-approved',
	'cms', 'seo', 'analytics', 'webmaster-tools', 'website', 'website-online');

include_once('connection.inc.php');
$con = mysql_connect(MYSQL_SERVER, MYSQL_USERNAME, MYSQL_PASSWORD) or die ("Connection failed: " . mysql_error());
mysql_select_db("cre8") or die ("Could not open database: " . mysql_error());

foreach ($checkboxFields as $fieldName) {
	if (isset($_POST[$fieldName])) {
		$id = $_POST[$fieldName];
	}
	$val = (int) isset($_POST[$fieldName]);  // will be 0 or 1

	mysql_query("UPDATE checklist SET $fieldName = $val WHERE project = $id;") or die ("Query failed: " . mysql_error());
	header('Location: index.php');
}

?>

jcbones · September 4, 2011

Put your fieldnames in backticks (`), cause the hyphen seems to be causing an issue.

fishbaitfood · September 4, 2011

Thanks for clearing that out!

But I still get this error (at the end of query):

Query failed: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '' at line 1

Okay, I've found the problem..

When checking a checkbox, the script works.

But when unchecking a checkbox, I get this error. This has to do with the unchecked value, I guess...? :s

UPDATE:

I only get the error when unchecking the first field... The rest works top-notch!

So what could be causing this first field to get an error when unchecking?

jcbones · September 4, 2011

You need to echo the query out, and make sure everything is in it that needs to be. That is why you should store the query string in a variable, so you can echo the string out in case of an error.

<?php
$sql = "UPDATE checklist SET $fieldName = $val WHERE project = $id";
mysql_query($sql) or trigger_error("Query failed: <br /> $sql <br /> ERROR: <br />" . mysql_error());

fishbaitfood · September 4, 2011

Now it's really buggy, and outputs no error whatsoever.

Some checkboxes save their state, others don't and get resetted to their previous state...

It behaves like it can only select/deselect a few, at first, and then it doesn't work anymore. Really strange.

jcbones · September 4, 2011

Did you move your header call outside of the foreach loop, the table may not be finished updating before you select the rows.

Psycho · September 4, 2011

I see a lot of problems with what you are trying to do. And, to be honest, I just don't have the time to debug it today. But, here are some comments

      if (isset($_POST[$fieldName])) {
         $id = $_POST[$fieldName];
      }

You are setting $id to the VALUE of the field. You need to be setting it to the INDEX of the field (if a mult-record form) or to some hidden field with the ID if a single record form. Based upon the code you have it looks to be a single record form.

echo yur query to the page

fishbaitfood · September 4, 2011

Hm, the checkbox value contains the ID, of that record, which is thus posted to the script.

Isn't it redundant to have another hidden field for that?

Or am I missing your point? If so, I apologize.

Btw, jcbones, thanks for the tip. Didn't pay attention to that, which is quite obvious. But it still behaves buggy.

Psycho · September 4, 2011

Hm, the checkbox value contains the ID, of that record, which is thus posted to the script.

Isn't it redundant to have another hidden field for that?

Or am I missing your point? If so, I apologize.

Btw, jcbones, thanks for the tip. Didn't pay attention to that, which is quite obvious. But it still behaves buggy.

First of all, you need to state exactly what you are doing now. There was some back and forth previously over whether you would allow editing of multiple records or only one. I don't know what your decision is.

If you are allowing of only one record then it is not necessary (or even a good idea) to put the record id as the value of the checkbox since, as you know, only checked field are passed in the post data. Therefore, if no checkboxes were selected then you would have no reference for the record ID. Second, it looks like you were trying to run an update query for each field individually. That is a terrible idea. You should never run queries in a loop - they are a resource/performance hog. Jsut get the updated values for ALL fields and run one update query for the record.

fishbaitfood · September 4, 2011

Indeed, sorry about that.

I'm going to have a form for each row/record, to reduce unnessary processing.

And yes, quite obvious actually, about the ID. So I'll fix that with a hidden field.

About the query.. I'd store ALL the field values in an array then?

Psycho · September 5, 2011

About the query.. I'd store ALL the field values in an array then?

For creating the query to update the records I would use the exact same array of field names that you used to create the checkboxes. That is why I suggested using the field names from the database as the names for the checkbox fields.

The code would look something like this

if(isset($_POST['id']))
{
    $project_id = (int) $_POST['id'];

    //Loop through all checkboxes to generate update code
    $updateValues = array();
    foreach($checkboxFields as $field)
    {
        $updateValues[] = "{$field} = " . ((isset($_POST[$field])) ? '1' : '0');
    }

//Create the query

$query = "UPDATE checklist SET " . implode(', ', $updateValues) . " WHERE project = {$project_id}";

}

[/code]

fishbaitfood · September 5, 2011

Hey mjdamato,

Thank you, that code makes much sense!

But when executed, it stays on the process page, with the values in the url (?project=20110001&fact-sent=on&fact-payed=on&....). Even with header('Location: index.php');

It seems to me, the query isn't executed, which it should. I don't see the reason why it shouldn't.

fishbaitfood · September 5, 2011

EDIT:

The code should be working perfectly, but like I said above, the query isn't executed, and I remain on the process page with the values in the url.

Sign In

How to shorten this code?

Recommended Posts

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Join the conversation

Important Information