From #1038160: Cleanup object / array entity CRUD code:

The main thing that I noticed are some duplicating/overlapping/confusingly named functions. Two examples:

simplenews_get_subscription($acount): Doesn't return a *subscription* but a subscriber object. Should probably be renamed to _get_subscriber, or better, _load_subscriber_by_account(), which is similar to e.g. user_load_by_mail() and so on.

simplenews_get_mailing_lists($show_all) is a wrapper of simplenews_categories_load_multiple() with either the hidden condition set or not.

CommentFileSizeAuthor
#11 simplenews_category_get_visible.patch6.16 KBberdir
#9 fixed_the_tests.patch6.17 KBberdir
#7 re-roll.patch6.63 KBberdir
#3 rename_categories.patch7.42 KBberdir

Comments

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

More:

simplenews_category_load()/list(), but simplenews_categories_load_multiple(). Should all be category, it's node_load_multiple(), not nodes_load_multiple().

Actually, category itself is kinda wrong, but that's probably too much of a change right now. Because simplenews_newsletter_*() already exists, but that's actually about newsletter issues..

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented

At least, we should unify the unclean plural+singular namings.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Active » Needs review
StatusFileSize
new rename_categories.patch7.42 KB

Yes.

The get_subscription() function has it's own issue now.

I am not sure what we should rename get_mailing_lists() to but the name is clearly misleading. Maybe simplenews_get_public_categories() or something like that because we only need to cover the $show_all = FALSE case.. the other one is already covered by simplenews_category_load_multiple() directly...

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

#3: rename_categories.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, rename_categories.patch, failed testing.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Needs review

Renamed simplenews_get_mailing_lists() to simplenews_get_visible_categories(), I think that's kinda fine.

Other than that, just a re-roll.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
StatusFileSize
new re-roll.patch6.63 KB

And now with a patch.

Status: Needs review » Needs work

The last submitted patch, re-roll.patch, failed testing.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Needs review
StatusFileSize
new fixed_the_tests.patch6.17 KB

Fixed the test fails.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Status: Needs review » Needs work

As discussed, i prefer having
simplenews_category as a common prefix for all category related functions.
So please update
simplenews_get_visible_categories() => simplenews_category_get_visible()

This allows better alphanumeric grouping in IDEs and provides better readability (at least for me ;-) ).

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Needs review
StatusFileSize
new simplenews_category_get_visible.patch6.16 KB

Makes perfect sense, updated.

Will commit this tomorrow in the train or so...

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Status: Needs review » Reviewed & tested by the community

Looks fine to me.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Reviewed & tested by the community » Fixed

Commited.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.