> From: Aaron Paden <aaronbpaden at 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
