3

What would be considered "best practice" in this case. I've got a class that's gathering remote resources, and it looks a bit like this:

class Gather {
    public function getAll($locations) {
        $results = array('All','My','Results');
        return $results;
    }
}

My question is, would it be considered a best practice to return the results, or to assign them as a property? ie.

// This
$results = $gatherer->getAll();
// vs This
$gatherer->getAll(); // now $gatherer->results can be used

Its quite likely I'm just overthinking this, but I've got no formal training and I'm wondering if there's "more correct" way of doing something like this.

2 Answers 2

7

Without question, the first one ($results = $gatherer->getAll()) is preferred. The reason is that the relationship between the value and where it comes from is explicit. In the second case it's not clear to the reader that $gatherer->results is populated by a call to getAll(). Maybe it came from some other call, or it's always there, or set by an outside caller.

This also makes it easier for the reader to trace through to understand the call. When getResults() returns the value it's clear that they reader should read the implementation of getResults() to see where it came from.

2
  • I agree. The alternative is only preferrable if the results 'belong' to the Gather instance, and the Gather object will perform subsequent work with them. Commented Feb 3, 2011 at 16:21
  • 1+ You could mitigate this a little by : $gatherer->loadAll(); $gatherer->results; But, it's only in VERY rare cases when you'd have to use this form. Commented Feb 3, 2011 at 16:22
1

I've been wrestling with this same issue lately. In the second version

$gatherer->getAll(); // now $gatherer->results can be used

I would change your naming convention to

$gatherer->initResults();

Then it's obvious that results are a property of $gatherer. You could even define $gatherer->initResults() like so:

public function initResults() {
    $this->results = $this->getAll();
}

public function getAll() {
    // do whatever to get results
}

So then, you can use either form.

Sorry, I know this is more of a comment then an answer but it was so code heavy it was practically unreadable as a comment.

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.