Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Naming of classes like Manager #8115

Closed
tsteur opened this issue Jun 16, 2015 · 3 comments
Closed

Naming of classes like Manager #8115

tsteur opened this issue Jun 16, 2015 · 3 comments
Labels
answered For when a question was asked and we referred to forum or answered it. RFC Indicates the issue is a request for comments where the author is looking for feedback.
Milestone
3.0.0-b1

Comments

@tsteur
Copy link
Member

tsteur commented Jun 16, 2015

I hope there is not already an issue about this since I can remember we had this topic already once re Plugin\Manager or so.

See comment here: #8045 (comment) where we have a class like Piwik\Measurable\Type\Manager. We already have a couple of classes like Manager (we know Manager is not a good name see eg http://www.slideshare.net/pirhilton/how-to-name-things-the-hardest-problem-in-programming but it's not about that particular name). There are suggestions to name such classes eg

  • Piwik\Measurable\Type\Manager
  • Piwik\Measurable\Type\TypeManager
  • Piwik\Measurable\Type\MeasurableTypeManager

to not having to write eg use Piwik\Measurable\Type\Manager as TypeManager.

I usually just use it in the code directly like this new Type\Manager(). This way I do not have to write the use ... as ... and it is still clear what is meant. So just Manager is usually ok for me personally but I can understand that it can be sometimes confusing to find the correct Manager class immediately when searching for the class name in eg PHPStorm. Still I personally prefer just Manager. For example it also allows us to change the wording Type without having to refactor the class TypeManager etc.

Goal of this issue is to maybe agree on a naming issue to have a consistent naming in Piwik and avoiding having discussions about this in PRs.

If we name the class eg TypeManager where would be put "subclasses" of it. In namespace Piwik\Measurable\Type\Manager\Whatever* or Piwik\Measurable\Type\TypeManager\Whatever\*?

In this case there might be also many Type classes. So would it be like Piwik\Measurable\MeasurableType\MeasurableTypeManager\Whatever\*?
@tsteur tsteur added the RFC Indicates the issue is a request for comments where the author is looking for feedback. label Jun 16, 2015
@tsteur tsteur mentioned this issue Jun 16, 2015
Merged
@mnapoli
Copy link
Contributor

mnapoli commented Jun 16, 2015

This is the issue for the plugin manager problem: #7656

The thing with Plugin\Manager is that it's definitely not as fast as typing pm and autocompletion will take care of everything (including inserting the use … statement). Same with all the "go to" shortcuts of the IDE (e.g. typing Maj + Maj). Having the use … handled entirely automatically by PhpStorm is awesome.

Another issue is that it means importing the whole namespace, whereas having the list of imported class at the top of the file is very useful to get an idea of how much this class is decoupled/coupled to other classes (it serves as a good indicator IMO).

I don't have so much of a problem with the "manager" word though (even though it's not the main point of this issue). Naming in Piwik is currently source of many problems and we are on the opposite side of DDD. So I'd rather go with dull yet consistent names because it would be more useful to the project, and it would be simpler to plugin developers, contributors, etc. That's why I argue for DAO or Service names even though I wouldn't necessarily use those in other projects.

Generally I like names that describe what we are dealing with. new Manager doesn't really mean anything. There is no such thing as a manager (well, not in our code base :) ). However PluginManager definitely means something.

For example it also allows us to change the wording Type without having to refactor the class TypeManager etc.

That's the only advantage I can see, but to me it's far fetched. On the contrary I find it easier to rename TypeManager rather than Manager because Manager could appear everywhere, searching for this returns way too much noise.

If we name the class eg TypeManager where would be put "subclasses" of it

Why would there be subclasses?

@tsteur
Copy link
Member Author

tsteur commented Jun 16, 2015

The thing with Plugin\Manager is that it's definitely not as fast as typing pm and autocompletion will take care of everything

So far that's the only advantage I can see and mentioned as well before but I personally don't use it that way. Others might do.

Another issue is that it means importing the whole namespace, whereas having the list of imported class at the top of the file is very useful to get an idea of how much this class is decoupled/coupled to other classes (it serves as a good indicator IMO).

That's true. I'm not sure if it's that important though. You can usually also see it by having a look at the constructor or you can usually also see it by other indicators or when scrolling over the file.

new Manager doesn't really mean anything. There is no such thing as a manager (well, not in our code base :) ). However PluginManager definitely means something.

new Type\Manager does :)

Why would there be subclasses?

Eg Settings and Storage is a class that we have quite often. There could be eg Settings\Storage etc. Eg:

  • core/Measurable/MeasurableSetting.php
  • core/Measurable/MeasurableSettings.php
  • core/Measurable/MeasurableSettings/MeasurableSettingsStorage.php

vs

  • core/Measurable/Setting.php
  • core/Measurable/Settings.php
  • core/Measurable/Settings/Storage.php

I don't have a very strong opinion here but wanna clarify things and come to one solution that we can use in most of the cases across the code bases. Maybe we can get some more feedback from other devs? Maybe there are some blog posts about this that could be interesting?

@mattab mattab added this to the 3.0.0 milestone Jul 15, 2015
@mattab
Copy link
Member

mattab commented Aug 13, 2015

This RFC didn't lead anywhere, so we're closing it

@mattab mattab closed this as completed Aug 13, 2015
@mattab mattab added the answered For when a question was asked and we referred to forum or answered it. label Aug 13, 2015
@mattab mattab modified the milestones: 3.0.0, 3.0.0-b1 Sep 23, 2015
@michalkleiner michalkleiner mentioned this issue May 29, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
answered For when a question was asked and we referred to forum or answered it. RFC Indicates the issue is a request for comments where the author is looking for feedback.
Projects
None yet
Milestone
3.0.0-b1
Development

No branches or pull requests
3 participants
@tsteur @mattab @mnapoli