Page MenuHomePhabricator

Explore creating an interface to check for emptiness of an object
Closed, ResolvedPublic2 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

Jdlrobson added a subscriber: phuedx.

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

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.

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.

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

In which file I should create a interface?

@polishdeveloper would you be able to help review this one? This is not a good first task for me to review and guide :)

Yes, definitely. Happy to help. Thank you for the ping @Jdlrobson .

Currently, for MobileCollection we had to implement Countable so we can somehow get the count of objects. But we do not need the count, we just need isEmpty(). Therefore we got the idea to implement an interface that can provide just that method. For context: https://gerrit.wikimedia.org/r/#/c/323337/4/includes/models/MobileCollection.php.

@Yash4357 IMHO this file should go to core so other things can use it. We need an interface with a single method isEmpty() in the MediaWiki core (that would be the first patch).
Then, in places like MobileFrontend, Minerva, Vector we would need to check for places where we pass objects or collections of objects - and the only thing we do with such objects is checking count().

Change 657892 had a related patch set uploaded (by Yash9265; owner: Yash9265):
[mediawiki/core@master] Creating an interface to check for emptiness of an object

https://gerrit.wikimedia.org/r/657892

I had uploaded a new patch. Please have a look on it.

I had uploaded a new patch from long time. Please review on it.

I'll look into it today. Sorry to keep you waiting

@Yash4357 Your core patch got merged. Are you interested improving the MobileFrontend by using this interface?

Yeah, soon I will generate the new patch for MobileFrontend
by using this interface

Change 657892 merged by jenkins-bot:
[mediawiki/core@master] Creating an interface to check for emptiness of an object

https://gerrit.wikimedia.org/r/657892

Change 672502 had a related patch set uploaded (by Yash9265; owner: Yash9265):
[mediawiki/extensions/MobileFrontend@master] Using Emptiable interface to check for emptiness of an object

https://gerrit.wikimedia.org/r/672502

I didn't see the original patch, so I didn't leave this as a -1 there, but since the Emptiable class has nothing to do directly with MediaWiki, it should be /includes/libs rather than just /includes in my view

So should I create a new patch, for creating the Emptiable
class in the /includes/libs.

Change 672502 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Using Emptiable interface to check for emptiness of an object

https://gerrit.wikimedia.org/r/672502

LGTM. Thanks to everyone involved!

  • 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.

Just a note that the the MediaWiki\Emptiable interface, defined in MediaWiki Core, was used.

phuedx claimed this task.