Aargh! Part Seven

Q: How do pirates keep their socks from falling down?

A: Thumbtacks.

I am insanely busy with bug fixing and performance testing today, so once more I’ll dip into my endless archive of rants about irksome coding practices I’ve seen one time too many.

Gripe #8: Assert the truth, the whole truth, and nothing but the truth

Debug assertions should always describe invariants — conditions that should always happen no matter what. They both help document your program, by making your invariants clear, and help catch bugs by alerting you when those invariants are violated. But assertions must describe what you believe to be always true about your algorithm, not what you hope is true:

pv = malloc(10);
Assert(NULL != pv);

Aargh! That’s drivin’ me nuts!


is a legal return value of malloc and you therefore can’t assert that malloc didn’t return it. That condition, rare though it might be, has to be tested like any other. You can’t simply declare that the world is going to turn out the way you’d like it.

Sometimes you want to pop up warnings in your debug build when incredibly rare things happen. That’s a great idea — but create a little function called CreateDebugWarning or some such thing, so that it does not get confused with


Gripe #9: Don’t be so darn friendly

In C++ you can hide implementation details with the private keyword, but sometimes you want to have two classes which have the ability to party on each other’s internals. For example, you might have a collection class and an enumerator class that need to have a private way to communicate with each other. Such classes are called “friendly classes”. One day I grepped the headers of an unnamed Microsoft application for the word friend. It appeared over 600 times! Aargh!

This is a bad sign. One class had eleven different friend classes. When you have that many friends, there really is no difference between the private interface and the public interface. Visibility modifiers exist in the first place so that you can do information hiding and clean polymorphism. If you have to allow friend access to so many different classes then your public interfaces are not clean. You have a bunch of classes depending on each other’s implementation details in order to work properly. The information hiding afforded by classes and interfaces is a feature of C++ specifically designed to reduce the complexity inherant in large software projects — friend is a way to work around that limitation when necessary. And there certainly are times when it is necessary, but overuse makes code more and more complex, intertwined, buggy and unmaintainable.

Use friends very sparingly in C++ — enumerators should be friends of collections, yes, but if documents are friends of views, you might have a problem on your hands.

In C#, JScript .NET and VB.NET there are no friendly classes. Rather, there is private, public and internal — in C#, private really is private, but any class in the same assembly can party on internal data. This gives you a lot of the benefits of friendly classes while still being able to restrict access to stuff that is truly private.


Comments (23)

  1. Nicole Calinoiu says:

    Actually, in C#, private really is essentially public. This is pretty high up there on my own personal "Aargh" list.

  2. DangerMike says:

    That’s not really a fair statement. The side-effect of reflection is that you can get inside anything and do anything you want to it. It is extrememly sleazy and easy to abuse, but it doesn’t change the fact that in any .NET language, the interface is a contract. Using reflection to manipulate private members is a breach of that contract, tantamount to getting a pointer to an object an playing with its guts directly (as a byte array or similar). But it sure does make for some beautiful debugging.

    So, I’m putting reflection (and Lutz Roeder, its undisputed master) on my visions-of-sugar-plumbs list and giving a hearty fist-shaking AARGH to undisciplined developers who misuse it.

  3. Eric Lippert says:

    > Actually, in C#, private really is essentially public.

    No, IN C# private is private.

    There’s no way to use the C# language to get at private data.

    You are confusing the language in the abstract with its concrete implementation. They are quite different animals!

    In one particular implementation of C#, you have access to a library that allows you to see the internals of an object if you are fully trusted, but that library has nothing to do with the C# language. We could come up with an entirely separate implementation of C# which didn’t use the PE format, etc, and therefore didn’t allow reflection on privates. AFAIK, there is nothing in the C# spec that says that privates have to be laid out in a particular manner so that reflection can party on them.

    Similarly, in C++ THE LANGUAGE there is no way to get at a private data member. In a PARTICULAR implementation of C++, maybe pointer arithmetic works — but that fact is a fact about a particular implementation, not a fact about the language.

  4. mattd says:

    *code example from MSDN*

    int IntegerDivide ( int dividend , int divisor )

    { Debug.Assert ( divisor != 0 );

    return ( dividend / divisor ); }

    According to Gripe 8 this is improper use of Assert. 0 is a perfectly valid value for a signed int. However this kind of code is everywhere on MSDN, maybe this is a bad example but my only point is Gripe 8 might be a little weak.

  5. Eric Lippert says:

    It depends. Is IntegerDivide a public method or is it private/protected/internal?

    If it is public then I would remove the assert — the exception ought to be warning enough to the caller that they’ve done a bad thing. Or I’d have the method throw another exception, or do some other appropriate behaviour.

    If it is not public then the assertion is warranted if it is really the case that the divisor is logically guaranteed to be nonzero.

  6. Derek says:

    > *code example from MSDN*

    > [snip]

    > …this kind of code is everywhere on MSDN… Gripe 8 might be a little weak.

    I apologize for paraphrasing, and I hope I’m not quoting you out of context. However, Microsoft doesn’t have a very good track record of writing good behind-the-scenes code. For example, as of VC++6.0, compiling with all of the warnings turned on results in quite a lot of warnings. And then there are circular include dependencies: ole2.h -> objbase.h -> urlmon.h -> servprov.h -> ole2.h, just to name one.

    So my argument is that the gripe isn’t weakened by Microsoft’s own (mis)use of Debug.Assert.

    To give another example of Gripe #8, where I work, our code is sprinkled liberally with ASSERT(0)’s. I’m not sure you can get much worse than this.

  7. Eric Lippert says:

    Indeed — if the argument is: "MSDN sample code is a paragon of excellence, therefore your gripe is incorrect" then I respectfully disagree.

    MSDN sample code is pretty darn good most of the time, but like all code, it’s not perfect. I have gripes about it like any other code.

    I would like to take this opportunity to say that though obviously there is always room for further improvement, I have been extremely impressed at the quality of some of the sample code.

    We went through a bit of a sea change a few years back when we realized that lots and lots of developers take the sample code and paste it right into their own applications, change the variable names and ship it.

    I look back at some of MY code that made it into the MSDN samples and I am now horrified that, for instance, I left out error handling to make the "main line" code more obvious. I meant well, but the right thing to do would be to put in the error handling and let the developers modify it to meet their needs. If it isn’t there, it won’t get modified, and I’ll have just contributed to more brittle code in the world.

    A lot of the samples are now a lot more complete as far as robustness and security goes.

  8. Eric Lippert says:

    An ASSERT(FALSE) comes in handy if you know that a particular branch of the logic can never be visited, but again, it might be worthwhile to give the macro a different name for that case. We had a macro, for example, where we could say something like


    SWITCHBUG("Someone updated the SCRIPT_FROB enum without adding a new case.");


    where SWITCHBUG(msg) was just a macro for ASSERT(FALSE, (msg))

  9. Keith Moore [exmsft] says:

    Slightly OT, but here’s a gross, disgusting, guaranteed-to-get-your-ass-kicked C++ hack I saw a few years ago:

    #define private public

    #define protected public

    #include <something_you_really_need_to_screw_with>

    Don’t try this at home, kids…

  10. Peter Milley says:

    The preprocessor is evil. Eeeevil.

    (Side story: As an intern at Watcom I once had an interesting project, namely figuring out how to get the C++ compiler to generate code-browsing information, so that future Watcom IDE’s could take you straight to the definition of any variable. When I first realized that the definition of any variable in C++ can potentially span an arbitrarily large number of files, I think I experienced actual horror.)

  11. Mike Dunn says:

    In C++ (not sure about other langs, I’m a C++ guy) you can write:

    ASSERT(!"Flagrant system error!");

    which is the equivalent of ASSERT(0) and prints the message in the assert-failed dialog.

  12. Mike Dunn says:

    Here’s a nice article at CodeProject that explains what asserts are and the right way to use them:


    The best material on asserts that I have seen is the chapter in "Writing Solid Code" dedicated to them.

  13. Frederik Slijkerman says:

    Actually, the Assert in the IntegerDivide function is not so bad — if the documentation for the function says that it should only be called with a non-zero divisor.

  14. Eric Lippert says:

    If the method is private/internal, then sure, you can say that the method is guaranteed to be never called with a zero. If it is, then you have a bug, and the assertion fires.

    But assertions should be for things which are logically impossible to be wrong. If the method is public then there is no restriction on the user from doing anything they like, whether the documentation says its OK or not. To be robust, the method has to handle bad input from its users.

    Maybe some kind of debug mode warning would be a good idea, but I’d use Debug.Fail or Debug.Write instead of Debug.Assert — I try to reserve Assert to actually do what it says — make logical assertions.

  15. Eric Lippert says:

    Of course, you have to draw the line at robustness somewhere. I think Larry Osterman had a post recently about whether to handle bad pointers gracefully, or crash and die. I agree with him — crash and die is the right way to go. But that’s another topic.

  16. Vishal says:

    I had this doubt for quite some time. Maybe it is obvious, but still. Shouldn’t implementations of strcpy() etc. have appropriate assert statements to check whether the pointer is valid? I never found them in textbooks or in the glibc.



  17. Nicole Calinoiu says:

    DangerMike: To be honest, I really don’t worry at all about incidental misuse by undisciplined developers, and it’s the sort of thing that’s probably a warn-once (if that) offense in many shops. However, I do worry a great deal about little things like deliberate bypassing of input boundaries when code is running in environments that are outside of my control. There are ways to raise the bar against this sort of thing, but they are tedious and time-consuming, and I’m more than a little fed up of designing and implementing to accomodate the workarounds.

    Eric: Actually, I wasn’t confusing the implementation with the spec–I was just being a wee lot too terse. Of course there’s no way to use the language itself to do much of anything at all, including inappropriate invocation of low-accessibility members. <gdr> However, most of us don’t spend our days compiling and running against the spec, and I’m a little more concerned about the behaviour of the "defining" production implementation than I am about the specification since it’s the former that causes me far more pain.

  18. Eric Lippert says:

    Right, I take your point. My point is simply that your complaint is about the design of the .NET Framework reflection classes, not about the design of the C# language.

    What I don’t understand is the nature of your complaint. Obviously, violating encapsulation is in general a bad thing to do, but there are some rare times when you need to do so. (In fact, the VSTO2 runtime does some private reflection, though I would like to eliminate it if possible.)

    But anyone who reflects upon a private should know that they’re playing with fire. What kinds of scenarios are you worried about?

  19. Eric Lippert says:

    > Shouldn’t implementations of strcpy() etc. have appropriate assert statements to check whether the pointer is valid? I never found them in textbooks or in the glibc.

    Yes, I think that debug builds should have some assertions to trap errors in callers. Retail builds, no.

    (Of course, a big part of the problem is that the standard library has a lot of problems with its function interface design. Another big part of the problem is that C’s memory model allows you to pass in bad pointers in the first place. Fix those two things and a lot of the need for assertions goes away.)

  20. Nicole Calinoiu says:

    Not all limitations of accessibility are mere matters of convenience. Some are meant to limit the set of actions available to external code for reasons of security or IP protection. One fairly common example is the establishment of an input boundary. Direct manipulation of private fields allows for trivial bypass of input boundaries. This can be countered in some cases via a large number of tedious and annoying CAS verifications, but it’s a lot of trouble, and there are cases where there’s no workaround short of a fairly major design change.

    There are really two main problems here:

    1. Many developers do not realize that reflection on supposedly low accessibility members can expose their code to risks they thought they had eliminated by declaring the low accessibility in the first place. These folks are shipping applications that are not as robust to attack as they might believe, even after they’ve potentially expended a great deal of effort to implement a variety of protective measures.

    2. Some are aware of the problem, but that almost only serves to make things worse. Client expectations sometimes require us to do whatever is possible to minimize the potential security holes opened by reflection into low accessibility members. There are essentially three choices: do a rather large amount of work to raise the bar a bit, pick a different platform, or target clients who don’t care if their internal users and/or administrators can hack your application in a trivial manner.

    (If you can’t see why scenarios like bypassing of input boundaries by legitimate users might pose serious security risks, or if you’re not aware of the painful steps required to prevent this, let me know, and I’ll post a private message. I’m just not very comfortable posting publicly what would essentially be a hacking guide.)

  21. > #8 – Debug assertions should always describe invariants

    Well, maybe for you, I use asserts to describe my assumptions (how do I know something is invariant? I don’t, I assume it is, so maybe I’m just splitting hairs). I agree your example is bad, but sometimes that is a development placeholder that says "I haven’t figured out what my failure handling procedure should be here, so I’ll assert this during development to show that I’ve assumed malloc() succeeding to work for now" (a bit lick, but better than , a "TODO" comment). You do manually inspect every assert() before releasing code, don’t you….

    > #9 – Use friends very sparingly in C++

    Well, maybe that’s how you use friends, but others may do it differently. Try reading Jiri Soukup’s book on Taming C++ which uses friends for a very different but useful purpose, he uses them to make design patterns distinguishable in the delivered source (where they normally vanish between the design docs and the coding). Again, I agree that overdoing friends CAN be a bad sign, but I’d point out that there is more than one way to write code in languages as rich as C++.

    I’d remind all developers to question their own assumptions as well as questioning other peoples’ implementations – if they don’t match up the fault is not always that of the "Other guy".

  22. peterb says:

    Oh my god yes, your example #8 drives me completely up a wall. I see it all the time:

    rc = some_function(args);

    ASSERT(rc == SUCCESS);

    Which makes me want to ask the developer: "What the hell is wrong with you?" and smack them with a bat.

    > Actually, the Assert in the IntegerDivide function is

    > not so bad — if the documentation for the function

    > says that it should only be called with a non-zero

    > divisor.

    Yes, it really is that bad (assuming it’s not private / testing some purely internal assumption). A function that will panic or assert when it is given bad input is a function that has a bug in it. All functions should check their inputs for validity and return an error if it finds they are invalid. Anything else is just begging for misuse and brokenness.

    Properly used, asserts should always, 100% of the time, indicate logical fallacies or programming errors.

    I’m conflicted on ASSERT(0). I understsand the motivation but hey, if you want to make your process panic so you can debug it, why not just use panic() instead? That’s what it’s for.

  23. Vishal says:

    I had this doubt for quite some time. Maybe it is obvious, but still. Shouldn’t implementations of strcpy() etc. have appropriate assert statements to check whether the pointer is valid? I never found them in textbooks or in the glibc.