Friday, January 11, 2013

The Role of Good Software Design in wxMaxima

See: wxMaxima Issue #233
Patch: wxMaxima Pull Request #241

I posted previously about my first attempt to solve this issue. I'm going to briefly discuss why that approach was wrong, and explain a little something about how wxMaxima is architected. Hopefully future developers on wxMaxima will benefit from this discussion, and I hope that developers on other projects will also try to avoid similar mistakes.

I decided to make this post about good software design because the actual patch was rather simple, and the reason for that comes from the beauty of good software design.

Good Design Prevents Bad Errors


In the past, I have complained to my colleagues about the fact that sometimes changing the order of the #includes in a C++ source file can affect whether or not your code will compile. This was, of course, in my early days of C++, where most of the inner workings of the language seemed pretty mysterious. Unfortunately, knowing more about a technology can sometimes [read: often] result in ugly hacks, like the #ifdef hacks I tried at the end of my previous post, or StarTrek Voyager's deflector array (satired here). Or just ask anyone who has ever had to maintain a Perl script.

But I've never had a problem like that when I was making changes with good software design in mind.

By now, you would think that I would know to look for the source of an error caused by bad design, rather than trying to decrypt C++ compiler messages, but whenever I see an interesting compile error, my ego gets in the way and says, "I'm clever enough to fix this!" In reality, when you see a strange error you should take a step back instead of trying to fix it right away, and try to see where you might have gone wrong. Especially when you're working on a well-established and stable project, it's probably definitely* your fault when something breaks, and you can't discount the possibility that it was a bad design decision on your part that caused the problem.

[*Unless you've exhausted all other possibilities and you can prove it was an existing design flaw that somehow your patch revealed. And even if you're right, it will probably be more expensive to fix the right way, so it's still on you to make it work within your patch. (That's another problem in the software industry, but it's hard to make a practical argument against it.)]

Understanding the GUI Layout


With that discussion out of the way, let's take a moment to understand the component design of wxMaxima. I'll provide some pictures to make this a bit easier to digest, but first a quick description that I've learned from a few weeks of working with the code.

wxMaxima.cpp and wxMaximaFrame.cpp define the GUI layout of the main window, the top level containers, the menus, hotkeys, etc. All the basic high-level GUI design.

MathCtrl is the pane which contains all the content, and the content is composed of a series of GroupCells (which are a subtype of MathCell). Most of the GroupCells are statically typed as MathCells in the source, but that's probably an artifact of an earlier version and not terribly important here. The MathCtrl corresponds to the document itself, so it makes the most sense that this is where we want to decide whether the document is in an unsaved state. It helps that the MathCtrl itself deals with user interactions with the content displayed in the MathCtrl.

The output is composed of a series of subtypes of MathCells (for instance the SqrtCell in Fig. 1 below), but that's not important for this patch.

Fig. 1: GUI components annotated in color.

Understanding the Relationships between Objects


If you've ever really studied what it means to have an Object-Oriented design (not just the mere fact of using classes, but actually designing software in an Object-Oriented way), you probably know that the main strength that OO design offers us is the ability to abstract and assign responsibility. In order to make sure that we have good abstract and role definitions, we need to do a few things:

First, we want to make sure that the object/module dependency graph is a tree. That is, there are no cycles: if module A calls module B, then module B should not call module A either directly or indirectly. Basically, class A is class B's drill sergeant. A can give orders to B, give data to B, or ask for data from B. B just does its job with whatever A gives it. If B starts giving orders to A or speaking out of turn (even if B thinks he's being helpful), there's going to be trouble.

The problem with my first approach to my patch is that I failed to acknowledge the relationship between Objects in wxMaxima (shown in Fig. 2 below), and wanted to give GroupCell a reference to the MathCtrl which contained it. Based on the description above, you can see that that is a pretty scary situation.

Fig. 2: A partial graph of Object relationships in wxMaxima (generated by Doxygen). Relationships directly relevant to this discussion are highlighted in yellow.

Luckily, g++ was kind enough not to allow me to compile the code in that way. I can't say for sure, but if it is even possible to get a C++ program to compile with that kind of circular dependency, I would have been really unlucky if the code had compiled without giving me any trouble at all. Circular inclusions are a tricky problem in C++... you get a weird compiler error and fixing it is rather touch-and-go, but you should definitely check whether you really want to introduce that circular dependency.

Sidebar: There are some metrics that can be used to judge the quality and maintainability of Object-Oriented code. One is how tightly coupled your classes are to other classes. If you have a circular dependency between two classes, those classes are tightly coupled. This is bad for code maintainability because if you change something in either one of those classes, you have to update the other class to adapt to the change. In good OO design, we want changes made to any class not to require changes to another class. Often times it takes several rounds of refactoring and some knowledge of design patterns to get this to be completely true, but we can avoid the tight-coupling problem really easily by just being aware of it.
Tip: for those who use Visual Studio, you can do source code analysis (at least in a C# project) and it will show you maintainability metrics like the one I described above.

Analysis and Patch


So basically, even though GroupCell is responsible for actually handling the folding and unfolding actions, MathCtrl is the class which processes the user input which should cause the folding to occur. MathCtrl passes the message on to the relevant GroupCell(s) which in turn execute the folding algorithms. Since hotkeys and menu items are registered with wxMaxima, any hotkey actions which cause folding will cause the wxMaxima object to pass a message to MathCtrl which, again, will pass a message to the relevant GroupCell(s).

A quick search in the file yielded 5 functions which process folding. All I needed to do was, for every time a folding event occurred, I needed to mark the document as unsaved. Luckily the functionality already exists for this and happens to be conveniently packaged as the simple function call: MathCtrl::SetSaved(false), which happens to be a simple setter function. The magic happens in the wxMaxima class, which polls for a change in the saved status when the window is idle (wxMaxima::OnIdle()). Since in almost every case, UpdateMLast() was called, and it wouldn't hurt to call it in the final case, I refactored the code which handles folding events into the function MathCtrl::FoldOccurred().

The logic of the patch is contained in commit e27d891. (The other commits in the pull request are documentation and maintenance.)

If you don't count the refactoring (which was a good choice because it will help with maintainability later, if the folding functionality needs to be changed again), the patch added about 5 logical lines of code, and the line I added in each place was the same.

If the function FoldOccurred() had already existed and had been used as I have used it, the change would have been a single line of code.

That's the power of good design. Highly maintainable code, which requires only small changes to extend or correct functionality. And the lesson to be learned here is that if you take the time to understand the design of the code you're working on, you can save yourself a lot of time in maintaining the code later on.

1 comment:

  1. Hi,

    as one of the developers that worked on the code that you patched: this is truly a better way (introduces less complexity as you've figured out) to achieve this.

    However sometimes the code is much shorter/simpler or it is plainly impossible to do it in such a top-down manner and you have to introduce some dependency between objects, then there is a way to do that in a relatively clean way.

    Let me demonstrate on your example.

    In a separate header you'd define a pure abstract class:

    struct DocumentModifiedInterface
    {
    virtual void MarkAsChanged() = 0;
    };

    This header would be included by both MathCtrl and GroupCell (no problems there).
    Now instead of pushing a reference to MathCtrl (via a pointer) into GroupCell instances, you would pass a DocumentModifiedInterface* into a GroupeCell - so GroupCell has no dependancy on MathCtrl, no problem there.

    The beautiful part is this:
    to get the MathCtrl part correct you just "implement" the interface in MathCtrl

    class MathCtrl : public DocumentModifiedInterface
    {
    virtual void MarkAsChanged()
    {
    // set the dirty flag
    }
    ...
    };

    Now when constructing GroupCells you'd pass ((DocumentModifiedInterface*)this) into the constructor.

    Yes this solution is still more complex since GroupCells can modify MathCtrl, but now you:
    a) have no compiler problems
    b) the interface which GroupCells can use is clearly defined (and limited)
    c) the code clearly states what you're trying to do
    d) the code is almost as short as your initial solution

    Thanks for contributing to wxMaxima, keep up the good work!

    ReplyDelete