Make handlers instances, or only static, not some mix

[Grazfather]

We mostly use class/static methods, while using inheritance to force the implementation of certain methods.

This is fine, but we then instantiate the class (for virtually no reason) when registering it in the factory. We could do away with this unless we need to handle more than one copy of a handler at a time.

[Kileak]

Not sure, if I completely understood the issue, but if you meant to make the classes completely static, not creating instances of the handlers at all, I tend to disagree.

Though I'm not sure, if I could come up quickly with a real use case, but I wouldn't throw away this functionality so easily.

What we could do with this, is to create handlers, which mostly do the same thing, but have changed functionality (or origin objects) depending on their ctor (strategy pattern comes to mind for this).

Also, I'm not sure, how you'd iterate the handlers without directly referencing them in the HandlerFactory if you don't have some registration-method (but then again, I might just missing some python functionality here). The handler factory should stay loose-coupled, thus not knowing anything about the available handlers, except for an interface (I know this is not entirely true in our implementation).

---

But it's true, that the implementation of the ChallengeHandler violates this idea! I think, it was a mistake to make the update_database_from_slack method static. Though it made implementing the reload command easier, it creates the constraint, that not more than one instance of the ChallengeHandler must exist (which is also already enforced, since we defined the database, that should be used for the ChallengeHandler hardcoded and static).

If we want to get the concept clear again, we could instead:

• Add a parent to the Command object (so it knows, the handler it belongs to)
• Add an argument object to the ctor's of our handlers, so we can give them initilization arguments
• Pass the database for the ChallengeHandler as an argument
• Make update_database_from_slack a class method again
• The Reload-Command could call the update via it's parent object

Initializing the ChallengeHandler hardcoded with the database in that way, doesn't really gives an advantage now, but it would make the handler usage clearer.

This makes more sense, if you think of future development, where we might add a startup configuration file, in which you define which handlers should get registered with which arguments and remove the hardcoded register-methods from the handler (which currently only are a workaround to make the handlers known to the factory. I would prefer a method, in which the factory could discover the available handlers for itself and register them dynamically at runtime (in C# i would do this via reflection, just don't know how to do it in python atm)).

Though not sure, if we should prioritize this, since it currently gives us no big advantage and we might have other issues to implement first.

But this might have potential for more discussion.

[Kileak]

If we decide, to continue with instanced handlers in the factory, I'll change the handling for Command and refactor the static methods, after the current pr's are merged, since this might produce too much noise in current merges.

[Grazfather]

I'm with you on allowing instances. Strategy or some other dependency injection is a good example as to why we probably want instances.

I should rename this bug so that the intent is more to choose one: Instances or static classes.

Also, I'm not sure, how you'd iterate the handlers without directly referencing them in the HandlerFactory if you don't have some registration-method (but then again, I might just missing some python functionality here).

You can register the class itself, without registering the instance. The only real change is that you wouldn't call the constructor

-HandlerFactory.register("ctf", ChallengeHandler())
+HandlerFactory.register("ctf", ChallengeHandler)

I would prefer a method, in which the factory could discover the available handlers for itself and register them dynamically at runtime (in C# i would do this via reflection, just don't know how to do it in python atm)).

This can be done. We could add a decorator that registers them, or we could also use reflection to find objects that subclass BaseHandler. There's a chapter in Fluent Python that discusses this very problem, coincidentally.