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!

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.

Comments

• Anonymous
  June 04, 2004
  The comment has been removed
• Anonymous
  June 04, 2004
  The comment has been removed
• Anonymous
  June 04, 2004
  The comment has been removed
• Anonymous
  June 04, 2004
  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.
• Anonymous
  June 04, 2004
  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.
  
• Anonymous
  June 04, 2004
  > 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.
• Anonymous
  June 04, 2004
  The comment has been removed
• Anonymous
  June 04, 2004
  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))
  
• Anonymous
  June 04, 2004
  The comment has been removed
• Anonymous
  June 04, 2004
  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.)
• Anonymous
  June 04, 2004
  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.
• Anonymous
  June 05, 2004
  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.
• Anonymous
  June 05, 2004
  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.
• Anonymous
  June 05, 2004
  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.
• Anonymous
  June 05, 2004
  The comment has been removed
• Anonymous
  June 06, 2004
  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.
  
  
  Thanks,
  Vishal.
• Anonymous
  June 07, 2004
  The comment has been removed
• Anonymous
  June 07, 2004
  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?
  
• Anonymous
  June 07, 2004
  > 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.)
  
  
• Anonymous
  June 08, 2004
  The comment has been removed
• Anonymous
  June 14, 2004
  The comment has been removed
• Anonymous
  June 14, 2004
  The comment has been removed
• Anonymous
  July 03, 2004
  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.
  
  
  Thanks,
  Vishal.