Thursday, January 17, 2013

FTGL: Bad Code smell

I'm hacking around the internals of FTGL an OpenGL font rendering engine, it's pretty nice to use but it's not structured well underneath. It uses the pimpl idiom which is fine (though for games isn't ideal) but it's managed to use both composition and inheritance to do the same thing! It needed only to pick one. Every class has the class itself and then an additional class that has the implementation - so there are double the number of classes straight of the bat class1 and class1_implementation.

Here's an example: TextureFont inherits from a Font base class. When you construct a texture_font in the constructor a texture_font_implementation object is created which inherits from a font_implementation base class, this is then passed up and stored as a member of the Font class. It's ugly and inefficient, pimpl speeds up compile time but FTGL is a tiny library that can be statically built there's no good reason to use it.

The code smell in the blog title isn't anything to do with though. I recogonise this poor code as I do fallen prey it to more than once!

        /* Allow our internal subclasses to access the private constructor */
        friend class FTBitmapGlyph;
        friend class FTBufferGlyph;
        friend class FTExtrudeGlyph;
        friend class FTOutlineGlyph;
        friend class FTPixmapGlyph;
        friend class FTPolygonGlyph;
        friend class FTTextureGlyph;

The above code is in FTGlyph (and it wouldn't surprise me if the exact same code was all over the place). Each time you add a specialized new font you're going to have to remember to update this list and probably several other places in the code. It makes the code harder to extend and maintain. That's the smell, when you need to add something it should be in one place and one place only, no lists like the above snippet, there's always a cleaner way to do it.

In this case just make the constructor public, there's no strong need for it to be private. I think the use case for the friend keyword is extremely rare in itself.

No comments: