A better way to do this

[razorsese]

My code works perfect for the job needed to be done

But  I feel is very sloppy

Is there any chance to optimize it?!

 

It's getting from a post all comment's id and variable 0 or 1 depending on input and then assign the ids whit value 1 to another variable then delete

the comment whit the specific id;

Initially all ids have the value 0;

 

$id = array();
$ids = array();
if(isset($_POST['submit']))
{

	$data = array();
$data = $_POST;

$i=0;
foreach($data as $key=>$value)
{
	$id[$i]=$value;
	$i++;

}

$j=0;

for($d= 1; $d<$i;$d++)
{
	if($id[$d] == 1)
	{
		$j++;
		$ids[$j] = $id[$d+1];
	}
}


$ctx= new Comment;

for($i=1;$i<=$j;$i++)
{
	$ctx->commentdelete($ids[$i]);
}
header('Location:admin.php');
}

[Muddy_Funster]

Haven't tested it (obviously), but this could be slightly leaner

if(isset($_POST['submit']))
{
foreach ($_POST as $k => $v){
if($v == '1'){
$id[] = $k;
}
}
$ctx = new comment;
foreach ($id as $x){
$ctx->commentdelete($x);
}
header('Location:admin.php');
}

[razorsese]

Var dump look like this:

 

array

  'submit' => string 'submit' (length=6)

  1 => string '0' (length=1)

  'h1' => string '57/' (length=3)

  2 => string '0' (length=1)

  'h2' => string '56/' (length=3)

  3 => string '0' (length=1)

  'h3' => string '52/' (length=3)

  4 => string '0' (length=1)

  'h4' => string '51/' (length=3)

  5 => string '0' (length=1)

  'h5' => string '47/' (length=3)

  6 => string '0' (length=1)

  'h6' => string '46/' (length=3)

[Muddy_Funster]

and what does your var_dump $ids look like in comparison?

[razorsese]

My var_dump whit  2 selected comments to delete :

 

array

  1 => string '57/' (length=3)

  2 => string '56/' (length=3)

 

your's: var_dump($id);

 

array

  0 => int 1

  1 => int 2

[Muddy_Funster]

ok, can I see a sample form your POST array as well, this isn't as obvious as it first looked

[razorsese]

Using  whit none comment selected to delete

 

foreach ($_POST as $k => $v){

echo $k."~".$v."<br>";
}

output:

submit~submit

1~0

h1~57/

2~0

h2~56/

3~0

h3~52/

4~0

h4~51/

5~0

h5~47/

6~0

h6~46/

 

and var_dump($_POST);

 

array

  'submit' => string 'submit' (length=6)

  1 => string '0' (length=1)

  'h1' => string '57/' (length=3)

  2 => string '0' (length=1)

  'h2' => string '56/' (length=3)

  3 => string '0' (length=1)

  'h3' => string '52/' (length=3)

  4 => string '0' (length=1)

  'h4' => string '51/' (length=3)

  5 => string '0' (length=1)

  'h5' => string '47/' (length=3)

  6 => string '0' (length=1)

  'h6' => string '46/' (length=3)

 

[Muddy_Funster]

right, making sense of it now, this should look better:

if(isset($_POST['submit']))
{
foreach ($_POST as $k => $v){
if($v == '1'){
$id[] = $_POST[$k+1];
}
}
$ctx = new comment;
foreach ($id as $x){
$ctx->commentdelete($x);
}
header('Location:admin.php');
}

[razorsese]

Still not working 

Also if i try to select all comments and use var_dump like in the below code appear

 

Notice: Undefined offset: 7 on: ($id[] = $_POST[$k+1]; )

array

  0 => string '1' (length=1)

  1 => string '1' (length=1)

  2 => string '1' (length=1)

  3 => string '1' (length=1)

  4 => string '1' (length=1)

  5 => null

 

if(isset($_POST['submit']))
{

foreach ($_POST as $k => $v){
if($v == '1'){
$id[] = $_POST[$k+1];
}
}


var_dump($id);


}

[xyph]

<?php

$_POST = array(
'submit'=>'submit',
'1'=>'0','h1'=>'57',
'2'=>'0','h2'=>'56',
'3'=>'1','h3'=>'52',
'4'=>'0','h4'=>'51',
'5'=>'1','h5'=>'47',
'6'=>'0','h6'=>'46',
'7'=>'1','h8'=>'53'
);

$to_delete = array();
foreach( $_POST as $k => $v ) {
// Make sure key is numbers only, and a matching 'h#' exists, and $v == 1
if( ctype_digit((string)$k) && isset($_POST['h'.$k]) && $v == 1 ) {
	$to_delete[] = $_POST['h'.$k];
}
}

print_r( $to_delete );

?>

 

This verifies the key follows the pattern we want, and makes sure a matching 'h#' key exists before checking if the value is one. If all of those conditions are met, it adds the value from the 'h#' key into the $to_delete array.

[razorsese]

Thanks xyph.

Works like a charm

 

 

Yep 

[xyph]

No problem, do you understand the code, and the logic being performed?

[razorsese]

Yep

But I didn't know of ctype_digit until now

[xyph]

ctype_digit is great, but it REQUIRES a string. That's why I used typecasting - it forces the variable to be a string. See below

 

<?php

$var = 1;

var_dump(ctype_digit(1) );
// returns boolean false

var_dump(ctype_digit('1') );
// returns boolean true

var_dump(ctype_digit($var) );
// returns boolean false

var_dump(ctype_digit((string)$var) );
// returns boolean true

?>

 

Hope this helps

[Muddy_Funster]

yeah, I didn't know about the ctype_ either, deffinate bookmark on that one.

 

as an afterthought, I did get my version of the code to work , but as it's not got the checks in place that xyph's does, and is therefor not as robust, I'll just leave it.

[Psycho]

I'll provide some suggestions not related to efficient code.

 

1. Create descriptive variable names such that someone can understand what the variable is for. On the first two lines you have the variables $id and $ids. Ok, they both hold IDs, but what are their purpose. Is one holding ids to be deleted?

 

2. Put code within appropriate conditions. With the same two variable definitions above you have them before the first if() condition, but it looks as if those vars are only used within that condition. SO, define them within there as well. Not so much of an issue with defining a couple variables, but I've seen instances where someone runs several queries that aren't even needed.

 

3. Comments! Add comments to your code. It will help you and anyone reading your code.

 

4. You don't need to initiate a variable as an array if you are simply going to define the value using another array.

    //$data = array();
   $data = $_POST;

 

5. This is unnecessary

   $i=0;
   foreach($data as $key=>$value)
   {
      $id[$i]=$value;
      $i++;
   }

Since you are simply assigning the variables and wanting a numerically based index, you could just do this

$id = array_values($data);

 

6. This is very important. It appears you are determining what records to delete based upon an arbitrary index. You need to pass the EXACT ids of the records you want deleted and not try to artificially calculate them based upon their position. This whole process would be much easier if the form was created in a different manner.

[razorsese]

Thanks for the tips.

I will definitely to follow them