Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have some PHP code where I use Variable functions to call the right function. I need to build a chart array (for example), and the chart array that comes out has a fixed format. But the data that goes into the chart or table comes from varying places in the underlying objects.

An example:

private function buildChartGroups(PropelCollection $groups, $propertyForDisplay) {
    $groupDisplayFunction = "getSum$propertyForDisplay";

    $chart = array();
    foreach ($groups as $group) {
            $chart[] = array('name' => $group->getName(), 
                             'y' => $group->$groupDisplayFunction()
                       );
        }
    }
    return $chart;
}

and then the function gets called something like:

$result = $this->buildChartGroups($groups, "Total");
...elsewhere...
$result = $this->buildChartGroups($groups, "Count");

where $groups is an array of Group objects...

class Group {
   private $name;
   private $sumTotal;
   private $sumCount;

   public function getName() {
       return $this->name;
   }

   public function getSumTotal() {
        return $this->sumTotal;  
   }

   public function getSumCount() {
        return $this->sumCount;
   }
}

This works fine and lets me re-use buildChartGroups() which I would otherwise not be able to do. But using Variable functions and passing function names as strings always seems a little dirty, even though I'm sure it has its place.

Is there a way to refactor this or some pattern I can use to avoid them?

share|improve this question
    
Welcome to Code Review! To make life easier for reviewers, please add sufficient context to your question. The more you tell us about what your actual code does and what the purpose of doing that is, the easier it will be for reviewers to help you. Unfortunately, it's a bit hard to review code like this because the code you're showing is looking more like example code rather than code that you're actually using in a real project. –  Simon André Forsberg Apr 10 at 22:24
    
Got it, thanks. Adding more of the real code. –  matt Apr 10 at 22:49
1  
Hi Matt, I'm Mat, or rather, Mat's Mug. If you have a moment while our php reviewers take a stab at your post, feel free to explore our meta site and to feel at home and browse the site's questions - you'd be surprised at how many things you could find in someone else's code, even if you're not a professional or whatever. If you enjoy answering StackOverflow questions, you'll enjoy answering CodeReview questions even more! Always feel free to come say hi in The 2nd Monitor, too! –  Mat's Mug Apr 10 at 23:42
add comment

2 Answers 2

up vote 4 down vote accepted

There are several other ways to solve this, but which one I'd go with depends a lot on the Group class.

  • Do you plan to have more than a handful of properties to choose from?
  • Do you have control over the Group implementation?
  • Is this property-choosing behavior needed anywhere else?

Here's an example that assumes the answers to the above are all "no": use constants to pick the property and provide one-liner public methods to avoid exposing them to callers:

const TOTAL = 1;
const COUNT = 2;

public function buildChartGroupTotals(PropelCollection $groups) {
    return $this->buildChartGroups($groups, self::TOTAL);
}

public function buildChartGroupCounts(PropelCollection $groups) {
    return $this->buildChartGroups($groups, self::COUNT);
}

private function buildChartGroups(PropelCollection $groups, $property) {
    return array_map(function ($group) {
        switch ($property) {
            case self::TOTAL:
                $value = $group->getSumTotal();
                break;
            case self::COUNT:
                $value = $group->getSumCount();
                break;
            default:
                throw new InvalidArgumentException("Invalid property");
        }
        return array(
            'name' => $group->getName(), 
            'y' => $value,
        );
    }, $groups);
}

If you have control over the Group implementation, I would prefer to move the constants and switch into it and add getSumProperty($property).

class Group {
    const TOTAL = 1;
    const COUNT = 2;
    ...
    public function getSumProperty($property) {
        switch ($property) {
            case self::TOTAL:
                return $this->sumTotal;
            case self::COUNT:
                return $this->sumCount;
            default:
                throw new InvalidArgumentException("Invalid property");
        }
    }
}

Now you can pass one of the constants to buildChartGroups which will forward it to getSumProperty and greatly simplify the code at the cost of exposing those constants. You could still provide the property-specific one-liners, though.

private function buildChartGroups(PropelCollection $groups, $property) {
    return array_map(function ($group) {
        return array(
            'name' => $group->getName(), 
            'y' => $group->getSumProperty($property),
        );
    }, $groups);
}
share|improve this answer
    
I do have control over the Group class, and this fits in well with that implementation. I can actually pass that $property down to the classes that the Group actually holds and it makes for much more readable code. And this will make it much easier to find where the getSumProperty methods are used, as opposed to my Variable functions solution. –  matt Apr 11 at 15:55
add comment

I agree, it does feel dirty to compute the function name like that.

A typical way to do dynamic dispatching is using polymorphism. You could formalize the mechanism for determining which function to call by making some property-extracting objects:

abstract class PropertyExtractor {
    public abstract function getProperty($group);
}

class SumTotalExtractor extends PropertyExtractor {
    public function getProperty($group) {
        return $group->getSumTotal();
    }
}

class SumCountExtractor extends PropertyExtractor {
    public function getProperty($group) {
        return $group->getSumCount();
    }
}

Your buildChartGroups() would take one of those extractors as the second parameter:

private function buildChartGroups(PropelCollection $groups, $propertyExtractor) {
    $chart = array();
    foreach ($groups as $group) {
        $chart[] = array('name' => $group->getName(), 
                         'y' => $propertyExtractor->getProperty($group));
    }
    return $chart;
}

Here it is in use:

$result = $this->buildChartGroups($groups, new SumTotalExtractor());
share|improve this answer
    
This is nice and clean! I'm accepting the other answer because it fit a little better with the rest of my code but both are great ideas, thanks! –  matt Apr 11 at 15:52
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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