5
\$\begingroup\$

I have a class Comparator that does several operations (union, intersection, etc) on 'sets'. In this specific case each 'set' is a 3 dimensional array.

in order to do these operations, I have to use a nested foreach to loop through each dimension before getting to the data I need to compare against.

I figure there are two ways to implement this. One is to simply do the loop in each operation, like so:

class Comparator  {

    //union is the set of unique elements from both setA and setB
    public function union()  {
        //initially add $setB to $union resultset.
        $union = self::$setB;

        foreach (self::$setA as $k => $v)  {
            foreach ($v as $k2 => $v2)  {
                foreach ($v2 as $k3 => $element)  {
                    if (!isset($union[$k][$k2][$k3]))  {
                        //add $element to $union resultset if it does not already exist in $union.
                    }
                }
            }
        }

        return $union
    }

    //Set of elements in setB that are not in setA.
    public function rightComplement()  {
        $complement = self::$setB

        foreach (self::$setA as $k => $v)  {
            foreach ($v as $k2 => $v2)  {
                foreach ($v2 as $k3 => $element)  {
                    //if $element is in setA, remove it from $complement.
                    unset($complement[$k][$k2][$k3]);
                }
            }
        }

        return $complement;
    }
}

Problem is I have about 9 other operations that all operate off of an identical loop... just a different thing is done inside the final foreach.

The other way is bit drier, but I feel somewhat confusing:

class Comparator  {

    //union is the set of unique elements from both setA and setB
    public function union($args = null)  {
        //initially add $setB to $union resultset.
        $union = self::$setB;

        if ($args === null)  {
            $this->iterate(self::$setA, __FUNCTION__);
        }  else  {
            if (!isset($union[$args['k']][$args['k2']][$args['k3']]))  {
                //add $element to $union resultset if it does not already exist in $union.
            }
        }

        return $union
    }

    //Set of elements in setB that are not in setA.
    public function rightComplement($args = null)  {
        $complement = self::$setB

        if ($args === null)  {
            $this->iterate(self::$setA, __FUNCTION__);
        }  else  {
            unset ($complement[$args['k']][$args['k2']][$args['k3']]);
        }

        return $complement;
    }


    public function iterate($obj, $function)  {
        foreach($obj as $k => $v)  {
            foreach ($v as $k2 => $v2)  {
                foreach ($v2 as $k3 => $element)  {
                    //call the method named in $function, pass back the keys and final value.
                    $this->$function(array(
                        'k1' => $k,
                        'k2' => $k2,
                        'k3' => $k3,
                        'element' => $element
                    ));
                }
            }
        }
    }
}

So my question is which (if either) is the better approach? Is the second approach too DRY? Too confusing? I'm open to other approaches as well.

\$\endgroup\$

1 Answer 1

3
\$\begingroup\$

I don't know if something can be too DRY. Too abstract perhaps.

    public function rightComplement($args = null)  {
        $complement = self::$setB

        if ($args === null)  {
            $this->iterate(self::$setA, __FUNCTION__);
        }  else  {
            unset ($complement[$args['k']][$args['k2']][$args['k3']]);
        }

PHP has closures. You don't need this pattern. Consider

    public function rightComplement()  {
        $complement = self::$setB;

        $this->iterate(self::$setA, function($i, $j, $k, $element) use ($complement) {
            unset($complement[$i][$j][$k]);
        });

Now rather than having two ways to call the function, one of which indirectly calls the other, there is one way. We use the closure to provide the single element behavior instead.

I like this better than either of your choices, as it is more direct about what it does than the second. And it is DRYer than the first version. We call iterate with a closure that handles each element.

You don't complete the implementation for the other function, so I didn't try to duplicate it.

\$\endgroup\$
1
  • \$\begingroup\$ Hadn't thought of closures... +1 \$\endgroup\$
    – mcmurphy
    Aug 21, 2017 at 18:04

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.