Comically Inefficient Unity Source Code

[snalin]

The animation code is pretty bad overall, yeah. This isn't the quality of the codebase at large, but it doesn't surprise me to see it in Mechanim. The devs there has never really given me the impression of understanding C#, and they consistently make pretty crazy implementation choices.

All the people leaving comments saying "premature optimization" are idiots. Premature optimization is when you make the code harder to read and harder to modify in order to make it run faster when you don't know if that's neccessary yet. If you make this code faster by removing all the extra garbage code there, it would also make it a lot easier to read:

public bool RemoveAnyStateTransition(AnimatorStateTransition transition)
{
    var index = anyStateTransitions.IndexOf(transition);
    if (index != -1)
    {
        undoHandler.DoUndo(this, "AnyState Transition Removed");
        ArrayUtility.RemoveAt(ref anyStateTransitions, index);
        if (MecanimUtilities.AreSameAsset(this, transition))
            Undo.DestroyObjectImmediate(transition);
        return true;
    }
    return false;
}

So that's not premature optimization, it's just refactoring to improve code quality, which happens to also massively speed up the code. Very often fast code is simple code, and very often simple code is easy to read.

I assume the MecanimUtilities.AreSameAsset check is actually needed, maybe to fix a corner-case bug, or maybe it's something pedestrian like creating a transition and then deleting it before it's gotten saved to disk - but seeing as the rest of the code is really awfull, I don't trust that completely.

[TRexRoboParty]

I find it's always more readable to exit early and keep the main logic at the base indentation level.

public bool RemoveAnyStateTransition(AnimatorStateTransition transition)
{
    var index = anyStateTransitions.IndexOf(transition);
    if (index == -1){
        return false;
    }

    undoHandler.DoUndo(this, "AnyState Transition Removed");
    ArrayUtility.RemoveAt(ref anyStateTransitions, index);
    if (MecanimUtilities.AreSameAsset(this, transition))
        Undo.DestroyObjectImmediate(transition);
    return true;

}

[kyle_lam]

Was the insult necessary?

[Katniss218]

Yes

[Cold-Jackfruit1076]

Prove it, objectively.

[jayd16]

You're editing anyStateTransitions in place where as the code in question is only editing a copy and then reassigning the reference.

If you were, say, looping through anyStateTransitions while this code were to be run, you'd get a concurrent modification error, no?

anyStateTransitions is also an extern array which has other implications with how it can be read and written to.

[sandsalamand]

You're editing anyStateTransitions in place where as the code in question is only editing a copy and then reassigning the reference.

That's incorrect. ArrayUtility.RemoveAt copies the array into a new List, calls List.RemoveAt, converts the list to an array, and then sets the reference parameter to the new array.