Recommend caching magic strings #49
Description
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...InFixedTimevariants.- All the parameter methods like
GetFloat,SetBool, etc.
Examples as above.
Layers
Applies to:
LayerMask.NameToLayerGameObject.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.LoadAssetDatabase.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.