9
\$\begingroup\$

Does annotating Unity lifecycle methods with the UsedImplicitly attribute improve the readability of your code?

For example:

public class Example : MonoBehaviour
{
    [UsedImplicitly]
    private void Awake()
    {
        DoSomething();
    }

    private void DoSomething()
    {
        // ...
    }
}

This blog post on "Structuring Your Unity MonoBehaviours" suggests this approach as a useful convention.

  1. Annotate any Unity lifecycle methods (Start, Awake, Update, OnDestroy etc.), event methods, and other functions that are implicitly (or automatically) invoked in your code with the [UsedImplicitly] attribute. This attribute, although included in the UnityEngine.dll, is used by ReSharper to disable code cleanup suggestions for methods that are seemingly unreferenced. It also helps to make the code easier to read for people who are newer to Unity and your codebase, especially for events that are referenced via the inspector.

(Note: I don't think ReSharper shows code-cleanup suggestions for seemingly unreferenced Unity lifecycle methods, so that might be outdated advice.)

I agree with the above post that it could be helpful for those newer to Unity. I also think it could be useful to label those Unity lifecycle functions that are not used as frequently as Awake, Start, and Update. But I am wondering if this is actually a good convention for "clean code", or if it's just noise that reduces readability.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ I've published 2 games in Unity and didn't know this existed. If I did I'd probably have used it for clarity and wouldn't consider it noise. \$\endgroup\$
    – McAden
    Apr 10 '18 at 16:52
  • 1
    \$\begingroup\$ Hey, I'm the original author of the post. Yeah I think it can be overkill for lifecycle methods, especially given the improved Resharper support. However I think that it's useful to annotate event callbacks (e.g. for animations & Unity's UI event system) that are only referenced via the inspector. It's up to the people managing the codebase though - when I wrote this, I was working at a software consulting company where nobody had game dev experience, so I wanted to establish a standard between us. It's a bit draconian in retrospect, since I didn't expect the post to get attention. \$\endgroup\$
    – Mana
    Apr 19 '18 at 4:34
10
\$\begingroup\$

It depends on the target demographic.

In the example above, the target demographic is the ReSharper tool, which benefits from these attributes because it suppresses messages which aren't helpful to you. But if you don't feel the need to use ReSharper or a similar tool (although maybe you should, they have some valid critique from time to time), then you don't need to care about that demographic.

Anyone who ever did a basic Unity tutorial knows very well that Start and Update are used implicitly by the Unity engine, so it won't give them any new information. It might in fact confuse the heck out of them if you forget this once. What do you want to say with that? Is this maybe not actually a MonoBehaviour? Or is there some obscure reason why you must call it explicitly which I am not aware of?

It can, however, help if you are working with inexperienced developers who might not be familiar with the more esoteric Unity events like Reset (dangerous, because calling it explicitly might not do what you think it does). The [UsedImplicitly] attribute might then tell them that they don't need to look for the class which calls that method because the engine does. But again, this is only of value if you have the discipline to do this consistently. And if you have that amount of self-discipline, you could just as well agree on adding a comment.

\$\endgroup\$
3
  • 1
    \$\begingroup\$ I would add a "for 99% demographic it provides no benefit". Even beginners after few hours will start to recognize lifecycle methods (they are colored differently in VS!). And resharper is: a costly licence, can be reconfigured, and as OP said it is probably already patched anyways. I think one can spend their time more effectively than decorating every second method with attributes of arguable value. \$\endgroup\$
    – wondra
    Apr 10 '18 at 16:25
  • \$\begingroup\$ @wondra Thanks for pointing out that they are colored differently in Visual Studio. I'll try to figure out why it doesn't do that for me with Visual Studio 2017, even though Tools -> Options -> Tools for Unity -> General -> Unity Messages syntax highlighting is set to true. \$\endgroup\$
    – sonny
    Apr 10 '18 at 20:15
  • 1
    \$\begingroup\$ I didn't look into why Visual Studio was not coloring my Unity methods, but another answer pointed out that ReSharper has a plugin for Unity that also automatically recognizes Unity event functions. There is also this related setting: ReSharper -> Options -> Code Inspection -> Settings -> Enable code analysis -> Color identifiers. \$\endgroup\$
    – sonny
    Apr 11 '18 at 15:02
6
\$\begingroup\$

I argue that no you should not use it.

The UsedImplicitly attribute is from the Jetbrains.Annotations namespace so the only case where it might apply is if everyone using the code will be using ReSharper.

ReSharper has a plugin for Unity which already handles this for you, making sure not just to remove warnings that they are not used but also marking them with a little Unity symbol so that anyone reading the code can see that they are called by Unity itself.

I'd also like to add an example when using them might go wrong. Assume you have a class inheriting from MonoBehaviour, but after a refactor it no longer has any inheritance. If you marked private methods with [UsedImplicitly] then you will not receive the correct warnings. If you use the Unity plugin for resharper it will notice that you no longer inherit from MonoBehaviour and will throw warnings as the methods indeed are no longer called.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ I was not aware that ReSharper plugin existed, thanks for pointing it out! \$\endgroup\$
    – sonny
    Apr 11 '18 at 12:08
  • 1
    \$\begingroup\$ Note that Unity has started including the ReSharper attributes in UnityEngine.dll as of Unity 5 - see this forum post. \$\endgroup\$
    – sonny
    Apr 11 '18 at 12:08
5
\$\begingroup\$

I'd make the argument that it can't really hurt.

For somebody very familiar with Unity's workings, it probably doesn't add anything. Those people are already going to be pretty familiar with what Unity's runtime calls on your behalf.

But for somebody who isn't, like me, it might be helpful. Even if I didn't know what it meant offhand, I'd go look it up, and that would probably be sufficient to give me a better understanding of what was going on in the code.

Also, if one does use static analysis tools that are incorrectly warning about the methods, then you'll want to quiet those warnings somehow, because "ignorable" warning noise makes it harder to see the real warnings in any such output. It's usually better to ignore such warnings in a surgical, localized fashion (in this case by decorating the offending methods with attributes) than through broader measures like disabling that specific warning project-wide.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ Thanks for your answer. I was thinking similarly to your answer when I posted the question, but I agree with other posters who point out that it could be potentially misleading if the attribute is not used consistently. \$\endgroup\$
    – sonny
    Apr 10 '18 at 19:33
  • 3
    \$\begingroup\$ Yeah, those are fair points. If one does have static analysis that trips over this, and one treats those warnings seriously, that can help you ensure the attribute is applied uniformly. But if you don't? It's pretty much up to human error then. \$\endgroup\$
    – user1430
    Apr 10 '18 at 19:47
2
\$\begingroup\$

The problem with this approach is that it's incredibly error prone.

If it isn't reliable, the tags will fail to convey useful information, and their presence or absence will occasionally succeed at conveying false information, which is harmful.

If you decide to go with it, you must remove the human error, which isn't hard to do, but takes a good 2 hours of work if you haven't done something like this before. There are 2 approaches - each with their own advantages - you can use either one:

  1. Verify and reject non-complying code on check in or automated build.
  2. Automated edit of code to fix tags on check in or automated build.

Is it worth to have it? Yes. Is it worth 2 hours of your time? Your call.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ I've accepted a different answer, but you make a great point about removing the human error for anyone who is thinking about using the UsedImplicitly attribute for this purpose. \$\endgroup\$
    – sonny
    Apr 10 '18 at 19:35

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.