Page MenuHomePhabricator

Explore creating an interface to check for emptiness of an object
Open, LowPublic2 Estimated Story Points

Description

This is a follow up to the conversation at https://gerrit.wikimedia.org/r/#/c/323337/4/includes/models/MobileCollection.php.

Basically, MF PHP codebase has a use case where it would be more efficient to check for emptiness of a collection rather than counting its elements and comparing it to 0.

A/C

  • Create a new interface with one method only, e.g. isEmpty, to check for emptiness of a collection object.
  • The interface should live in MF for now and then be upstreamed to core if need be.
  • Audit the MF codebase and replace the use of count( $object ) === 0 with $collectionInstance->isEmpty() where needed.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptNov 28 2016, 7:58 PM
bmansurov triaged this task as Low priority.Nov 30 2016, 5:29 PM
bmansurov moved this task from Incoming to Triaged but Future on the Readers-Web-Backlog board.
Jdlrobson added a subscriber: phuedx.

@pmiazga @phuedx thoughts?
This seems like premature optimisation to me and I'm tempted to decline.

phuedx updated the task description. (Show Details)Apr 19 2017, 5:18 AM

I'd only approach this from a quality/readability perspective as the performance perspective is moot – @pmiazga's summary of empty vs count is accurate: $collectionInstance->isEmpty() reads better than count($collectionInstance) === 0.

phuedx updated the task description. (Show Details)Apr 19 2017, 5:31 AM

If someone feels a strong urge to do this lets by all means do it... (JFDI! :)) Does this make the code more readable? Is there a reason not to?

But if nobody does feel that urge, let's be realistic, is there value in keeping this around as a task that is probably never going to be scheduled for a sprint? There are thousands of similar tasks I could create for code I don't like in MobileFrontend but I don't see the value in creating tasks for it - I'd rather just write a patch.

Thoughts?
How can we motivate these kinds of changes without overwhelming our backlog with low priority tasks?
Does anyone want to champion this task? (If so there's more motivation for me to put this into an upcoming sprint if I know somebody is motivated to work on it)?

I think we should do it as it makes easier to read code. My concern is where to put it as IIsEmptyChecker as it Core seems the most appropriate place, but currently, MW doesn't look like "interface driven platform".

@Jdlrobson let me be the Champion. IMHO this task is valuable, not because "optimisation" but because of code readability. I'll add an easy tag as this is easy part, maybe some volunteer wants to work on it.

Restricted Application added a subscriber: TerraCodes. · View Herald TranscriptApr 21 2017, 6:15 PM

Thanks for the vote of confidence! If anyone wants to work on this @pmiazga has signed up to help review this!

Jdlrobson raised the priority of this task from Low to Medium.Feb 14 2018, 11:52 PM
ovasileva set the point value for this task to 2.Feb 20 2018, 5:53 PM
Jdlrobson lowered the priority of this task from Medium to Low.Apr 9 2019, 4:14 PM

Reflecting reality