Automattic / Co-Authors-Plus

Multiple bylines and Guest Authors for WordPress
https://wordpress.org/plugins/co-authors-plus/
GNU General Public License v2.0
291 stars 204 forks source link

Separate out some functions #396

Open philipjohn opened 7 years ago

philipjohn commented 7 years ago

co-authors-plus.php is pretty big and unnecessarily so. It should be split up into more manageable chunks.

For example, filter_ef_calendar_item_information_fields() and filter_ef_story_budget_term_column_value() could move into the integrations folder, along with the AMP compat stuff.

Admin UI functionality could be split into a separate file/class too.

TheCrowned commented 6 years ago

I have taken some time to think about how the co-authors-plus.php file could be split to make it more manageable. This is what I have come up with - please do feel free to provide feedback @philipjohn !

I am going to list the new files I would create, the functions I would put in them and how much this would reduce the main file size.

php/class-edit-screen.php This file would contain functions that change the default behavior of the edit.php page.

Lines removed from main file: ~ 120

php/class-users-screen.php This file would contain functions that change the default behavior of the users.php page. Not too sure about this one though.

Lines removed from main file: ~ 35

php/class-metabox.php This file would contain all functions related to CAP metabox handling.

Lines removed from main file: ~ 100

php/email-notifications.php This file would contain all functions related to (email) notifications. These are currently not class functions, but rather global ones.

Lines removed from main file: ~ 140

php/integrations/edit-flow.php This file would contain all functions related to Edit Flow integration.

Lines removed from main file: ~ 50

php/integrations/jetpack.php This file would contain all functions related to Jetpack integration.

Lines removed from main file: ~ 30

In total, this would strip the main file of ~ 475 rows, or ~ 25% of its current size. With class-users-screen.php, we would be at ~ 510 rows, 29%.

I see the AMP functions are not wrapped into a class, but rather are all prefixed. Is there a reason for this? Or rather, is there a good reason for prefixing functions for Edit Flow and Jetpack instead of wrapping them in a class?

On a side note, this would also be a good opportunity to catch up with the inline documentation of some functions, which is sometimes quite lacking :)

philipjohn commented 6 years ago

php/class-edit-screen.php This file would contain functions that change the default behavior of the users.php page.

The description here is supposed to say edit.php rather than user.php I think?

I see the AMP functions are not wrapped into a class, but rather are all prefixed. Is there a reason for this? Or rather, is there a good reason for prefixing functions for Edit Flow and Jetpack instead of wrapping them in a class?

Heh, we risk the whole functional vs class programming debate here ;) Personally, I'd advocate to use classes only for describing objects. I'm not sure why these functions were not included in the class - I guess personal preference of the person adding them.

On a side note, this would also be a good opportunity to catch up with the inline documentation of some functions, which is sometimes quite lacking :)

Absolutely! I :heart: good inline documentation.

Overall this looks like a good logical breakdown of the areas that the code deals with. It's a great start, thanks!

I see you've prefixed some of the filenames with class-. Are you planning to move the functions into new classes? If so, what impact is that going to have on the calls to those functions throughout the rest of the codebase?

Could moving some of those functions result in direct calls from theme/other plugin code from failing? Do we need to consider that?

TheCrowned commented 6 years ago

The description here is supposed to say edit.php rather than user.php I think?

Absolutely - I have corrected that, sorry!

I see you've prefixed some of the filenames with class-. Are you planning to move the functions into new classes?

I thought so. For what I have seen, most of the functions I would move could be declared as static. It doesn't make much of a difference with having standalone prefixed functions. I thought about classes because all the current functions are wrapped in a function, but I don't really have any strong preference.

If so, what impact is that going to have on the calls to those functions throughout the rest of the codebase?

Actions/Functions invoking them would need to be updated, but other than a hassle, I don't think that would cause problems.

Could moving some of those functions result in direct calls from theme/other plugin code from failing? Do we need to consider that?

I think I can make sure no issues arise from other parts of the plugin. Themes should only use template-tags.php functions, so changes in the main file shouldn't cause problems (that is, as long as we are careful in updating template tags functions accordingly) :)

philipjohn commented 6 years ago

Okay, sounds good - thanks!

TheCrowned commented 6 years ago

I am working on this, but leaving out email notification functions since they actually be removed as per #421.