C# Tricks for NPVR - Comments

[mvallevand]

Rather then littering Iain's original thread was endless posts and comments on the "tricks", I created this thread as a the garbage can, allowing people to follow up.

Hmm, it's time I retired so I won't go into details about why having empty catch blocks or a 'catch all' block are really bad ideas. Not good practices to teach people based on Iain's opening statement and the spirit of this thread.

Cheers,
Brian

Hi Brian,

As a self-taught C# programmer, who never used it before creating the original WebRadio plugin for PVRx2 could you explain why it is a bad idea as a self-improvement tool. I probably have lots of bad practices and I do want to improve.

In my code I would say I don't trap 95% of the individual commands that could return an error, I group them, since I try and structure my code with one return per function and many catches with lots of ifs would make it almost unmaintainable. When I do a trap most of the time it is just a Logger.Debug(e.Message) which hardly impacts the program flow. Isn't that really the same as an empty catch? I did adopt my own notation of catch {} on one line to show that it was empty so that I wouldn't accidentally think the following non-indented code was the catch.

In the example I gave http://forums.nextpvr.com/showthread.php...post430834, both precdeding lines could return exceptions and I really should have been looking for a 404 etc, but ultimately what I am concerned about is the string returned is valid xml, and when the doc.Load(strContent) fails in the next line I do handle that.

Martin

[bgowland]

I think a misconception when first working with a language that uses exceptions is that all exceptions are errors that just need to be caught and logged or, if they're obscure and unlikely to impact code functionality, can simply be ignored with an empty catch block. I have to admit it took me quite a while to really understand the power of exceptions and the way they effectively allow realtime / runtime monitoring of 'state'.

One very simple example would be a FileNotFoundException when attempting to open a plugin 'settings' file. Assuming a plugin has a set of default settings, perhaps in a settings-master-do-not-edit.xml (for example) it is better to have a local try/catch around the line of code which attempts to open the file than an all-enclosing one around all of the code which simply traps the 'Exception' type. With the latter, the exception is caught but in a place where it's too late to recover. With an in-place handler, it's possible to log the exception but to then create a new copy of the settings file from the master and proceed without disruption to the user.

I admit that, like you, I do use a single try/catch for type 'Exception' in many places but usually only in methods with a few lines of code after I've reviewed the possible exceptions that can be thrown, and usually only if those exceptions are obscure and unlikely to occur in most cases (IO exceptions due to faulty disk hardware or corrupt sectors, for example). In that sort of case, trying to recover the situation in-place is possibly unlikely to work and perhaps isn't even a good thing to attempt.

As for empty catch blocks, the general point of any code throwing an exception is at the very least to report a particular condition (error or informational) and it makes sense to at least log the exception message. Admittedly a plugin developer may never see 99.99% of end-users' log files, but when a user does upload a log file to a support forum, the presence of those 'informational' exception logs (even if harmless) can give the developer information about how the user's system, or use of the plugin, differs from their own. I've had situations where I've written code where certain exceptions would never be thrown on my own system but have seen them in log files from users and adapted my code as a result.

I agree that the more in-place handlers there are in code can make it look pretty ugly and more difficult to manage but it's a case of striking a good balance - identify which exceptions are most likely to be thrown (if possible) and, in particular, which ones can be recovered from. Having a 'catch all' as a last resort is a good idea but if all it can do is say "Something unexpected happened, that's all folks!", it's not very helpful. In-place handlers, where possible, can be far more productive.

Cheers,
Brian

[mvallevand]

Thanks, Brian. Originally I thought that I was doing something structurally wrong that could lead to corrupting the stack etc, and I am glad it is not that. It has given me a couple of hours of good times reading about empty catches on the "Net" and I can see it is generally severally frowned upon. I know I had and still have sound rationale for how I wrote the code, (definitely not wanting to log failures that will generate posts with "I found an object not found error in the log...") and I would do it again for my code, as an example this is not a good idea.

However typical examples (without the log message) often imply that exceptions need to be trapped so some understanding of error trapping knowledge is going to be assumed in most snippets.

Code:

WebRequest objRequest = WebRequest.Create(streamURL);
                WebResponse objResponse = objRequest.GetResponse();
                StreamReader sr = new StreamReader(objResponse.GetResponseStream(), System.Text.Encoding.UTF8);
                string strContent = sr.ReadToEnd();

Martin

[bgowland]

Thanks, Brian. Originally I thought that I was doing something structurally wrong that could lead to corrupting the stack etc, and I am glad it is not that.

In theory no single uncaught exception should do anything too detrimental but can certainly cause a state of confusion, i.e., a state where following code is assuming all is well when in fact it's not. Lack of exception handling can also result in an avalanche or domino effect depending on how deep the code structure is as the levels of code assuming 'all is well' attempt to proceed.

It has given me a couple of hours of good times reading about empty catches on the "Net" and I can see it is generally severally frowned upon. I know I had and still have sound rationale for how I wrote the code, (definitely not wanting to log failures that will generate posts with "I found an object not found error in the log...") and I would do it again for my code, as an example this is not a good idea.

I think when wanting to bypass / ignore an exception that we as developers probably know won't impact the operation, it's still a good idea in most cases to at least log the situation. It isn't necessary to dump the stack trace in a way that would alarm a curious user but something along the lines of "Plan A didn't work, resorting to Plan B" (worded in whatever way is least alarming to an end-user but informational to the developer if they happen to see the logs).

However typical examples (without the log message) often imply that exceptions need to be trapped so some understanding of error trapping knowledge is going to be assumed in most snippets.

Yes and no. On the one hand it's definitely better if a code snippet doesn't promote the use of an empty catch block. On the other, if providing a snippet using code which may be problematic, then a short-hand approach is at least a good reminder to people to think about what my go wrong. I generally use ellipses

Code:

try
{
    // Try something
}
catch (...)
{
    ...
}

I find VS as an IDE is particularly good at allowing people to knock together code using the Intellisense approach for showing possible methods/ properties for a class, but it's particularly bad at enforcing exception handling. Using Eclipse for Java development, the damn thing won't even let me build my code sometimes unless I've put the right handlers (catch blocks) in place. Very annoying but it at least makes me stop and think about the possible outcome of each stage. It's not a perfect solution and never will be as it comes down to the developer's approach to coding but if VS did that it might do developers and end-users a favour.

Cheers,
Brian