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 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 2

For something like logging I would be inclined to use a Singleton like you do in the first example. The biggest arguments against it are testing (as the previous answer stated) and when you are likely to want to have more than one different logging "sink". Both of those can be worked around potentially by manipulating the logger in the test harness (e.g. resetting the state of it before every testcase). For scenarios where multiple sinks becomes important the singleton can still be applicable if you add some kind of "routing" decision process to the logger.

Answer 3

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 4

The approach to logging that I like best is to have a member of type Logger in my class (not a reference or pointer, but an actual object).
Depending on the logging infrastructure, that makes it possible to decide, on a per-class basis, where the output should go or which prefix to use.

This has the advantage over your second approach that you can't (accidentally) create a situation where members of the same class can not be easily identified as such in the logfiles.

Answer 5

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...)

Answer 6

I would stick with the first method and use the Logger as a singleton. Different sinks and identifying where data was logged from is a different problem altogether. Identifying the sink can be as simple or as complex as you want. For example (assuming Singleton<> is a base-class for singletons in your code):

class Logger : public Singleton<Logger>
{
public:
    void Log(const std::string& _sink, const std::string& _data);
};

Your class:

class FileNameLoader
{
 public:
     virtual bool LoadFileNames(PluginLoader& _pluginLoader)
     {
         Logger.getSingleton().Log("FileNameLoader", "loading xyz");
     };

     virtual ~FileNameLoader(){}
};

You can have an inherently complex Log Manager with different sinks, different log-levels different outputs. Your Log() method on the log manager should support simple logging as described above, and then you can allow for more complex examples. For debugging purposes, for example, you could define different outputs for different sinks as well as having a combined log.

Answer 7

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.]