1
\$\begingroup\$

I've created some controller for handling clicked links statistics. Does this class meet Single Responsibility Principle?

class StatisticsController
{
    protected $statisticsQuery;

    public function __construct(StatisticsQuery $statisticsQuery)
    {
        $this->statisticsQuery = $statisticsQuery;
    }

    public function recordClickedLink(array $request)
    {
        (...)
        if ($this->canRecordClick()) {
            // record clicked link
        }
        (...)
    }

    public function getStatistics(array $request)
    {
        (...)
        $someRequestParam = $request['id'];
        $statistics = $this->statisticsQuery->get($someRequestParam);
        (...)
    }

    protected function canRecordClick()
    {
        // returns true of false
    }
}

If has two public methods. The first is responsible for saving clicked link to a database by delegating it to a model class. The second public method is responsible for getting statistics and it is the only method which use $statisticsQuery object passed to a class' constructor. Is it OK that recordClickedLink method doesn't need constructor's argument?

I wonder also if it is OK that my class contain canRecordClick method with logic for checking if link click can be saved during particular request.

If this class doesn't meet SRP, how can I refactor it?

\$\endgroup\$
2
\$\begingroup\$

As far as I can see in the code example you've provided, it seems that the getStatistics and recordClickedLink methods are independent of each other. So it seems intuitive to have in this case two controllers: a StatisticsController and a something like a RecordController where the two methods recordClickedLink and canRecordClick live. In that way the StatisticsController is responsible for statisics and the RecordController for the record.

It is ok that the class contains canRecordClick, but protected means that any class extending the StatisticsController will be able to call this method too. If it will only be used in the StatisticsController, private would be better.

\$\endgroup\$
4
  • \$\begingroup\$ Do you think that constructor's argument can be considered when telling if this class applies SRP? I wonder if argument or arguments passed to constructor should be obligatorily used by all class' public methods. \$\endgroup\$ – Szymon Czembor Mar 30 '20 at 11:47
  • \$\begingroup\$ @SzymonCzembor The argument of a constructor or properties of a class can give a good indication but it is not a strict rule that each method should use all the properties of a class (there are probably some who disagree). Determining what falls under a single responsibility of a class can sometimes be difficult to determine and sometimes you also have to take into consideration how easy some code is to understand vs adding a new class. \$\endgroup\$ – pepijno Mar 30 '20 at 12:02
  • \$\begingroup\$ And what do you think about this? If I have two classes (StatisticsController and RecordController) and then I need to change the way I record a link click (for example altering database table) in RecordController then I have to change also StatisticsController because data for calculating statistics come from the altered place. Does it imply that this two methods should be in one class, doesn't it? \$\endgroup\$ – Szymon Czembor Mar 30 '20 at 12:18
  • 1
    \$\begingroup\$ If that is the case you could extract the code which alters and reads from the database to a separate repository class and pass that repository to both the StatisticsController and RecordController. Then when you change the way you record a link you only have to change the repository. \$\endgroup\$ – pepijno Mar 30 '20 at 13:16

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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