HomeWildfire Games

ShouldConsiderOurselvesAtDestination renamed to…
Concern RaisedrP22299

Description

ShouldConsiderOurselvesAtDestination renamed to CloseEnoughFromDestinationToStop and stateful effects moved at the caller.

It improves readability of the code when a function that seems like it does a simple check actually only does a simple check and the caller is the one changing state.

Differential Revision: https://code.wildfiregames.com/D1884

Event Timeline

elexis raised a concern with this commit.Mon, Jun 24, 4:07 PM
elexis added a subscriber: elexis.

ShouldConsiderOurselvesAtDestination from rP17226.
Agree with that previous name being broken because this is only about the unit, not about the reader.

function that seems like it does a simple check actually only does a simple check and the caller is the one changing state.

Then rename the function and update the comments to indicate that this function does something instead of adding duplication.

This commit now has outstanding concerns.Mon, Jun 24, 4:07 PM

The reason why I moved the stateful calls outside the functions was mostly in planning of further diffs, which remove StopMoving() and "FacePointAfterMove", i.e. the duplication.

These two commits have been merged already and it now looks like this:

This duplication of MoveSucceeded isn't necessarily going to remain forever either since PathResult might want to do something different there instead.
My opinion is that this concern has already been fixed.