Filtering by value X from array Y with a fallback

Question

I wrote a function that solve a special problem. But one of my 'special hardcore software engineers' told me that this is a bad solution - but he will not give me a better solution. So I ask you: What could I do better? (This is more an academic exercise than a bottleneck in my software.)

I have an array $tags of {n} objects. Each object has an attribute called type. Further I have an array $types of {n} integers (tag types).

First value (index 0) of $types has highest priority, second value (index 1) of $types second highest priority and so on.

If this function finds a higher priority tag type during its iteration thru $tags it discards the lower priority tags from $returnArray.

$count just tell this function how many tags of same tag type are allowed. So if there are three tags of highest priority tag type it will return first occurrence.

Expected result 1:

    $tags = [
          $obj1, //type 50
          $obj2, //type 1
          $obj3 //type 25
        ]

    $types = [1, 50]

filterTagsByType($tags, $types) //returns [$obj2] because it has type 1

Expected result 2:

    $tags = [
          $obj1, //type 50
          $obj3 //type 25
        ]

    $types = [1, 50]

filterTagsByType($tags, $types) //returns [$obj1] because it has type 50

This is my function - I hope comments will clarify everything.

/**
 * This function filters and returns one or many tags of a defined tag type.
 * Define tag types in $types - first tag type (index 0) has highest priority. 
 * This function returns only highest
 * tag type found and will ignore lower priority tag types.
 *
 * @param Tag[] $tags Array of tag models
 * @param int[] $types Array of tag types, first (index 0) has highest priority
 * @param int $count Max tags of one type to be returned
 * @return Tag[]
 */
public function filterTagsByType($tags, $types, $count = 1)
{
  $priority = 1000;
  $returnTags = [];

  foreach ($tags as $tag)
  {
      // get priority level of this tag, 0 is highest.
      $newPriority = array_search($tag->type, $types);

      // check if this tag has same priority like a tag before
      //if so, check if there is still room for another tag in $returnTags
      if ($priority == $newPriority && count($returnTags) < $count)
      {
          $returnTags[] = $tag;
      }
      // check if new priority is less (higher priority) than last tags (that
      // was added to return array) priority
      elseif ($newPriority < $priority)
      {
          // clear and add new highest priority tag to return array
          // it won't overwrite same priority tags because this case would
          // caught by first if.
          $returnTags = [$tag];
          //set new priority
          $priority = $newPriority;
      }

      //break this foreach as soon as highest priority tag typ was found and in
      // return array is no room anymore.
      if (0 == $priority && count($returnTags) == $count)
      {
          break;
      }
  }

  return $returnTags;
}

Answer 1

Edit:

You code is over complex knowing there are some really good built in php functions to filter an array.

I think that is where the 'special hardcore dev' started disliking your code. Here an example that uses an anonymous function and array_filter. It also does the same as in the other answer. Loop over the $types instead of the tags. I think it is safe to say that there will be more tags then types.

function filterTagsByType($tags, $types, $count = 1)
{
    $tagFilter = function($type) {
        return function($tag) use($type) {
            return $tag->type == $type;
        };
    };

    foreach ($types as $type) {
        $filteredTags = array_filter($tags, $tagFilter($type));
        if ( count($filteredTags) > 0 ) {
            return $filteredTags;
        }
    }

    return array();
}

You should ofcourse still add the count check. this is easy with array_slice:

return array_slice($filteredTags, 0, $count);

Not sure how this is performance wise, but it shouldn't be an issue.

Answer 2

There are a few things I would have done differently, I'll say those before showing my own solution:

• What's this magic number 1000? What does priority 1000 mean? Why is it hardcoded?
• You should be using type hints wherever possible (You can typehint an array).
• Instead of iterating all of the $tags array, and then discarding the elements which have lower priority, iterate the types array, and find matching tags. This way you don't have to iterate the entire array.

My take:

<?php

function filterTagsByTypes(array $tags, array $types, $count = 1) {
    $returnTags = [];
    sort($types); //Sort from low to high => high priority to low priority
    usort($tags, function($a, $b) {
        if ($a->type == $b->type) {
            return 0;
        }
        return $a->type > $b->type ? 1 : -1;
    }); //Sort tags array from low type to high type.
    foreach ($types as $type) {
        foreach ($tags as $tag) {
            //In this case, since both arrays are sorted, the priority
            //you are looking for will not arrive.
            if ($tag->type > $type) {
                break; //So break out and continue to the next type.
            }
            if ($tag->type == $type) {
                $returnTags[] = $tag;
            }
            if (count($returnTags) >= $count) { //End it now!
                return $returnTags;
            }
        }
    }
    return $returnTags; //Which will be either empty, or contain less than $count elements.
}

// Usage:

class Tag {
    public $type;
    function __construct($type) {
        $this->type = $type;
    }
}

$tags = [
    new Tag(50),
    new Tag(25)
];

$types = [1, 50];

var_dump(filterTagsByTypes($tags, $types));