Skip to:
Content

BuddyPress.org

Opened 5 years ago

Closed 4 years ago

#6271 closed enhancement (wontfix)

Separate activity WHERE conditions into new method

Reported by: johnjamesjacoby Owned by:
Milestone: Priority: low
Severity: minor Version: 1.6
Component: Activity Keywords: 2nd-opinion has-patch
Cc:

Description

BP_Activity_Activity::get() is a fairly large method, and around half of it is from figuring out $where_conditions. We should be able to separate them out into a new method similar to Notifications without too much hassle.

Attachments (1)

6271.01.patch (9.4 KB) - added by johnjamesjacoby 5 years ago.

Download all attachments as: .zip

Change History (8)

#1 @johnjamesjacoby
5 years ago

  • Keywords has-patch added

6271.01.patch separates out $where_conditions into a new get_where_sql() method. Tests still pass, and I haven't identified any breakage.

The new method is marked protected, but not really for any other reason than it does no good to call it from outside the class anyways.

#2 @boonebgorges
5 years ago

Agreed that get() is an overly large method, but I'm not sure I see the benefit of separating out the logic in just this way. In the case of Notifications, it's abstracted in one chunk because it's used in more than one place in the class. Could that potentially be the case as well for get_where_sql() (I haven't looked). In terms of sheer organization, it seems like we'd get more benefit from abstracting each specific WHERE clause into its own method, though much of this is done already (get_date_query_sql(), get_filter_sql(), etc).

I'm not necessarily against the proposed change, but I would like to be sure we're not refactoring with no benefit, especially if there's an alternative way to refactor that would be more beneficial in terms of testability and reusability.

#3 @johnjamesjacoby
5 years ago

We could also break up the individual conditions, which would make them easier to test.

Notifications does use this method in two places, but we will likely end up eliminating that extra count query to better match what's currently in the Activity component.

The addition of Notification meta in #6257 means it's under similar scrutinization, so we can apply our conclusion there as well.

Last edited 5 years ago by johnjamesjacoby (previous) (diff)

#4 follow-up: @boonebgorges
5 years ago

Notifications does use this method in two places, but we will likely end up eliminating that extra count query to better match what's currently in the Activity component.

Why?

#5 in reply to: ↑ 4 @johnjamesjacoby
5 years ago

Replying to boonebgorges:

Notifications does use this method in two places, but we will likely end up eliminating that extra count query to better match what's currently in the Activity component.

Why?

I haven't done a full comparison between the approaches, so I don't have a technical one. Ideally though, if there's no reason to be inconsistent, why would we want to be?

#6 @DJPaul
5 years ago

  • Milestone changed from 2.3 to Under Consideration

#7 @DJPaul
4 years ago

  • Milestone Under Consideration deleted
  • Resolution set to wontfix
  • Status changed from new to closed

This has gone nowhere in a year; apart from probable staleness, it looks like we required a little more technical information about the desired change. I'm going to close this ticket for now.

Note: See TracTickets for help on using tickets.