I'm on a project where the team has defined a custom exception type, in the constructor of which is a call to a Logging method which logs the exception passed into the constructor.
I would have thought that this was bad - is it?
The problem is that whilst I could remove the "self logging" I don't know how much people are relying on the logging being in place there.
The C++ community debated the proper way to handle exceptions during construction for ever. The problem is that if you are throwing something in the constructor, it should mean that you were unable to construct the object. If instead, the code is just noting that things were suboptimal, but construction continued, that should be done in some other way. That would invoke the Don't Use Exceptions for Flow of Control antipattern.
I would say it's probably bad. It reminds me of some code I read that a coworker wrote a while ago - in a public helper method, instead of throwing an exception, they did a MessageBox.Show() with an error message. That was pretty bad for several reasons, one of those being that I wanted to use the method without showing a stupid error to the user.
Personally, I like to have control over whether a method logs information or not (for the most part). What if I want to handle the exception in a way that doesn't clutter my log files?
On the other hand, maybe the person who wrote the code designed it this way on purpose, to make sure other programmers didn't get away with NOT logging when they should. It depends on the scenario.
I personally don't like it because it's a separation of concerns problem - the logging should probably be decoupled from the task that the method is trying to perform. Unless logging is an essential part of that task. And for certain exceptions, the logging may very well be important.
I think it is bad. Why not use unhandled exception eventhandler to catch them all and do the proper loggings there? The way you describe it it violates the Single Responsibility Principle (SRP), and also remember that exceptions are serializable, so for example if you generate an exception in a service context (e.g. WCF Service) and then transfer it to your client (a web/desktop application) where does the logging should occur? Questions/problems like this tells me it is best to separate logging from exception and keep them separate.
