Aargh! Part Seven

Aargh! Part Seven

  • Comments 23

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!

NULL 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 Assert .

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.

 

  • Actually, in C#, private really is essentially public. This is pretty high up there on my own personal "Aargh" list.
  • 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.
  • > 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.
  • *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.
  • 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.
  • > *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.
  • 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.
  • 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
    ...
    default:
    SWITCHBUG("Someone updated the SCRIPT_FROB enum without adding a new case.");
    }

    where SWITCHBUG(msg) was just a macro for ASSERT(FALSE, (msg))
  • 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...
  • 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.)
  • 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.
  • Here's a nice article at CodeProject that explains what asserts are and the right way to use them:
    http://www.codeproject.com/cpp/assertisyourfriend.asp

    The best material on asserts that I have seen is the chapter in "Writing Solid Code" dedicated to them.
  • 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.
  • 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.
  • 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.
Page 1 of 2 (23 items) 12