Mixedrealitytoolkit-unity: InputManager: update member signatures to be static

[StephenHodgson]

Seeing as the InputManager is a singleton it makes sense that the members could/should be static.

Pros:

• InputManager.Method(); VS InputManager.Instance.Method();

Cons:

• Light refactoring. People would need to remove the Instance part of their code.

What do you guys think? Is there any other pros/cons I may have looked over?
This change should not affect behavior in any way.

[thebanjomatic]

I like this idea... If you wanted to be backward compatible, you could just delegate the static methods to the Singleton instance

public static void Method() 
{
    InputManager.instance.Method();
}

and then mark the InputManager.Instance property [Obsolete("InputManager methods are now exposed on InputManager itself and '.Instance' is no longer needed"].

The only downside I can see for refactoring the implementation itself to be strictly static, is that it prevents you from having more than one instance (if that's something you might ever need). It also makes things like ThreadLocal instances of things more difficult, but I don't think either of those are an actual issue here.

[StephenHodgson]

The only downside I can see for refactoring the implementation itself to be strictly static, is that it prevents you from having more than one instance (if that's something you might ever need)

I think it's good to only have one instance in this case because InputManager is supposed to be a singleton.

[maxouellet]

My two cents: I don't really like this idea. Having static methods all over the place is pretty bad for isolating components and properly unit testing code. In fact, I've been trying to move away from singletons as much as possible across my project's codebase for that exact reason. Instead of using singletons, I use some dependency injection to achieve a similar purpose in a more modular and decoupled way. While that solution might be too complicated for the HoloToolkit, moving to having static classes and methods completely breaks anyone that wants to do that.

You could also imagine that someone might eventually want to inherit from InputManager and add more functionalities to it. If you make it static, that makes this scenario impossible.

If you want to avoid having to call .Instance everywhere, you can always get the instance once in your Start method and cache it. That way, it's super simple to switch how you retrieve the input manager (if it ever moves away from being a singleton, for example).

[StephenHodgson]

Max I like your perspective and appreciate your view on this. I also think being modular & decoupled is definitely the way to go.

Having static methods all over the place is pretty bad for isolating components and properly unit testing code.

Wouldn't this actually help with tests because the results should be the same no matter the circumstances?

[maxouellet]

It wouldn't really help: it actually makes it impossible to unit test your classes without taking a dependency on a specific implementation of the static class. Static methods make sense when you use them for generic math functions or similar functions that have no complex dependencies, but that isn't the case for InputManager.

You are right that it doesn't make a difference for integration tests.

Eventually, I assume we will also want to add optional features to the InputManager, or have alternate implementations. When/if that happens, having static methods would prevent these scenarios.