Jump to content

How to shorten this code?

fishbaitfood

By fishbaitfood
in PHP Coding Help

 Share

Recommended Posts

fishbaitfood Member

fishbaitfood

Posted
Posted

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>';
				}
			?>

Link to comment
https://forums.phpfreaks.com/topic/246357-how-to-shorten-this-code/
Share on other sites
fishbaitfood Member

fishbaitfood

Posted
  • Author
Posted

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>';
}
?>

Psycho Prolific Member

Psycho

Posted
Posted

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 Advanced Member

PFMaBiSmAd

Posted
Posted

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 Member

fishbaitfood

Posted
  • Author
Posted

Thanks all, exactly what I needed!  :D

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.  :D

 

Thanks again for taking the time.

fishbaitfood Member

fishbaitfood

Posted
  • Author
Posted

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 Member

fishbaitfood

Posted
  • Author
Posted

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 Prolific Member

Psycho

Posted
Posted

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 Member

fishbaitfood

Posted
  • Author
Posted

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');
}

?>

fishbaitfood Member

fishbaitfood

Posted
  • Author
Posted

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 Newbie

jcbones

Posted
Posted

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 Member

fishbaitfood

Posted
  • Author
Posted

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.

Psycho Prolific Member

Psycho

Posted
Posted

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 Member

fishbaitfood

Posted
  • Author
Posted

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 Prolific Member

Psycho

Posted
Posted

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 Member

fishbaitfood

Posted
  • Author
Posted

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 Prolific Member

Psycho

Posted
Posted

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 Member

fishbaitfood

Posted
  • Author
Posted

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.

This thread is more than a year old. Please don't revive it unless you have something important to add.

Join the conversation

You can post now and register later. If you have an account, sign in now to post with your account.

Guest
Reply to this topic...

×   Pasted as rich text.   Restore formatting

  Only 75 emoji are allowed.

×   Your link has been automatically embedded.   Display as a link instead

×   Your previous content has been restored.   Clear editor

×   You cannot paste images directly. Upload or insert images from URL.

×
 Share
Go to topic listing
×
×
  • Create New...

Important Information

We have placed cookies on your device to help make this website better. You can adjust your cookie settings, otherwise we'll assume you're okay to continue.