Tidy up method

Hi,

Have a method here which I think is more complicated than it should be, can anyone see a way it can be optimised?

  /* have a fixed set of places from 1 - 98, return array - used places omitting passed to it  */

    public function getFieldIds(array $editimage=null)
    {
        $params = array(
            'fields' => array('position'),
        );
        $op = $this->Image->find('all', $params);  //gets a list used spaces
        //remove current item if an edit
        if ($editimage) {
            foreach ($op as $key => $value) {
                if ($value['Image']['position'] == $editimage['Image']['position'])
                    unset($op[$key]);
            }
        }
        //pop into a simple array
        $existing = array();
        foreach ($op as $item) {
            $existing[] = $item['Image']['position'];
        }
        $full = range(1, 98);
        $listitems = (array_diff($full, $existing));
        $new = array();
        foreach ($listitems as $key => $value) {
            $new[$value] = $value;
        }
        return $new;
    }

Open in new window


using cakephp
LVL 13
darren-w-Asked:
Who is Participating?

[Product update] Infrastructure Analysis Tool is now available with Business Accounts.Learn More

x
I wear a lot of hats...

"The solutions and answers provided on Experts Exchange have been extremely helpful to me over the last few years. I wear a lot of hats - Developer, Database Administrator, Help Desk, etc., so I know a lot of things but not a lot about one thing. Experts Exchange gives me answers from people who do know a lot about one thing, in a easy to use platform." -Todd S.

crazedsanityCommented:
There are a lot of unknowns here that would keep us from being able to help.  Namely, the call to an internal object named "Image" (see line #8 in posted code).  

Are you looking for this code to be optimized for readability or functionality?
darren-w-Author Commented:
Image is the DB model, it just returns an array
Slick812Commented:
greetings darren-w- , , I looked at your function, And I think I see what it is suppose to do, however, it seems to return an Inverse (opposite numbers than the $op  array position had). It seems to do several operations to try and eliminate the $op positions from the $full array. I have made a function which works for me, but I really have NO IDEA what the $editimage  array has in it or the $op array, I had to guess at this whole thing, not easy to do.

my code that does the work is ->

foreach ($op as $key => $value) {
      if ($value['Image']['position'] != $test1) unset($new[$value['Image']['position']]);
      }

which is a single loop, and I guess is more efficient.
also in the function parameter you have an array  $editimage
function getFieldIds(array $editimage=null)

and Yet in your function you do NOT use any of that array as an array, you just use a single integer value from
$editimage['Image']['position']

and yet the PHP system must copy the entire array to this function, It would be more efficient if you used a single integer as the Parameter ->
function getFieldIds($test1 = -1)
then only that integer is copied to the function and that's all that is needed in the function.

I hope this works for you, ask questions if you need more information or corrections.
function getFieldIds(array $editimage=null) {
$op = array(array('Image' => array('position' => 10)),array('Image'=>array('position' => 20)),
	array('Image'=>array('position' => 30)),array('Image'=>array('position' => 5)),
	array('Image'=>array('position' => 15)),array('Image'=>array('position' => 1)));
// the $op array above is just my guess at whatever may returned from your $this->Image->find( )

if (isset($editimage['Image']['position'])) $test1 = $editimage['Image']['position']; else $test1 = -1;
$new = range(0, 98);// start at Zero so all Keys and values will match
foreach ($op as $key => $value) {
	if ($value['Image']['position'] != $test1) unset($new[$value['Image']['position']]);
	}
unset($new[0]);
return $new;
}


// below is the Test code to see what output I get
$editimage = array('Image' => array('position' => 15));// leaves the 15 in the output
$reAry = getFieldIds($editimage);
echo '<br />';
foreach ($reAry as $value) echo $value,', ';

Open in new window

darren-w-Author Commented:
Ok, sussed it out;

 private function getFieldIds(array $editimage=null) {
        $params = array(
            'fields' => array('position'),
            'conditions' => array('position !=' => $editimage['Image']['position'])
        );
        $existing = $this->Image->find('list', $params);
        $listitems = (array_diff(range(1, 98), $existing));
        $new = array();
        foreach ($listitems as $key => $value) {
            $new[$value] = $value;
        }
        return $new;
    }

Open in new window


The value needs to start at 1 because they go into a drop down list, What I have done is omit the passed id from the sql call if it exists, also cakephp will return a list view this removes lines 17 - 20 in original.

Experts Exchange Solution brought to you by

Your issues matter to us.

Facing a tech roadblock? Get the help and guidance you need from experienced professionals who care. Ask your question anytime, anywhere, with no hassle.

Start your 7-day free trial
darren-w-Author Commented:
Thank for taking a look
It's more than this solution.Get answers and train to solve all your tech problems - anytime, anywhere.Try it for free Edge Out The Competitionfor your dream job with proven skills and certifications.Get started today Stand Outas the employee with proven skills.Start learning today for free Move Your Career Forwardwith certification training in the latest technologies.Start your trial today
Web Frameworks

From novice to tech pro — start learning today.