Is it better to take object argument or use member object?

Question

I have a class which I can write like this:

class FileNameLoader
{
     public:
         virtual bool LoadFileNames(PluginLoader&) = 0;
         virtual ~FileNameLoader(){}
};

Or this:

class FileNameLoader
{
     public:
         virtual bool LoadFileNames(PluginLoader&, Logger&) = 0;
         virtual ~FileNameLoader(){}
};

The first one assumes that there is a member Logger& in the implementation of FileNameLoader. The second one does not. However, I have some classes which have a lot of methods which internally use Logger. So the second method would make me write more code in that case. Logger is a singleton for the moment. My guess is that it will remain that way. What is the more 'beautiful' of the two and why? What is the usual practice?

EDIT: What if this class was not named Logger? :). I have a Builder also. How about then?

Answer 1

I don't see what extra advantage approach two has over one (even considering unit testing!), infact with two, you have to ensure that everywhere you call a particular method, a Logger is available to pass in - and that could make things complicated...

Once you construct an object with the logger, do you really see the need to change it? If not, why bother with approach two?

Answer 2

I prefer the second method as it allows for more robust black box testing. Also it makes the interface of the function clearer (the fact that it uses such a Logger object).

Answer 3

The first thing is to be sure that the Logger dependency is being provided by the user in either case. Presumably in the first case, the constructor for FileNameLoader takes a Logger& parameter?

In no case would I, under any circumstances, make the Logger a Singleton. Never, not ever, no way, no how. It's either an injected dependency, or else have a Log free function, or if you absolutely must, use a global reference to a std::ostream object as your universal default logger. A Singleton Logger class is a way of creating hurdles to testing, for absolutely no practical benefit. So what if some program does create two Logger objects? Why is that even bad, let alone worth creating trouble for yourself in order to prevent? One of the first things I find myself doing, in any sophisticated logging system, is creating a PrefixLogger which implements the Logger interface but prints a specified string at the start of all messages, to show some context. Singleton is incompatible with with this kind of dynamic flexibility.

The second thing, then, is to ask whether users are going to want to have a single FileNameLoader, and call LoadFileNames on it several times, with one logger the first time and another logger the second time.

If so, then you definitely want a Logger parameter to the function call, because an accessor to change the current Logger is (a) not a great API, and (b) impossible with a reference member anyway: you'd have to change to a pointer. You could perhaps make the logger parameter a pointer with a default value of 0, though, with 0 meaning "use the member variable". That would allow uses where the users initial setup code knows and cares about logging, but then that code hands the FileNameLoader object off to some other code that will call LoadFileNames, but doesn't know or care about logging.

If not, then the Logger dependency is an invariant for each instance of the class, and using a member variable is fine. I always worry slightly about reference member variables, but for reasons unrelated to this choice.

[Edit regarding the Builder: I think you can pretty much search and replace in my answer and it still holds. The crucial difference is whether "the Builder used by this FileNameLoader object" is invariant for a given object, or whether "the Builder used in the call" is something that callers to LoadFileNames need to configure on a per-call basis.

I might be slightly less adamant that Builder should not be a Singleton. Slightly. Might.]

Answer 4

In general I think less arguments equals better function. Typically, the more arguments a function has, the more "common" the function tends to become - this, in turn, can lead to large complicated functions that try to do everything.

Under the assumption that the Logger interface is for tracing, in this case I doubt the the user of the FileNameLoader class really wants to be concerned with providing the particular logging instance that should be used.

You can also probably apply the Law of Demeter as an argument against providing the logging instance on a function call.

Of course there will be specific times where this isn't appropriate. General examples might be:

• For performance (should only be done after identification of specific performance issues).
• To aid testing through mock objects (In this case I think a constructor is a more appropriate location, for logging remaining a singleton is probably a better option...)