dboehmer / coocook

👨‍🍳🦉 Web application for collecting recipes and making food plans
https://coocook.org/
Other
11 stars 2 forks source link

Refactor controller names (please review, but don’t merge yet) #137

Open dboehmer opened 3 years ago

dboehmer commented 3 years ago

I had a quick idea to resort the modules below Coocook::Controller. Please review: Primarily is that an enhancement? Technical review might not be that important because tests pass and I am pretty sure it’s complete. Please see below for open question.

The thing is our controllers which handle project-specific data were spread all over the controller namespace. Some of them would chain from /project/base and others not.

Before

Can you spot the project-specific controllers?

$ tree lib/Coocook/Controller/
lib/Coocook/Controller/
├── Admin
│   ├── FAQ.pm
│   ├── Terms.pm
│   └── User.pm
├── Admin.pm
├── Article.pm
├── Badge.pm
├── Browse
│   ├── Project.pm
│   └── Recipe.pm
├── Dish.pm
├── Email.pm
├── Error.pm
├── FAQ.pm
├── Item.pm
├── Meal.pm
├── Organization
│   └── Member.pm
├── Organization.pm
├── Permission.pm
├── Print.pm
├── Project.pm
├── PurchaseList.pm
├── Quantity.pm
├── Recipe
│   └── Import.pm
├── Recipe.pm
├── Root.pm
├── Session.pm
├── Settings.pm
├── ShopSection.pm
├── Tag.pm
├── Terms.pm
├── Unit.pm
└── User.pm

After

$ tree lib/Coocook/Controller/
lib/Coocook/Controller/
├── Admin
│   ├── FAQ.pm
│   ├── Terms.pm
│   └── User.pm
├── Admin.pm
├── Badge.pm
├── Browse
│   ├── Project.pm
│   └── Recipe.pm
├── Email.pm
├── Error.pm
├── FAQ.pm
├── Organization
│   └── Member.pm
├── Organization.pm
├── Project         # all of them well together
│   ├── Article.pm
│   ├── Dish.pm
│   ├── Item.pm
│   ├── Meal.pm
│   ├── Permission.pm
│   ├── Print.pm
│   ├── PurchaseList.pm
│   ├── Quantity.pm
│   ├── Recipe
│   │   └── Import.pm
│   ├── Recipe.pm
│   ├── ShopSection.pm
│   ├── Tag.pm
│   └── Unit.pm
├── Project.pm
├── Root.pm
├── Session.pm
├── Settings.pm
├── Terms.pm
└── User.pm

Open question

In classic Catalyst logic we have Project.pm and Project/ with submodules next to each other. This is all project-specific.

Then the are the Browse modules like I named them. There are not about 1 project but exploring all Coocook data. That is:

Current state of this PR

├── Browse
│   ├── Project.pm
│   └── Recipe.pm
├── Project
│   ├── Article.pm
│   ├── ...
├── Project.pm        # does this belong to Browse or to Project??

Solution 0: Project::Project controller

That name reads like a mistake.

Solution 1: Root controller

This is also what Catalyst does for the application’s main controller MyApp::Controller::Root.

├── Browse
│   ├── Project.pm
│   └── Recipe.pm
├── Project
│   ├── Root.pm       # what has been Controller/Project.pm
│   ├── Article.pm
│   ├── ...

Solution 2: explicit namespace for project-specific controllers

├── Browse
│   ├── Project.pm
│   └── Recipe.pm
├── PerProject         # notice new name!
│   ├── Project.pm       # what has been Controller/Project.pm
│   ├── Article.pm
│   ├── ...

Optional next step: dissolve Controller::Browse namespace

Did you ever dislike the Browse namespace? It’s pretty generic and POST actions are not like “browsing”.

├── PerProject         # or similar, see options above
│   ├── Project.pm
│   ├── Article.pm
│   ├── ...
├── Project.pm        # what was Browse::Project 
├── Recipe.pm        # what was Browse::Recipe
dboehmer commented 3 years ago

ping @moseschmiedel