+972 52-548-6969
ReviewMicrosoft N Layer App Sample, Part III–Abstraction is as abstraction does
Continuing my review of http://microsoftnlayerapp.codeplex.com/, an Official Guidance (shows up at: http://msdn.microsoft.com/es-es/architecture/en) which people are expected to read and follow.
This quite annoys me:
Why is it annoying?
Because of the author note, it implies that you can switch the implementation to something else. Except that you really can’t.
Take a look at the actual interface, what does it means TraceStop, TraceStart, TraceStartLogicalOperation, TraceStopLogicalOperation? Well, they are all about expose functionality that exists only for the System.Diagnostics stuff.
Too add to the gravity of the situation, there is no where in the code that is even using those methods!
And there there is the actual usage of the trace manager:
No, before that, even, there is the actual name. ITraceManager? Because ILogger or something like that wasn’t important enough? What, pray, does implementer of this interface are going to manage? There is no managing, so there shouldn’t be a manager.
And there there is the actual usage of the trace manager:
Well, let us imagine that we wanted to read the logs of this application. We can’t, because the application, while having a log, is actively hiding important information. Namely, the full exception (including all the inner exceptions and stack trace).
But we will ignore that as well, and say that we want to move to log4net. Guess what, you can’t! Well, you can if you want to get a flat log, but one of the most important features of the log4net is the idea of loggers, that you can dispatch some messages based on their origin to different location (for example, color them differently when writing to the console).
You can’t do that, there is no way of saying what the origin is of anything here.
I’ll touch the “IoC” stuff in a future post, I think that this is enough for now.
Comments
What annoys me the most is that he's -not- using NLog ;)
/me ducks, weaves and hides.
Ayende - u also didn't mention ELMAH in that post. First thing to do when u first add an MVC app (because webforms SHOULD be dead - don't deny it folks) is to NuGet ELMAH & your fav IOC.
/me shivers at the day (if i get enough $$$) to get Ayende to code review my art.
@Ayende
Although it may seem heavy, I say it again you do not make sense (only if you are looking to harm) is to review fixed wrong code...
http://microsoftnlayerapp.codeplex.com/SourceControl/changeset/view/63507#1100076
http://microsoftnlayerapp.codeplex.com/SourceControl/changeset/view/63507#1100077
@Jhon But when was the changeset committed?? http://microsoftnlayerapp.codeplex.com/SourceControl/changeset/changes/63507 Committed On: Thu Jun 30 2011 at 5:25 PM
Ayende had this post sat on the queue way before then!
The first two arguments with some sense. However, they are already fixed in version 2. I do not find too much weight on them to make as much noise
I love the way MS put out code to "show how it's done" - and they reference the IoC _all_ _over_ _the_ place
Anti-pattern HELL!!!
So... The team behind DDD has fixed a design error so quickly that this post is totally irrelevant... I sounds like it is usefull to keep an eye in a project with a team that is so resposible and when problems arise.
Regards.
That IoCFactory.Instance.CurrentContainer is scary as hell.... and actually, for a big part, it suffers from the same issues as the ITraceManager. What if I want to use Autofac and use some of the Resolve overloads?
@Iker
it's so riddled with nasty code it will forever be relevant.
Whilst I take the point that the issue has been fixed I find it incredible that it was even in there in the first place, this is simple stuff that is being done badly. Also looked at the "fixes", (and i've not looked elsewhere) but having ILogFactory is odd, isn't that what your container is for?! Maybe there is a valid reason but...
A bit off-topic, but I love the Future Posts countdown! It would be nice, though, if it would be smarter and say "on monday" instead of "3 days from now".
@Iker If the rest of Ayende's recommendations are as good as these and the team follows all of them, everyone wins.
Kash, I have contacted the team when I wrote those posts, and gave them early access to them. I am not really surprised that the issues that you see here are fixed, but I consider this to be more a case of the issues being fixed because they were found rather than being fixed independently.
I can somewhat understand why they are using the IOC container to retrieve the logger (Though I don't understand why you need a LogFactory class).
Logging, being a cross cutting concern, can be somewhat noisy if you are using constructor injection,
Setter injection is cleaner here, though a lot of people don't like setter injection in general.
I think the cleanest way to inject a log provider dependency is something along the lines of Springs aspects, though I don't know if there is anything in the .NET world that works like that.
@Jonathan Holland
Aop in .net is easy. its the fact that they are calling on the IOC directly. the class should not know anything about the IoC - there is no need for it.
More spaghetti mess from MS basically
My favorite thing so far is project Domain.Core. It overlaps with term core domain what Eric Evans is speaking about. Only - he means most important and valuable domain logic as opposed to boring (e.g. ISQL interface) but most referenced infrastructure stuff. I think that pretty much reflects sample project main problem.
I would keep error logging and tracing separate. As outlined here: Logging is not Tracing: http://geekswithblogs.net/akraus1/archive/2009/06/21/132968.aspx
The usage of a real tracing library is a must if you want to follow your application flow with automatic tracing of thrown exceptions. You do not want to turn this fully on or you will drown in GB of output but in case of sporadic errors you can enable it to get the full history.
http://geekswithblogs.net/akraus1/archive/2010/07/04/140761.aspx
Yours, Alois Kraus
The IOC usage is off-topic, though I am insterested in Oren's future comments on it. Frankly that usage isn't IOC, it's turned it into a Global Registry. I've had to fight against anti-pattern abuses of containers like this frequently. However, using a container as a registry isn't all bad. For legacy applications and setter injection it can be practical and elegant. But it does require discipline so that the container isn't making appearances "all over the _place_" as iwayneo points out.
Back on topic that logging implementation is complete crap. It doesn't abstract logging providers, and that they would have released code that merely logged an exception message is ridiculous. Granted it's not bound to be a primary detail in their architecture but if they cut corners on the small stuff it doesn't leave me very confident that the important stuff will be demonstrated well.
Frankly I'm sick of investigating new technologies and while looking for practical examples, finding nothing but trivial, non-relevant examples of how they can be implemented. So do bash away at their examples. Maybe, just maybe they will read it and actually provide a suitable sample for their next silver bullet.
Common.Logging ftw!
A tracing API lets you get metrics about your service, and in a way that is a lot more convenient than interpreting logs. It lets you profile in production, a bit, so you can alarm when your average latency is over threshold, or track the number of recoverable errors, or see what time of day your service is hit most often.
Trace data should always be graphable. It's rarely going to help you diagnose a problem, but might point one out to you.
The API they outline is pretty ugly for tracing and horrible for logging.
(And they've already changed it to be somewhat better.)
The 'manager' hell - turning every part of an application into a manager of something is one of the worst design mistakes one can make. If you consider naming sth like that, try to find out a few more alternatives, like ILogger in this example and prove that 'manager' is the best. In 99% you cannot and you should swap the name,
Comment preview