Functionality critical to test should not be locked away in private methods

This is part of the Semicolon&Sons Code Diary - consisting of lessons learned on the job. You're in the architecture category.

Last Updated: 2021-05-15

When building the reverse auction for project_s, I basically looped through the results of a complex query chooseAdvisors(n) and sent an email to each of these advisors. The worst nightmare bug was that every one in the database would get an email - i.e. that the n argument would be ignored.

The code was written as follows

// This is the method that the job executer triggers
public function handle()
    $advisors = $this->chooseAdvisors(10);
    foreach($advisors as $advisor) {

private function chooseAdvisors($numberToContact) { }
private function contactAdvisor($advisor) { }

This is a design I regret. For confidence, I wanted to see if chooseAdvisors was returning the right number of entries, depending on the argument given. Unfortunately, as a private method of a job, I could not test it easily (either in unit tests or in the production environment). Therefore I had reduced confidence.

In future I would have made it a public method, placing it on another class if needs be, in order to get a clean and sensible interface.