Project Type Update & Logging Interface

[furkanvarol]

As last we talked in #2, I have changed project type from Portable Class Library to Class Library. Also I have dropped .Net 4.5 to 4.0 since it is enough for this project.

Also, I have implemented Logging package like in Android Library but I didn't have time to adapt NLog for this interface but I will do it as soon as possible.

[davidgyoung]

All of the debug logs inside the parser probably need to be wrapped in an if statement that checks to see if debug is enabled before constructing the string. It is really important to do that here because this code will run hundreds of times per second when lots of bluetooth devices are around.

[furkanvarol]

Hmm, then the flag name "VerboseLoggingEnabled" must be changed to "BeaconParserLoggingEnabled", right ?

[davidgyoung]

No, I don't think that is necessary. If you have VerboseLoggingEnabled set to false, lines like Logger.Debug("cannot parse term " + term); will do nothing anyway. All you are accomplishing by wrapping these in if statements like:

if (LogManager.VerboseLoggingEnabled) {
  Logger.Debug("cannot parse term " + term);
}

is making it so the string object construction and concatenation does not take place unnecessarily, which saves processing time. Even if you don't do this everywhere, doing it inside beacon parsing is important because it is gets executed in a tight loop.

[furkanvarol]

Btw, I can write add LogManager.VerboseLoggingEnabled condition to every logger so that they wont log anything like it supposed to do and also if I use logger with params like Logger.Debug("cannot parse term {0}", term);, log string won't even get built (but of course in this case it will change nothing since there is only one param).

Also, this is a debug log not a verbose that is why I asked to change name of the flag since VerboseLoggingEnabled kind of misleading for disabling debug logs.

[davidgyoung]

The flag could change to DebugLoggingEnabled, yes.

[furkanvarol]

Okay, I will change the flag name. So what about the other thing that I told you above ? Does it sounds good or not even necessary ?

Also, what about the comments below ?

[furkanvarol]

I have wrapped this log with condition and renamed VerboseLoggingEnabled to DebugLoggingEnabled

[davidgyoung]

I don't think you have to "add LogManager.VerboseLoggingEnabled condition to every logger". It is only really necessary inside the parser where performance counts.

[furkanvarol]

Okay, I have only wrapped one log which is proposed by you above.

[davidgyoung]

Are the factory classes really helpful? Since each one just instantiates a single class, it seems like unnecessary code vs. just calling the constructor directly. What advantage do these give?

[furkanvarol]

Unlike Android logger interface, NLog and such libraries requires that each class will have it own logger therefore name of the class (TAG aka in Android) is given at its constructor. So, when a client of LogManager requires a logger, it needs to give a unique (or cached logger with same name) logger instance for that client.

Actually, I agree with you. With factory classes logging interface of this library has became a mess :) even though it only has one method but I couldn't think a nice way to get rid of it. If you have any idea I would love to hear it and implement it.

Also, there one major flaw about this design. Unlike logging interface in Android beacon library, in windows library changing logger factory in LogManager won't affect the classes which already took a logger from LogManager. And this flaw makes logging configuration hard. Client must set logger factory at the very beginning of its code (or at least before using any class of this library). However there is way to solve this problem but not that beautiful :). Anyway, I also believe this is not a big problem in general since most of the loggers requires configuration beforehand.

Btw, sorry for late response, I was out of town.

[furkanvarol]

Hey @davidgyoung, I am still waiting for your response. I am kind of stucked here :)

[davidgyoung]

Maybe I don't understand. Why do we need to have a ILoggerFactory getter/setter on BeaconManager at all? Why can't we just have a ILogger getter/setter? You could also add a convenience method for setting a warn, debug, empty, logger that takes no parameters and constructs one when the method is called.

[furkanvarol]

Actually, I do not want any factory method either but most of the logger librarier (except Android builtin logger) requires a unique logger instance per class which is requires a factory. You can check following links to see that every library has one:

https://github.com/NLog/NLog/wiki/Tutorial
http://www.mkyong.com/logging/log4j-hello-world-example/

But like I said, I do not like it either. Since this library should focus on beacon scaning, loggers should not be a mess. Hovewer, I could not think a way to create a unique instance per class without a factory.
Also, like I said before, this approach has one flaw (which is not a great one and I think every logger library has this) and that makes clients to configure logger before doing anything else.

In short, I like to remove them but I have no idea to do it without using reflection which actually I don't want to use.

Btw, I can just add a setter/getter for ILogger but that will make that all library have to use one instance for all logging operations which is not a good practice.