dtr-org / unit-e

A digital currency for a new era of decentralized trust
https://unit-e.io
MIT License
45 stars 15 forks source link

Application class with no macro/template magic #723

Open scravy opened 5 years ago

scravy commented 5 years ago

Opening a discussion based on https://github.com/dtr-org/unit-e/pull/721#issuecomment-469052989 – intially commented on by @kostyantyn

I have a general question. Why do we need macros for injector? What is the critical improvement it gives us that we should go with the macro and preprocessor logic over simple C++ syntax?

What if we had a class, let's say Application (like UnitEInjector now) which in its constructor initializes each component in the same order we have now and in the destructor invokes the delete on each object in the correct order?

pseudo code:

class Application {
public:

Application() {
  m_state_register = new StateRegisterImpl();
  m_state_processor = new StateRegisterImpl(m_state_register);
  // ...
}

~Application() {
  delete m_state_processor;
  delete m_state_register;
}

StateRegister GetStateRegister() { return m_state_register; }
StateProcessor GetStateProcessor() { return m_state_processor; }

private:
  StateRegister *m_state_register;
  StateProcessor *m_state_register;
}

I see that with macro we don't need to write initializer, deinitializer and getter methods so it's an improvement. But having a bit of boilerplate (we don't create new components often, and at the code review we can check that they are initialized properly), we would have a simple C++ syntax.

Then we don't need templated GetCompoment function, we would just call g_application.GetStateProcessor() for instance. Then we don't need Dependency<...> alias, we will direcrly use * or unique_ptr or whatever the reference is.

scravy commented 5 years ago

Then we don't need Dependency<...> alias, we will direcrly use * or unique_ptr or whatever the reference is.

You can already use * or whatever the reference is. The Dependency alias is not needed it is a way to communicate to the reader the purpose of it. It signals that the lifecycle of this thing is taken care of.

Then we don't need templated GetCompoment function, we would just call g_application.GetStateProcessor() for instance.

This relates to #721 and #618. If everything was in components and injected properly then there would be no need for either GetComponent() of g_application.GetStateProcessor(). This is an instance of the service locator pattern which I for one do consider harmful. It is a stopgap solution to interface existing spaghetti code that relies on global state with the Application/Injector.

But having a bit of boilerplate (we don't create new components often, and at the code review we can check that they are initialized properly), we would have a simple C++ syntax.

The existing mechanism is built in such a way that the individual components/classes precisely do not need any template/macro magic (unlike other such frameworks for C++). This mechanism pushes the complexity to this one single point - the Injector. It already requires a fair amount of boiler plate but I for one do not have a problem with non-critical boiler plate.

I see that with macro we don't need to write initializer, deinitializer and getter methods so it's an improvement.

It's not about not having to write them, still you have to write a lot of boiler plate; it's about the fact that the injector manages the lifecycle of the objects and therefore rules out a class of bugs. Consutruction and destruction is guaranteed to be correct - by construction.

What is the critical improvement it gives us that we should go with the macro and preprocessor logic over simple C++ syntax?

The lifecycle management I mentioned in the paragraph above. I for one do consider the Injector (Note: I refer to the thing in injector.h) simple C++ syntax. It is simple, it is declarative, it prevents certain errors from happening. It is extensible (for example as in #720) and does make things uniform without having to adhere to certain guidelines.

Another thing which I for one very much appreciate about declarative stuff is that it limits/constrains people from what they can do; by construction it has to fit into the "framework". Nobody can simply slap something in the initialization/destruction sequence which makes things go south.

scravy commented 5 years ago

The existing mechanism also makes changes more easily doable and in my opinion more easily reviewable, precisely because it is declarative. An application class like mentioned would have to be reordered when the dependency graph changes. The injector does not have to be.

scravy commented 5 years ago

Another thing: #721 and #618 demonstrate how the lookup via GetComponent<typename> directly relates to the type. This is lost with Application::GetComponentByName. The Application would require change, e.g. if you had finalization::StateRepository and snapshot::StateRepository and one was introduced after the other which might already occupy StateRepository. Now more things need to change, i.e. rename StateRepository to FinalizationStateRepository (if it wasn't named like that already) and introduce the new thing as SnapshotStateRepository. Or take Params (like we have snapshot::Params.

Then again in my mind each class should have a unique name across a project, no matter the namespace, but that's a separate discussion.