Yarn: There should not be five getEntities methods

Created on 22 Jun 2020  路  8Comments  路  Source: FabricMC/yarn

As of 1.16-rc1+build.12, there's five (!) methods available on a ServerWorld all named getEntities:

  • EntityView#getEntities(@Nullable Entity, Box) gets all entities in the box excluding the one in the first argument.
  • EntityView#getEntities(@Nullable Entity, Box, Predicate) gets all entities in the box excluding the one in the first argument that also match the predicate.
  • ServerWorld#getEntities(@Nullable EntityType, Predicate) gets all entities in the whole world matching the first argument's type and predicate.
  • World#getEntities(@Nullable EntityType, Box, Predicate) gets all entities in the box matching the first argument's type and predicate.
  • EntityView#getEntities(Class, Box, Predicate) gets all entities in the box matching the first argument's class and predicate.

Why is this a problem?

  • Two of them exclude the first argument, three filter to the first argument; IMO this is reason enough to change the name.
  • When calling any of the three-argument forms with null as the first parameter, you need to cast the null at the call site because the call is ambiguous.
    image
    (^ Taking IntelliJ's advice there and casting to Class would NPE at runtime, btw, because that method is not null-safe.)

Proposed solution:

  • To clear up the ambiguous parse:

    • Rename both methods that take Entity as a first parameter to getEntitiesExcept.

    • Rename getEntities(Class, ...) to getEntitiesOfClass or something like that.

  • Maybe rename the one that doesn't take a Box to, I dunno, getAllEntities.
  • Propagate these renames to the WorldChunk methods I'm pretty sure these all delegate to.
discussion refactor

Most helpful comment

Still, that's what the method does - it gets all the entities passing the other conditions, except the one that you passed in.

For reference, the majority of calls to the first-argument-Entity form pass either null or this. I think getEntitiesExcept(this, ... scans well. (And the parameter is already named except in Yarn.)

All 8 comments

getEntitiesExcept doesn't make sense. the entity parameter is provided because the provided entity is the entity performing search for other entities.

Still, that's what the method does - it gets all the entities passing the other conditions, except the one that you passed in.

For reference, the majority of calls to the first-argument-Entity form pass either null or this. I think getEntitiesExcept(this, ... scans well. (And the parameter is already named except in Yarn.)

Giving them different names would definitely make their purposes and uses clearer.

They all have the same purpose: Getting entities in the world, or in a box if given, excluding the searcher entity if given, applying the predicate if given. Simple as that, getEntities is the right name.

the ambiguous call stuff is incredibly annoying

Imo we should rename ones specifying a type/class getEntitiesOf to clarify.

The ones taking an entity can be named getSurroundingEntities, and the ones taking a type can be called getMatchingEntities or getEntitiesByType

This is really cursed. they are just variants of a method calling getEntities(Type, Entity, Box, Predicate),
where Type can be defaulted to any type, entity can be defaulted to null, box can be defaulted to infinity, and predicate be defaulted to always true. I don't see a reason to name these 5 methods differently at all.

Was this page helpful?
0 / 5 - 0 ratings

Related issues

ChloeDawn picture ChloeDawn  路  6Comments

ChloeDawn picture ChloeDawn  路  5Comments

asiekierka picture asiekierka  路  4Comments

Juuxel picture Juuxel  路  5Comments

quat1024 picture quat1024  路  3Comments