search

AltBeacon / windows-beacon-library

Tools for working with beacons on Windows
Apache License 2.0
33 stars 14 forks source link

Project Type Update & Logging Interface #3

Open furkanvarol opened 9 years ago

furkanvarol commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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

davidgyoung commented 9 years ago

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 commented 9 years ago

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.