[Workman-devel] [PATCH] WorkmanPlugin: make it a public class

Josh Poimboeuf jpoimboe at redhat.com
Tue Apr 16 15:29:05 UTC 2013


On 04/16/2013 08:50 AM, Daniel P. Berrange wrote:
> On Tue, Apr 02, 2013 at 03:23:22PM -0500, Josh Poimboeuf wrote:
>> Make WorkmanPlugin a public class so that the GObject introspection
>> compiler won't complain when we add a WorkmanPlugin as a property of
>> WorkmanObject.
>> ---
>>   workman/Makefile.am      |  2 +-
>>   workman/libworkman.sym   |  2 ++
>>   workman/workman-plugin.c | 21 ++++++++++++++++++++-
>>   workman/workman.h        |  1 +
>>   4 files changed, 24 insertions(+), 2 deletions(-)
>
> Hmm, this is something I'm really not at all happy with from
> an API architecture POV, because it is causing implementation
> details to leak out into the application visible interfaces.

We have full control of the API, so we can hide whatever we want pretty
easily.  In this case we could easily make the object's plugin property
private by replacing it with a private get/set_plugin interface.


> IMHO the root cause of the problem are the changes which have
> turned workman-object/consumer/partition into concrete classes.
> Originally I had them designed as abstract classes, which just
> define the API contract. The glue to the plugin objects would
> thus be in subclasses, thus none of the impl would be able to
> leak out to the callers. Given the problems highlighted by
> this & the next patch, I think we need to move back towards
> the original design I had. Make object/consumer/partition be
> abtracct and have plugin-consumer/plugin-partition as subclasses
> which provide the glue to the plugin.

In my experience, multiple levels of inheritance can bring their own set
of problems, including tightly coupled interfaces, leaky abstractions,
and exponential class growth, making the code much less flexible.  Which
is why I wanted to use composition instead of inheritance here.

And I have concerns about spreading each of the plugins' code across 2
separate classes: as a sub-subclass of WorkmanObject and as a subclass
of WorkmanPlugin.  Sounds like an explosion of classes, all of which are
tightly coupled to each other.  It seems really fragile and brittle to
me.

However, it's fine with me if you want to give it a shot at refactoring
or even rewriting any of the existing code.  I have no problem with
throwing away code if it results in something better.  And this is
basically your library, so you're the boss...

FYI, I'm working on the CLI now so we won't step on each other's toes.
Although I'll probably need to make a dummy plugin soon for testing, so
please keep me updated on your plans.


Josh




More information about the Workman-devel mailing list