 |
 | FxCop warnings
Jaap Faber | 2:49 3 Jul '07 |
|
|
Hi,
Thanks for this. Like other writers in the messages, I too am used to Delphi Asserts. And it was the first thing in C# I missed. The code presented here solves part of the problem. And a extra debug hidden tag helps a lot. But there is one problem for me: I like to use FxCop to check for (some) errors. One of them is the checking of parameters. But with the code presented here, FxCop fails to understand that parameter checking is just wat we are doing! Is there a way to combine the use FxCop and the code presented here?
Regards, Jaap.
|
|
|
|
|
| Nice, but...
Sander Leer | 3:56 28 Dec '04 |
|
|
I've used the same kind of library the last few years for my own projects, but I couldn't find a way to throw the exception from the correct source-code.
Currently an exception is thrown from the library methods instead of the method that is using this library. This is confusing for the programmer who's using your methods. Things get even worse when using the overloaded methods because they increase the length of the stack-trace.
Anyone knows how to throw an exception from a specific source location (in Delphi you can use raise object at address to raise an exception from an earlier point in the stack than the one where the error actually occurred).
|
|
|
|
|
|
|
I get your point but I don’t see how it makes much practical difference. To illustrate, if a class library uses Exceptions.cs to raise an argument exception, here’s what I see in my Call Stack window when an exception is thrown to my WinForms client:
- Umbrae.ClassLibrary1.dll!Umbrae.Exceptions.ThrowIfOutOfRangeIncludeMaxCore(System.IComparable value = {0}, string name = "positiveArgument", System.ValueType lowerBound = {0}, System.ValueType maxValue = {2147483647}) Line 2922
- Umbrae.ClassLibrary1.dll!Umbrae.Exceptions.ThrowIfOutOfRangeIncludeMax(int value = 0, string name = "positiveArgument", int lowerBound = 0, int maxValue = 2147483647) Line 2373
- Umbrae.ClassLibrary1.dll!Umbrae.ClassLibrary1.Class1.MyMethod(int positiveArgument = 0) Line 21
- Umbrae.WindowsApplication1.exe!Umbrae.WindowsApplication1.Form1.button1_Click(System.Object sender = {Text="button1"}, System.EventArgs e = {System.EventArgs}) Line 90
- [<Non-user Code>]
- Umbrae.WindowsApplication1.exe!Umbrae.WindowsApplication1.Form1.Main() Line 84
I don’t know what’s confusing or bothersome about that. Exceptions.Throw­If­Out­Of­Range­Include­Max­Core() and Exceptions.Throw­If­Out­Of­Range­Include­Max() seem to be self-explanatory as internal methods for checking argument validity. And if Umbrae.ClassLibrary.dll is referenced as a compiled assembly without source code, it won’t show up in the Call Stack anyway unless the developer chooses the “Show Non-user Code” preference.
Besides, this kind of somewhat extraneous information already shows up in the stack trace if you pass a bad argument, say, to System.Windows.Forms.Check­Box.OnPaint(): a null argument throws a Null­Reference­Exception with a stack trace that starts out:
- System.Windows.Forms.dll!System.Windows.Forms.CheckBox.PaintUp(System.Windows.Forms.PaintEventArgs e = <undefined value>, System.Windows.Forms.CheckState state = Unchecked) + 0x51 bytes
- System.Windows.Forms.dll!System.Windows.Forms.CheckBox.PaintOver(System.Windows.Forms.PaintEventArgs e = <undefined value>, System.Windows.Forms.CheckState state = Unchecked) + 0x48 bytes
- System.Windows.Forms.dll!System.Windows.Forms.CheckBox.PaintStandard(System.Windows.Forms.PaintEventArgs pevent = <undefined value> ) + 0x33 bytes
- System.Windows.Forms.dll!System.Windows.Forms.CheckBox.PaintControl(System.Windows.Forms.PaintEventArgs pevent = <undefined value> ) + 0x32 bytes
- System.Windows.Forms.dll!System.Windows.Forms.CheckBox.OnPaint(System.Windows.Forms.PaintEventArgs pevent = <undefined value> ) + 0x35 bytes
- Umbrae.WindowsApplication1.exe!Umbrae.WindowsApplication1.MyCheckBox.OnClick(System.EventArgs e = {System.EventArgs}) Line 21
- [...]
- Umbrae.WindowsApplication1.exe!Umbrae.WindowsApplication1.Form1.Main() Line 94
Note that I had to choose the “Show Non-user Code” preference to get the WinForms methods to show up in the Call Stack.
I say stop worrying about confusing developers in this way.
-- /\/. _/.
|
|
|
|
|
|
|
It makes a lot of difference when there's an error in Class1.MyMethod and the stack trace show there's an exception thrown from Exceptions.ThrowIfOutOfRangeIncludeMaxCore. To an average programmer this is confusing; because they are not used to look halfway down the stack trace to see were the error originated.
Second, when referencing (as you suggest in your article) the Exception framework, the debugger breaks into the source-code of Exceptions.ThrowIfOutOfRangeIncludeMaxCore, which might be confusing too.
Your second example using a CheckBox doesn't make sense. In none of the methods on the stack trace there are argument validation checks. So what you see in this example is a plain old bug (hence, it is throwing NullReferenceException instead of a ArgumentNullException). Happily CheckBox.PaintUp is on the top of the stack trace, because that's where I must fix this bug! (Microsoft should have added validation checks in PaintUp).
Don’t get me wrong: I like your code and I hope every developer starts using it. Then at least a lot of pre-conditions are caught and overall quality of source-code will improve. I just whished I had some more control adding debugging code using "throwing exceptions at addresses", "method in-lining" or "preprocessing my source code".
Cheers, Sander Leer
PS. For me the "show non-user code" preference is always on because the bug is always in somebody’s else his code 
|
|
|
|
|
|
|
Sander Leer wrote:
Second, when referencing (as you suggest in your article) the Exception framework, the debugger breaks into the source-code of Exceptions.Throw­If­Out­Of­Range­Include­Max­Core, which might be confusing too.
This article does not provide an exception framework, only a C# source file to be included in your projects. The article states, “This utility class is designed to be included in your C# projects, not referenced as a compiled assembly.”
I used the CheckBox example to illustrate that it’s typical, when using referenced assemblies such as WinForms, to have exceptions thrown from private methods. You’re right that MS probably should be throwing an Argument­Null­Exception in Check­Box.OnPaint(), but it makes no difference to you because the only bug you can fix is in your own code that passed in the null Paint­Event­Args. Generally I find “Show Non-user Code” to be useful only to tell me under-the-covers stuff that’s nice to know about the BCL since I don’t have the source and can only fix my “user code” anyway.
I’ll agree with you that it would be cool to have Delphi’s simple way of tweaking the stack frame.
Thanks for the thoughtful feedback.
-- /\/. _/.
|
|
|
|
|
|
|
If you just want to see a shorter call stack when you break into the debugger, you could tag all the methods in the Exceptions class with the [DebuggerHidden] attribute. It won't help if you retrieve the stack trace programmatically, but if you're using Visual Studio, it will make the debugger break at the line where the problem actually occurred (the point where you're calling Exceptions.ThrowIfNull or whatever), instead of breaking inside ThrowIfNull.
|
|
|
|
|
|
|
This is exactly what we needed. I just tried it and it works like a charm. I’ll upload the change.
Many thanks.
-- /\/. _/.
|
|
|
|
|
| Speed/size analysis
Ed Brey | 4:58 22 Dec '04 |
|
|
One thing lacking from your article is the impact that using the Exceptions methods will have on size and speed of the code. A nice feature of the "if (...) RaiseError()" pattern, is that so long as the compiler is smart enough to not inline RaiseError, you get a fast check with minimial code size. (Of course, sometimes, RaiseError() is trivial, and won't even be a separate method.)
By bundling the check into the Exceptions method, the compiler would have to be smart enough inline only the check to get the optimal combination.
Another question: For the oh-so-common check for a null argument, why do you provide your own description, rather than let ArgumentNullException take care of that for you? It would seem that you're adding extra code only to undo the localization built into the .NET framework.
|
|
|
|
|
|
|
Ed, thanks for the feedback.
You’re right that refactoring check-and-throw logic into a separate method may compromise compiler optimization. The question is, Does it matter? I generally avoid helping compiler optimization at the expense of programmer convenience unless I have hard data from a profiler to prove it’s a problem. My guess is that the impact will vary from app to app. Only a code profiler run against your app will prove that you need to back out the refactorings I’ve suggested here.
You’re also right about ArgumentNullException—I am needlessly building the message. I’ll change this.
-- /\/. _/.
|
|
|
|
|
|
|
I agree that developers' brains have lots of things better to concentrate on than compilation details. That's all the more reason for one, centralized analysis at the library level. Just as a developer appreciates knowing that the library is rung out with umpteen unit tests, it's a big time saver to see look up the typical time/space cost of using the library, and from that make the call: (1) performance hit acceptable: use without remorse, or (2) too much, use the standard logic. The one thing no one wants to have to do is start using the library and then try to discover via profiling whether its resulting in a ten percent slow-down due to extra calls or cache misses caused by a larger native image.
|
|
|
|
|
|
|
You may have a point, but I stand by my assertion that these things vary from app to app—there is no “typical.” Thus the need for profiling and perf counters.
For example, if your project is a class library, like a set of WinForms controls, you’re likely going to have many public members and therefore will make a lot of calls to Exceptions.ThrowIf…. A monolithic WinForms app won’t have the need for as many calls to Exceptions.ThrowIf… because most of the code is internal to the assembly.
I don’t know that you can generalize about image size—it all depends on how often you call these methods.
Check out the free Process Explorer for powerful .NET app runtime analysis and nprof for profiling.
-- /\/. _/.
|
|
|
|
|
|
|
Ed Brey wrote:
he one thing no one wants to have to do is start using the library and then try to discover via profiling whether its resulting in a ten percent slow-down due to extra calls or cache misses caused by a larger native image
This is, with todays Hardware/Compiler - Technology, very unlikely to happen. Most 10% slowdowns are not noticed by the user, and those which are can be nailed down to 1or a few places - and in these places, the "callee verifies" approach is likely to be a problem anyway.
we are here to help each other get through this thing, whatever it is Vonnegut jr.
boost your code || Fold With Us! || sighist | doxygen
|
|
|
|
|
| Why not assert?
Marc Clifton | 12:24 20 Dec '04 |
|
|
Since this seems to be for method argument validation rather than user input validation, why not use Debug.Assert? If the argument is bad, then it's a bug, and it should be caught during testing.
Marc
MyXaml
Advanced Unit Testing
|
|
|
|
|
|
|
Excellent question.
I’ll update the article to address it, but here’s my take on this:
- Use
Debug.Assert() within private and internal members
- Use
throw within protected, protected internal and public members
My rationale is as follows. If the code can be called by code running in a different assembly, then such an assembly may not belong to you—it may be another developer calling your code. That developer will appreciate your throwing a properly formatted exception that he can catch instead of having a failed assertion foul up the works of his app that references your assembly. (Never mind that he gave you a bad argument—he still deserves an exception.)
If the code can only be called by code running in the same assembly, then it’s your source and you might like the convenience of a failed assertion.
I check arguments and throw exceptions instead of Assert()ing even in a situation like this:
using System;
using System.Drawing;
using System.Windows.Forms;
namespace Umbrae.Windows.Forms
{
public class MyForm : Form
{
public MyForm() : base()
{
}
protected override void OnPaint(PaintEventArgs e)
{
Exceptions.ThrowIfArgumentNull(e, "e");
}
}
}
The reason is that I don’t know who might subclass MyForm at some point and pass a bad PaintEventArgs argument to my implementation of OnPaint(). They should get an ArgumentNullException, not a NullReferenceException or some other inappropriate weirdness.
Just my opinion, though.
-- /\/. _/.
|
|
|
|
|
| Nice work.
Angelos Petropoulos | 1:51 20 Dec '04 |
|
|
The whole project is tested thoroughly and is well documented. Very nice work.
Would it be OK if I changed the name of the class from Exception to something else and used it in my projects?
Angelos Petropoulos
|
|
|
|
|
|
|
Thanks.
Sure—you can adapt it however you need to. I expected developers to want to change the namespace to match their own root namespace.
It would be great if you kept my name in the credits, though.
I’m not using VS 2005, but C#’s new partial keyword would come in real handy here. You could easily add your own application-specific argument exceptions in a separate .cs file and still have them show up as members of the same class.
-- /\/. _/.
|
|
|
|
|
|
|
Thank you. I will most certainly be keeping your name in the credits, including a link to this article.
Angelos Petropoulos
|
|
|
|
|
| Long names
Frank Hileman | 18:16 15 Dec '04 |
|
|
We created a similar class. You don't really need the "ThrowIf" prefix everywhere, if the class name fulfills that purpose. For example, if the class is named check, Check.ArgumentNull(...). Just a little more compact.
Nice article.
check out VG.net: www.vgdotnet.com
An animated vector graphics system integrated in VS.net
|
|
|
|
|
