Review Component Based Architecture API

Started by
1 comment, last by duckflock 8 years, 2 months ago

I have written a simple Component Based Architecture API in C++. It verges on Event-Drive Architecture.

The objective is simply to create a Component Based Architecture API that follows C++11 practices (with some C++14) and doesn't use any bad C++ practices or no-nos.

The API can be found including a working example. Its very simple and easy to comprehend in my opinion. The API only has 5 main classes.

I'm hoping people could be kind enough to review the code. I am looking for criticism and advice regarding:

  • Any bad c++ practices and no-no's
  • The API relies on void* as a way to pass generic structures. Is there a better alternative/pattern?
  • How to ensure classes that inherit from IApp can only be instantiated through IApp::create<type>() and not directly through the constructor
  • Is it thread-safe? What ways could this be optimised?
  • I recently ran into the 'static initialisation order fiasco', are there any other problems I have implemented that I don't know about?

*Note: Some code is cross-platform but the main objective of the API isn't to be cross-platform. The Win32 example components are (obviously) quite coupled to OS. The objective of the API isn't to be released or be an alternative to existing API's just an exercise to use C++, improve coding skills and fulfil a code boner I've always had for CBA.

The main structure is like so:


// Event Delegate //
#pragma message("TODO: Similar to WinForms passing event parameters as a void pointer. Any ideas of a better solution that maintains polymorphism?")
typedef std::weak_ptr<void> EventArgs;
const EventArgs NULL_ARGS;

class EventDelegate
{
public:
	typedef std::function<Status(EventArgs)> EDelegate;

	EventDelegate(EDelegate delegate, GUID gUidContext);

	bool operator== (const EventDelegate& other) const;
	bool operator!= (const EventDelegate& other) const;

	Status operator()(const EventArgs& evtArgs) const;

private:
	GUID gUidContext;
	EDelegate delegate;
};


// Component //
class Component
{
public:
	GUID gUid;

	template<typename T>
	static std::unique_ptr<T> create(std::weak_ptr<IApp> app)
	{
		return std::unique_ptr<T>(new T(app));
	}

	virtual ~Component();

	bool operator< (const Component& other) const;

	template<typename T>
	bool addComponent()
	{
		std::unique_ptr<T> cmp = Component::create<T>(app);
		auto res = components.emplace(cmp->gUid, std::move(cmp));

		return res.second;
	}

protected:
	std::weak_ptr<IApp> app;
	std::unordered_map<GUID, std::unique_ptr<Component>> components;

	Component(std::weak_ptr<IApp> app);
	Component();

	virtual Status init(const EventArgs& evtArgs) = 0;
	virtual Status terminate(const EventArgs& evtArgs) = 0;
	virtual Status registerEvents() = 0;
};


// IApp //
class IApp : protected Component
{
public:

	static Status S_APP_EXIT;
	static Status S_UNREGISTERED_EVT;
	static const int EVT_MOUSE = 50000;
	static const int EVT_KEY = 50001;
	static const int EVT_INIT = 50002;
	static const int EVT_TERMINATE = 50003;
	static const int EVT_EXIT = 50004;

	template<typename T>
	static std::shared_ptr<T> create()
	{
		std::shared_ptr<T> app(new T);
		app->app = app;
		return app;
	}

	Status exec();
	Status registerForEvent(const int& eventId, EventDelegate delegate);
	Status unregisterForEvent(const int& eventId, EventDelegate delegate);

protected:
	static std::unordered_multimap<int, EventDelegate> evtRegistry;

	IApp();
	virtual ~IApp();

	virtual Status update() = 0;
	static Status eventHandler(const int& evtId, const EventArgs& evtArgs);

private:

};
Advertisement
  • The API relies on void* as a way to pass generic structures. Is there a better alternative/pattern?

Typedef it to something readable if it going to be used as an API

  • How to ensure classes that inherit from IApp can only be instantiated through IApp::create<type>() and not directly through the constructor

Make the constructor private

I'm hoping people could be kind enough to review the code. I am looking for criticism and advice regarding:

1) Any bad c++ practices and no-no's
2) The API relies on void* as a way to pass generic structures. Is there a better alternative/pattern?
3) How to ensure classes that inherit from IApp can only be instantiated through IApp::create() and not directly through the constructor
4) Is it thread-safe? What ways could this be optimised?
5) I recently ran into the 'static initialisation order fiasco', are there any other problems I have implemented that I don't know about?

*Note: Some code is cross-platform but the main objective of the API isn't to be cross-platform. The Win32 example components are (obviously) quite coupled to OS. The objective of the API isn't to be released or be an alternative to existing API's just an exercise to use C++, improve coding skills and fulfil a code boner I've always had for CBA.

Alright, I'll bite *chomp*:

1)

- You're developing a solution without a problem. In science that is a nice way to publish lots and lots, but nevertheless completely useless. That's a huge no-no wink.png

- There is no documentation. How am I supposed to know how to use this? Do I derive from Component or IApp? IApp isn't even an interface, why is it named as one?

- Your example shows that you don't solve a problem. Why should I use a Component-derived Window? Why not a simple Window class?

- Who owns anything? Right now you're forcing me to use shared_ptr with built-in new to create components.

One of the main uses of Components in these Component-based architectures (I think this term is grossly overused. Everything is based on Components or it would be unreadable Spaghetti code) is to be able to effectively pool data structures by usage patterns. In that sense, Component-based architectures are close to the nowadays so common and heavily discussed System-Component architectures, which are a prime example of data oriented design. This may come off as an angry rant, but nevertheless: IMHO most of the time "Components" are total overkill -- see your example. If you want to listen to and dispatch events, write an EventListener/EventDispatcher system. Your ownership problems come to light the moment you're asking "who should call terminate?"

2)

You could use use boost::any and/or boost::variant. Alternatively, you could use a base class with an integer to hold its type to enable run-time type safety, similar to this:


enum EventTypes {
  EVENT_FOO,
  EVENT_BAR,
  EVENT_BOO,
  ...
};

struct EventBase {
  EventBase(int type);
  int type();
private:
  int type_;
};

struct EventFoo : EventBase {
  EventFoo() : EventBase(EVENT_FOO) { }
};

You could provide a checked_cast<T> template in EventBase that makes sure the cast is correct at runtime. I leave this as an exercise to the reader tongue.png

3)

You can require a custom struct instance with a private friend constructor in IApp's constructor, like so:


class Foo {
public:
  struct Bar {
    private:
      Bar() { }
      friend class Foo;
  };
  Foo(const Bar& bar);

  template<typename T, typename... Args>
  static std::shared_ptr<T> Create(Args&&... args) {
    return std::make_shared<T>(bar_, std::forward<Args>(args)...);
  }
private:
  static const Bar bar_;
};

class Derived: public Foo {
public:
  Derived(const Foo::Bar& bar, int a, int b) : Foo(bar) {
    // ...
  }
};



This forces Derived to take a parameter to Bar, which necessarily routes any instantiation through Foo::Create. Alternatively, simply make Derived's constructor a private friend of Foo.

4) I haven't taken a deep look, so I don't know. I highly doubt that registerForEvent/unregisterForEvent is thread safe. Never assume something is thread safe by default. Never. shared_ptr does not make anything thread safe automatically. "What ways could this be optimised?" -- optimised with respect to which use case? Thread safety? smile.png

5) Some thoughts:

* You're violating the Rule of Five: https://en.wikipedia.org/wiki/Rule_of_three_%28C%2B%2B_programming%29#Rule_of_Five

* You're using std::weak_ptr for event arguments. I assume I trigger an event by pushing the event delegate somewhere and calling it later (that's the only case where using shared ownership makes sense at all), why do I need to keep track of a shared_ptr object until the event executes? I need to keep track of 2 things this way: the moment the event fires and the data it will use. I don't care about any of these two. That's why I call a delayed event anyone could handle in the first place! A shared_ptr seems appropriate here, because calling an event without its data does not make sense.

* What's up with the weak_ptr to IApp in Component? I can't really make sense of this. Why does a Component need to know something about its implementation (I guess)? Why a weak_ptr?

* shared_ptr/weak_ptr are not used as often as you think. There is nothing wrong with storing a raw pointer! Using a raw pointer says "hey, I need that object, but I do not own it, so make sure it lives as long as I need it" to me. shared_ptr is required very rarely. Most of the time, unique_ptr suffices. As a bonus it is very explicit about object lifetime, which is a good thing.

* You're very allocation heavy. Most of the time this is of no concern and certainly shouldn't be to learning programmers. I just mention it because I feel that one of the advantages of Component-System architectures is to organize memory ownership and locality. Basic shared_ptr usage does nothing of that and potentially hides problems.

* A further note on thread safety: thread safety is very, very hard. Very hard. happy.png

To conclude: Most of your problems stem from the fact that you have no real use case. I can't find the problem you're trying to solve. I strongly encourage you to use your code to do something real. I know that learning new stuff is fun and exciting. I think so, too. I have also realized that designing clean interfaces without a concrete use-case is nigh impossible. I feel that the hardest part of architecturing code is ownership. Object cleanup sequences have bitten me several times in unexpected and obscure ways. That's why I think purpose is a very important concept. Code without purpose is just a bunch of bytes lying around waiting to be deleted.

Also, comment your code if you want others to read it. As you can probably see, I have only skimmed over your code and interface and did not understand as much as I probably should have smile.png

This topic is closed to new replies.

Advertisement