How to make my code less redundant

Kadin · September 30, 2010, 8:27pm

I wrote this code to delete all 3 images and remove the image names from the database. Then I realized what if I only want to delete one or two images. Therefore, I modified the code. It works, however it does not update the database plus the code can be made more efficient but I do not know how to go about doing that. Can someone show me? Thanks.

if (isset($_POST['submitted']) && 
		($_POST['radio00'] == 'delete') ||
		($_POST['radio01'] == 'delete') ||
		($_POST['radio02'] == 'delete')) {
			
		$query = "SELECT image0, image1, image2 FROM table WHERE  status = '$status' ";
		if($result = $db->query( $query )){
			$row = $result->fetch_array();
		}		
		
		if($_POST['radio00'] == 'delete') {unlink( UPLOAD_DIR.$row['image0'] ); $filename0 = '';}
		if($_POST['radio01'] == 'delete') {unlink( UPLOAD_DIR.$row['image1'] ); $filename1 = '';}
		if($_POST['radio02'] == 'delete') {unlink( UPLOAD_DIR.$row['image2'] ); $filename2 = '';}
		
		if($filename0 = '') {$query = "UPDATE table SET image0 = '$filename0' WHERE status = '$status' ";}
		if( !$db->query( $query ) )	$errString .= "<p>error with database.</p>";
		
		if($filename1 = '') {$query = "UPDATE table SET image1 = '$filename1' WHERE status = '$status' ";}
		if( !$db->query( $query ) )	$errString .= "<p>error with database.</p>";
		
		if($filename2 = '') {$query = "UPDATE table SET image2 = '$filename2' WHERE status = '$status' ";}
		if( !$db->query( $query ) )	$errString .= "<p>error with database.</p>";
	
}

Kadin · October 1, 2010, 8:03pm

Yes. The code I posted is the actual code I am using. I will try to integrate $imageValue into this. I think I see where your going with this.

Thanks.

Kadin · September 30, 2010, 11:00pm

Thank you both.

I see the way you handle this as an array without getting stopped by unlinks() no array policy. I am learning something. I will try to rewrite my code using your way of thinking.

Merging into a single query is something I would like to learn. Are you saying there is a way to write a query to update a table with only one change even when there are multiple situations or image names involved?

Raffles · September 30, 2010, 10:41pm

You could also merge it all into a single query rather than do a query for each iteration of the loop. This would be beneficial especially if you have lots of images to deal with.

ahundiak · September 30, 2010, 10:37pm


function unlink_image($image)
{
  unlink( UPLOAD_DIR.$image );
  
  // Add your sql stuff here

}

$delete_images = $_POST['delete_images'];
foreach($delete_images as $image)
{  
   unlink_image($image);
}

You really don’t need a function at all though it doesn’t hurt. Just put your code inside of the foreach block.

Kadin · September 30, 2010, 10:24pm

Thanks. I will remember that as I continue working on this.

I am using arrays in other parts of my script, but it’s my understanding unlink() will not except an array. That’s why I split it up.

So far my php books are not helping me outline this into a function. Can someone give me a kind of template like hint of a function structure or idea of how to delete one or more of these image files and update the database accordingly.

In other words where to begin.

Thanks.

ahundiak · September 30, 2010, 10:04pm

Also, take a look at how to use arrays in posting. Instead of radio00,radio01 etc you will want something like:
<input type=“radio” name=“delete_images” value=“image1” />
<input type=“radio” name=“delete_images” value=“image2” />
<input type=“radio” name=“delete_images” value=“image3” />

After posting you will end up with an array in $_POST[‘delete_images’] which in turn will contain the values of all the selected images to delete.

Kadin · October 1, 2010, 7:18pm

Thanks for your response.

I am sorry I don’t understand your suggestion.

This is supposed to be designed to select and delete either one, two or all three images from the directory and delete the image or images names from the table.

When I use

unlink(UPLOAD_DIR . $row[0]);

All images delete.

Kadin · October 1, 2010, 8:45pm

It works. I can delete individual images.

It did not work at first, but then I saw what I was doing wrong. I did not include unlink() in the if ($imageValue == ‘delete’) { unlink() } statement. Plus I did not know what exactly to put in the unlink(). $row, $row, $row[0], $row[1], $row[image0], etc. I ran some tests and $row[0] seems to bingo.

I guess that means all the image names are located horizontally in the table on a row and there is only one row. row[0].

Thanks for your help, the both of you. I will try to delete the names from the table next.

if (isset($_POST['submitted']) && 
        ($image = $_POST['image'])) {
        
        foreach($image as $imageKey => $imageValue) {

            if ($imageValue == 'delete') {
                $query = "SELECT $imageKey FROM table WHERE  status = '$status' ";
                if($result = $db->query( $query )){
                    $row = $result->fetch_array();
                }
            
                print_r($row);
                unlink( UPLOAD_DIR.$row[0] );
            }
        }
    }

Jaanboy · October 1, 2010, 6:56pm

You need to access a specific key of the array, i.e:

unlink(UPLOAD_DIR . $row[0]);

Also, don’t forget to sanitise those variables before running them through the database.

Kadin · October 1, 2010, 7:14am

I ran into a problem using this,

because my web page has all three of these per image, so that’s nine radio buttons.

If I use name=“delete_images” in place of the delete name=radio00, radio01 and radio02 buttons on all three images then the radio keep buttons don’t uncheck, so I end up with more than one button checked at the same time.

That seems to prevent this from becoming an array.

Any help on making this transition would be appreciated. Here is what I have so far. Thanks.

if (isset($_POST['submitted']) &&
		isset($_POST['delete_images'])) {
	
		$delete_images = $_POST['delete_images'];

		foreach($delete_images as $image) {  
			$query = "SELECT $image FROM table WHERE  status = '$status' ";
			if($result = $db->query( $query )){
				$row = $result->fetch_array();
			}
			unlink( UPLOAD_DIR.$row );
			$query = "UPDATE table SET image0 = '$row' WHERE status = '$status' ";
		} 
	}

ahundiak · October 1, 2010, 2:58pm

Radio buttons are grouped by name so you need something like:


<input type='radio' name='images[1]' value='keep' checked='checked'/>
<input type='radio' name='images[1]' value='delete' />
<input type='radio' name='images[1]' value='replace' />

<input type='radio' name='images[2]' value='keep' checked='checked'/>
<input type='radio' name='images[2]' value='delete' />
<input type='radio' name='images[2]' value='replace' />

<input type='radio' name='images[3]' value='keep' checked='checked'/>
<input type='radio' name='images[3]' value='delete' />
<input type='radio' name='images[3]' value='replace' />

$images = $_POST['images'];
foreach($images as $imageKey => $imageValue)
{
  // Use $imageKey to determine which image to process
  // Use $imageValue to determine the process
}

ahundiak · October 1, 2010, 7:44pm

Is the code you posted the actual code you are using? I don’t see where you are using $imageValue at? You should have something like:


foreach($image as $imageKey => $imageValue)
{
  if ($imageValue == 'delete')
  {
      // Do the query followed by the unlink command
  }
}

Kadin · October 1, 2010, 6:40pm

I put this together the best way I know how and ran a series of tests to understand it better. I think the code can almost unlink() an image, but I still get this error message. I think my use of $row in unlink() is not correct. Does someone know what I’m doing wrong? Thanks.

My table is set up as image0, image1, image2, status. status is like an id number.

if (isset($_POST['submitted']) && 
		($image = $_POST['image'])) {
		
		foreach($image as $imageKey => $imageValue) {
			// Use $imageKey to determine which image to process
			// Use $imageValue to determine the process
			$query = "SELECT $imageKey FROM table WHERE  status = '$status' ";
			if($result = $db->query( $query )){
				$row = $result->fetch_array();
			}
			print_r($row);
			unlink( UPLOAD_DIR.$row );
		}
	}

Array ( [0] => red.jpg [image0] => red.jpg )
Warning: unlink(upload_test/Array) [function.unlink]: No such file or directory in /home/public_html/test.php on line 109

Array ( [0] => green.jpg [image1] => green.jpg )
Warning: unlink(upload_test/Array) [function.unlink]: No such file or directory in /home/public_html/test.php on line 109

Array ( [0] => blue.jpg [image2] => blue.jpg )
Warning: unlink(upload_test/Array) [function.unlink]: No such file or directory in /home/public_html/test.php on line 109

ahundiak · September 30, 2010, 11:34pm

Sure. You can update multiple columns in one swoop. Take a look at some of the mysql tutorials.

But your table actually has columns called image0,image1 etc. I think you will end up with a table called images with one row per image where you store the file name.

But take one small step at a time.

Kadin · September 30, 2010, 9:21pm

Thanks for your response.

I am fairly new to php and I thought a function or something is how someone more experienced than I would approach this problem, but I don’t know how to begin to make a function out of this. Any hints would be appreciated. Mean while I will read through some of my php books about functions. Thanks.

tmapm · September 30, 2010, 9:15pm

Why don’t you write a function? Pass in your POST radio value and the image field it should affect.

Topic		Replies	Views
Possible to delete and move in same foreach block? PHP	4	415	October 5, 2010
How to update multiple rows with an array PHP	9	6576	October 23, 2019
Unlink files from server PHP	15	2532	March 27, 2010
How to upload multiple image in one record database SQL? PHP scripts	23	22307	October 29, 2015
Unlink image and delete from mysql PHP	11	20842	October 8, 2014

How to make my code less redundant

Related topics