search

KnpLabs / KnpMenuBundle

Object Oriented menus for your Symfony project.
http://knplabs.com
MIT License
1.4k stars 203 forks source link

increasing the code quality #451

Closed sewelseb closed 2 years ago

sewelseb commented 2 years ago

I tried it as a training for myself in refactoring. don't hesitate to reject, but please provide me feedbacks (if you have time)

derrabus commented 2 years ago

I don't think that we should merge this. The current code is perfectly readable and I find splitting it into tiny methods not really helpful.

sewelseb commented 2 years ago

ok, no issue. As I have said, feel free to reject it.

Just to explain why I like small functions: I like to have functions that are shorter than 5 lines of code. The reason is that most of the time if it is longer, it breaks the single responsability principle. When I (a new developper) entered the code, it was difficult for me to understand it.

Anyway, it was a pleasure to read your comments and feedbacks.

derrabus commented 2 years ago

The reason is that most of the time if it is longer, it breaks the single responsability principle.

This package and especially the class you're fixing here is pure integration code. It's glue between a framework and a library. By design, such a class violates the SRP: You will need to change it whenever the integrated library adds, removes or changes functionality that this package is supposed to integrate. And splitting this class into microscopic methods does not change that: it's still a class with multiple resposibilities. And that's fine.

sewelseb commented 2 years ago

The reason is that most of the time if it is longer, it breaks the single responsability principle.

This package and especially the class you're fixing here is pure integration code. It's glue between a framework and a library. By design, such a class violates the SRP: You will need to change it whenever the integrated library adds, removes or changes functionality that this package is supposed to integrate. And splitting this class into microscopic methods does not change that: it's still a class with multiple resposibilities. And that's fine.

ok, thanks :)

stof commented 2 years ago

Technically, this class has a single responsible: configure the container services based on the user-provided configuration. The difference is the size of the code needed to implement that responsibility. For such responsibility, 5 LoC is not enough.

Anyway, I'm closing this for reasons already given in comments.