On Fri, 2010-08-20 at 12:44 -0600, Alex Rousskov wrote: > On 08/20/2010 11:06 AM, Andrew Beverley wrote: > > On Fri, 2010-08-13 at 18:19 -0600, Alex Rousskov wrote: > >> On 08/11/2010 03:25 PM, Andrew Beverley wrote: > >> > >>> I've moved these, as well as most of the other QOS functions, into > >>> Ip::Qos. I have also removed the QosConfig namespace, as it didn't seem > >>> to fit with all these additional functions. > >> > >> * A patch preamble with the proposed commit message would be nice. > >> > >> * I am not sure what Qos class is. It is not documented. If it is a "QOS > >> configuration" class, I understand why we have a global instance of it, > >> but the original QosConfig name seems better in that case. If it is not > >> a configuration class, then I am not sure why we have a global instance > >> of it. And the QosConfig file name does not seem to match the class name > >> any more. > >> > >> Perhaps we need two classes, one for configuration and one for > >> manipulation? Or a Qos namespace with a configuration class and global > >> manipulation functions? The latter seems more likely. > >> > > > > I was trying to copy the approach used by the Interceptor class, which > > has its configuration variables and methods in one class, with a global > > instance of it. I thought that it would keep things nice and tidy by > > having all qos aspects in one single class (with the configuration > > variables within private space). > > Trust me, I feel your pain. Copying existing code on its own does not > guarantee much because a lot of code is broken, unfortunately. [ I am > not saying Interceptor class is or is not broken because I have not > reviewed it. ] > > > > I'm happy to go with whatever approach you want though, and can separate > > the config aspects back out from the functions. > > > > Before I go ahead and do this, I want to check which option I should go > > for. As Alex says, the options are: > > > > 1) > > - Reinstate the QosConfig class with the configuration variables as > > public members > > - Create a new class (QosFunctions?) with the relevant functions in > > - Create one global instance of each class, > > Rule of thumb: If you can use a namespace instead of a class, use a > namespace. For example, classes that only group related stateless > functions are usually a bad idea. > > > > or alternatively create a > > new instance of the functions class each time the functions need to be > > accessed (is the latter inefficient?) > > It is difficult for me to imagine a worse design (one class/instance per > function, perhaps?), but I am sure you can find it in Squid :-). > > > > 2) > > - Reinstate the QosConfig class with the configuration variables as > > public members > > - Create the functions as global functions in the namespace > > This is the best option if functions do not share or keep state. > > Please note that QosConfig should become Qos::Config once you add Qos > namespace. > > > > 3) > > - Keep everything in one class :-) > > > > > > If going with option 2, the functions can no longer be const (which most > > of them currently are). > > Well, if your methods do not share or keep state, they should not be > const. They should be static! Const means "I use but do not modify the > state of my object". And if a class has nothing but static methods, it > should be a namespace in most cases... > > > In summary, > > * Work inside Qos namespace. > > * Keep configuration state (data) inside Qos::Config class. You can use > a single global Qos::TheConfig "current configuration" instance of that > class if needed. > > * Functions that exist to interpret or modify configuration should be > Qos::Config methods (const if possible). > > * Functions that wrap library calls or otherwise manipulate some > external state/data that they do not and cannot own should be declared > outside Qos::Config and inside Qos. > > * If there is a non-configuration state/data that you can encapsulate > and manipulate in a class setting, then create more Qos::* classes.
Thanks Alex. An excellent explanation. I fully understand now, and have gone with option 2 as you suggest, just keeping the isActive functions in the config class. Updated patch to follow soon. Andy
