Recommend caching magic strings

[SilentSin]

Problem statement

The need for magic strings is unfortunately widespread throughout Unity's APIs. For example:

animator.Play("Attack");

This is inefficient because Unity has to call Animator.StringToHash every time, but it's also a bad practice because of the magic string. Are you sure you spelled it correctly? Are you sure it's not "Light Attack" or "Attack 0"? Is that animation state referred to anywhere else in the script or the project? And so on.

The same thing applies to Layers, Materials, Navigation Areas, Resources, Scenes, Shaders, Tags, and probably a few other areas of the engine. But I'll only go over the areas I'm most familiar with.

Proposed solution

I know that Jetbrains Rider is able to validate that tag strings in your code are actually valid tags in the Unity project. I don't know if that would be possible or practical to do with an analyser, but some simple code fixes to use a static field would be a good starting point.

Fix 1 - In current type

public static readonly int Attack = Animator.StringToHash("Attack");

...
animator.Play(Attack);

Fix 2 - In global type

public static class Animations
{
    public static readonly int Attack = Animator.StringToHash("Attack");
}

...
animator.Play(Animations.Attack);

This would need the analyser to create a new script if the Animations class doesn't already exist (like how the warning suppressions can create a GlobalSuppressions.cs file). Probably put it in Assets/Code or Assets/Scripts if they exist, but otherwise just in the root Assets folder. Can it simply ask where you want to save a file?

Animator

Applies to:

• Play, CrossFade, and their ...InFixedTime variants.
• All the parameter methods like GetFloat, SetBool, etc.

Examples as above.

Layers

Applies to:

• LayerMask.NameToLayer
• GameObject.layer

Example:

var mask = (1 << LayerMask.NameToLayer("Default")) | (1 << LayerMask.NameToLayer("Water"));
if (Physics.Raycast(... mask ...))

...
gameObject.layer = 5;

Becomes

public static readonly int DefaultWater = (1 << LayerMask.NameToLayer("Default")) | (1 << LayerMask.NameToLayer("Water"));
public static readonly int Layer5 = 5;

...

if (Physics.Raycast(... DefaultWater ...))

...
gameObject.layer = Layer5;

The separate class would be called Layers.

Ideally the analyser could call LayerMask.LayerToName to determine a proper name for the Layer5 field and initialise it with LayerMask.NameToLayer. Otherwise it could just add a comment recommending the use of LayerMask.NameToLayer.

Resources

Applies to:

• Resources.Load
• AssetDatabase.LoadAsset

Example:

var prefab = Resources.Load<GameObject>("GUI/Text/Damage Text");
var instance = Instantiate(prefab, ...);

Becomes:

private static GameObject _DamageText;

public static GameObject DamageText
{
    get
    {
        if (_DamageText == null)
            _DamageText = Resources.Load<GameObject>("GUI/Text/Damage Text");
        return _DamageText;
    }
}

...
var instance = Instantiate(DamageText, ...);

The separate class for this would be called Assets since Resources is obviously already taken.

This one should only be a suggestion (not a warning) since the loaded asset might only be used once anyway and it adds a lot of extra verbosity.

A fix to move the string out to a constant could also be reasonable.

[jbevain]

Thank you for filing this, that also makes a lot of sense. I'll discuss with the team how to best approach this and we'll get back to this. Thanks again!

[sailro]

Automatically adding "help wanted" tag for stale issues identified as good community contribution opportunities

[imniko]

Any update on this one? Actually I am more interested how to go about parsing some unity files (specially for tags and layers) and then using those results in the analyzers. And that is it even doable?

[Monsoonexe]

This is a solid suggestion and good advice.