Re: [Vala] Issues will vala and pulse vapi

[Al Thomas <astavale yahoo co uk>]

> From: Aaron Paden <aaronbpaden gmail com>

> Sent: Sunday, 22 November 2015, 2:48
> Subject: Re: [Vala] Issues will vala and pulse vapi
> >  From an object oriented design point of view your SinkSoureList
> >  class is doing too much. Initialisation should be done elsewhere
> >  and I think it is unusual to have GLib.MainLoop within a class.
> >  Some refactoring would be helpful here to see the problem more
>
> Well I've taken a few classes but I'm definitely at the level of a
> hobbyist, so I have lots of gaps in my understanding of best
> practices. SinkSourceList's purpose is simply to hold a list of pulse
> sources/sinks regardless of what the rest of the application is or

> isn't doing. 


Sounds very reasonable, although I wonder if SinkList and SourceList
should be separate classes. 


The class you are creating is a wrapper around the asynchronous 

PulseAudio API that blocks until the complete list is returned 

or a timeout occurs. It's essentially a snapshot that you can 

then iterate over later. I hope that's a fair summary.

You could think about using a Vala signal instead. Other objects
can hook into this to have a function called when the list is
ready. This is the Vala implementation of the observer pattern.


> It allows you to pass a MainContext to the constructor,> an only creates a MainLoop if MainContext is null. 
> For example, you
> might use SinkSourceList to write a one off utility like pactl list
> short sources. In that case, the application logic really doesn't need
> a mainloop, and the mainloop becomes an implementation detail of pulse

> that can be abstracted away.

> From what I can see creating the list is dependent on a PulseAudio.Context.
I think it would be better to pass an instance of that in the constructor.
Then your initialize_sources method goes, but the code within it becomes
the constructor code block.

You can then create a factory function that returns a PulseAudio context
created in a way that fits your rules. So your current constructor and
initialize_pulse method are taken out of the current class completely
to create a separate factory function. This way you can re-use the context
in other objects you create later, instead of duplicating that code for
each object.

I hope that makes sense of what I meant when I said your current 

class does too much.

> That was my reasoning, anyway. Could be completely faulty, and I'd
> love to be corrected if I've obviously doing something incorrect.


Obviously there isn't a single right way, but generally I find the
SOLID principles helpful when reviewing code. In this case the 'S'
of SOLID applies - the Single Responsibility Principle. You want
your object to create the list, not also set up the PulseAudio context.

All the best with it,

Al