C Library and API character functions: the program

Home Back To Tips Page

This article describes a little project I wrote to show the functionality of the various character functions that are available.  The problem is that in the default case, if no special action is taken by the programmer, that tolower and CharLower produce different results, islower and IsCharLower return different values, and isalpha and IsCharAlpha produce different results, just to cite a few examples.

The output is shown on the output display page.  This article describes the program that I used to create that table.

First, I want to observe that the first draft of the output was not produced by this program; it was produced by a little console app that just wrote HTML to stdout, which I then piped to a file.  The problem was that my original approach to output was rather naïve, and did not produce really "nice-looking" output.  I then spent a fair amount of time playing with the formatting in FrontPage getting something I was happy with.

But I wanted to add locale support to the project, and I was not going to have to require the user to invoke a command line app with an LCID (locale ID). That was just too ugly.

So what I did was take the Microsoft HTML Edit View example and modify it to produce the desired output.

Along the way, I cleaned up the gross and disgusting code that Microsoft had published as an example of how to write a program.  Microsoft examples should illustrate "best practice", and not "we hacked it until it worked".  The problems I discovered and corrected are discussed at the end of this article.

The program

The C Locale

The default Locale for the C library is the "C" locale; this was used to produce the default character table shown here.

Column display and order

The View > Columns... brings up the column-order dialog

Using this dialog, columns can be checked and unchecked, and the display position can be moved.  It is a multi-select listbox, so several items can be selected and move at the same time.  Note that the first column, Character, cannot be unchecked and cannot be moved; it will always be the first column.

To see the result of this change, the Go button must be clicked.

It's an HTML editor...

This is based on the HTML editor example.  This example has some unfortunately poorly-written aspects which I had to correct, but it does mean that you can see the raw HTML code.  This was very handy during the development and tweaking process.

At this point, if you only want to use the program, you can stop reading.  But the problem with this code was that it was so badly written that I would have been personally embarrassed to have put my name on this code, even though it was based on a Microsoft example.  It was beyond any doubt some of the grossest, most disgusting, ugly code I have seen in a while.  It was written by a beginner, who did not understand fundamental concepts such as "interface" and "abstraction".  There was nothing I could do but rewrite this code so that it resembled something like best practice, instead of embodying examples of the worst possible ways to write code.

An example of not just one, but a whole set of worst-practice styles

The code of the HTMLEditView example was clearly a model of turn-it-loose-on-a-summer-intern-who-will-hack-it-until-it-works.  There was no supervision, no code review, no attempt at what we call "design".  Code was tossed in, willy-nilly, then #includes were tossed in until it compiled.  Unfortunately, there are those who will take a piece of trash like this and think that it is an exemplar of good design methodology and clean implementation technique.  This is not really an acceptable practice.  Code examples should represent "best style" so that good models are presented to programmers who are learning.  The original HTML editor represents so many anti-patterns it is hard to enumerate them all.  It was clearly a case of hack-it-until-it-compiles, not a really good methodology for software development in the real world.  The result could be most politely referred to as an unmaintainable mess.

When is a good example not a good example?

Learn where module boundaries exist

Microsoft example code is often useful for low-level techniques, but far too many of the examples are very poor examples of programming style.  In this sense, the Microsoft code is akin to the books by Petzold, that teach away from good programming practice.

One of the worst examples of how this is done is the pattern called "add enough include files until you get it to compile".  There is no design here; it is amateur programming at its worst.  Every module in the system knows the internal structure of every other module, and the result is profoundly ugly and truly unmaintainable code. 

It is essential to understand the notion of "module" and "interface".  The HTML Edit View example is a great example of how to screw this up beyond belief.

First, there is the issue of docking toolbars.  For reasons that are incomprehensible, the mainframe of HTMLEditView knows the internal structure and creates the controls for the toolbar!  Why, exactly, there is this gross violation of structure escapes me.  I added a new toolbar, and very little of the internal structure of this escapes to the mainframe, and none of it is known to any other component of the system.  I then rewrote the other toolbar to isolate completely within the toolbar itself all the details of its implementation.  The result is that the mainframe knows nothing about how the toolbars are implemented.

The worst cases, however, are embodied in how the document and views interact with the main frame.

• The document knows the variables and methods of the main frame
• The document knows the variables and methods of the views
• The views know the variables and methods of the main frame
• The mainframe knows the variables and methods of the views

None of these make sense.  There is no rational world in which a view would know about the mainframe or CWinApp-derived class.  There is no rational world in which a document would know the methods or variables of a CView.  Ever.

Key to this is the notion of interface.

The only thing a document or view should know about the mainframe is that

• It exists
• It is represented by a CWnd *
• It is capable of receiving messages, and in some cases, implicitly routing them

The only thing a mainframe should know about a view is

• It might exist
• It can be obtained by GetActiveView
• It is of type CView*
• It supports methods of CView
• It can accept messages

The only thing a mainframe should know about a document is

• It might exist
• It can be obtained by GetActiveDocument
• It is of type CDocument*
• It supports methods of CDocument

The only thing a document should know about a view is

• Some arbitrary number of views exist
• They can be notified of actions to take by using UpdateAllViews

The only thing a document, view, dialog, or other component should know about a CWinApp-derived class is

• What it is and does is none of their business.
• The only valid reason for AfxGetApp is to use methods of the base CWinApp class, such as LoadCursor.
• There is a name theApp which should never be typed in by the programmer, anywhere.

What a dialog knows about anything outside its own class is

• Nothing whatsoever

A common error of thinking is "Global variables are bad.  But if I put a variable in the CWinApp-derived class, it is not global, therefore it is Good".  This is not thinking, this is delusion.  If there truly is a need for a global variable (and this is rare), then it should be a global variable.  Better still, it should be a singleton class. It should be defined in a header file of its own, or one which defines several closely-related and interdependent global variables (better still: if they are closely-related and interdependent, why not make them into a singleton class/)  It should not be put into the CWinApp-derived class; that is simply syntactic saccharine (not even as good as syntactic sugar) on a global variable.  It is the same idea, disguised by incidental syntax. 

While there are useful methods of CWinApp, the first time you write

CMyApp * app = (CMyApp *)AfxGetApp();

you can assume you have, at that instant, indicated a fundamental design error.  And just using theApp is even worse.  You do not need to know about the structure of the CWinApp-derived class. Ever.  If you think you do, you are wrong.

Microsoft does not make this obvious; for reasons that make no sense whatsoever, every class generated by MFC has a

#include "project.h"

where project is the name of the project, and thus the header file for the CWinApp-derived class appears in every file.  In a rational world, this would never occur.  I consider the presence of this #include to be offensive.  I delete it.  I will either delete it entirely, as I might in a custom control, or more typically replace it with

#include "resource.h"

which is all you should ever need.

But this code made the class CWinApp hack look elegant by comparison.  Views were reaching inside the mainframe and touching toolbars.  The mainframe was reaching into views.  In a gross example of poor design, there were two methods called "UpdateView" in two different views, but to call these methods you had to know which view you had.  Nothing as clean as a superclass view from which the two views were derived, and a virtual method UpdateView that was implemented in each.  That would have actually required design effort, which was clearly absent in this example.  However, I think the whole notion that a mainframe knows methods of the views is wrong.  What is required is a design of clean interfaces.  The notion that every class knows the methods of every other class is just bad design.  It isn't helped by the C++ language, that has a mid-1970s view of modularization, with primitive concepts such as "public" and "protected" instead of the exporting of named interfaces.

A module is defined by its interfaces.  These include

• Variables
• Methods
• Messages

Of these, the only one which does not require knowing the internal structure of the class is the message, which is a "universal virtual function call" on any CWnd-derived class.  Some purists will argue that this is not a clean interface because it is not type-safe; you can send messages to windows that do not implement that message type, for example.  But I think this is one of the strengths of this approach because it gives us virtual methods free.  In addition, the argument (and this one has more strength) that since everything has to be cast in and out of WPARAM and LPARAM types that there is a chance of type errors means this is not a clean methodology.  I find that I have so few messages that there is little chance of error.  Of course, it would be possible to create ON_... macros for the message table that were application-specific, but my experience is that when you have few such interfaces, the mysteries of penetrating the message map macros are not worth exploring. 

I tend to use the older C-style casts, but using dynamic_cast<type> would eliminate most of the arguments about type safety of the casts..

Window messages are defined by either the sender interface or the receiver interface.

The sender interface says "When something of this nature happens, a message of this type will be sent to a designated window".  The most common interface we see for this is the WM_COMMAND and WM_NOTIFY messages which a child window sends to its parent.  It doesn't have to know anything about the implementation of the parent; the parent does not need to know anything about the internals of the control.  All the parent knows is that if it wishes to receive a notification from a child, it creates a menu handler for that notification.

The receiver interface says "When you wish to invoke an operation on me, send me this message".  The most common interface we see for this is the various messages that can be sent to windows, such as LB_ADDSTRING, TVM_SELECTITEM, WM_SETTEXT and so on (there are very few actions which can be taken on a window which are not accomplished by sending it a message; notable among these are AdjustWindowRect,  BringWindowToTop, DestroyWindow, GetClientRect, MoveWindow, SetFocus, SetWindowPos, and ShowWindow).  Most other major operations are accomplished by sending messages to a window.

So let's see how this extends to documents, views and frames.

Updating views from the document

A document informs its view of changes by using UpdateAllViews.  Therefore, it is never appropriate for a document to know the internal structure of any view.  A view includes the header file for the document; but a document never includes the header file for any view.

Typically, you specify the behavior of the interface by specifying, in the lHint of UpdateAllViews, an integer.  I most commonly specify an enumeration value.  The enumerations are exported as part of the CDocument-derived class:

Because this is an interface, each interface gets a specification of what it is and does.  It is not sufficient to say "read the code".  Code is not documentation!

class CMyDocument : public CDocument{
     ...
     public:
        typedef enum {
            /*********************************************************************************
            *                   CMyDocument::SetText
            * Inputs:
            *     lHint: */ SetText, /*
            *     pHint: (CObject*)(CString*): Text to set
            * Effect:
            *     Sets the text into the edit control in the view
            *********************************************************************************/
             
            /*********************************************************************************
            *                    CMyDocument::RetrieveText
            * Inputs:
            *     lHint: */ RetrieveText, /*
            *     pHint: (CObject*)(CString*): String to fill with text
            * Effect:
            *     Retrives the text from the edit control in the view
            *********************************************************************************/
            } DocumentRequests;
                 
     ...
};

Note the clever use of /*...*/name,/*...*/ to actually include the name as part of the description.  This takes advantage of the fact that within an enumeration, there can be a comma at the end of the list and it is still syntactically correct.

Due to a major failure in thinking, the UpdateAllViews requires, as its pHint parameter, a CObject *.  A number of people are fooled by this into thinking they can only pass a CObject * value in this parameter, and watching their agony in trying to figure out how to create a CObject-derived class to hold what they need is a distressing sight.  In a rational world (have you noticed that phrase being used quite often?) this would have been specified as an LPVOID, so the simplest solution to using the pHint is to simply cast whatever pointer you want to pass as a CObject * and recast it in the OnUpdate handler back to whatever you have, so logically treat it as if it were an LPVOID parameter.  Whatever you want, you cast to a CObject * and in the handler you cast it back to the type you want. 

Key to writing a good OnUpdate handler is to keep in mind that if the <lHint, pHint> pair is <0, NULL> then you simply call the superclass handler and are done with it.  You must always assume that this pair will be sent at various times.

The sender parameter is typically ignored, unless you want to make an exception about doing something if your view invoked the UpdateAllViews method.  Note that when the send is specified from a view, the OnUpdate handler is never called for that view.  So the only reason the sender parameter might be needed is to obtain something from the view, but I consider that bad design.  If it is to be communicated, it should be part of the document.

void CMyView::OnUpdate(CView * sender, LPARAM lHint, CObject * pHint)
    {
     // handle the default case
     if(hHint == 0 && pHint == NULL)
        { /* default case */
         CView::OnUpdate(sender, lHint, pHint);
         return;
        } /* default case */

     switch(lHint)
        { /* lHint */
         case CMyDocument::SetText:
             { /* SetText */
              CEditCtrl & ctl = GetEditCtrl();
              CString * s = (CString *)pHint;
              ctl.SetWindowText(*s);
             } /* SetText */
             break;
         case CMyDocument::RetrieveText:
             { /* RetrieveText */
              CEditCtrl & ctl = GetEditCtrl();
              CString * s = (CString*)pHint;
              ctl.GetWindowText(*s);
             } /* RetrieveText */
             break;
        } /* lHint */

     } // CMyView::OnUpdate

There are several techniques worth noting here

• If the input parameters are <0, NULL> just invoke the superclass handler.
• For each case, the binding of the pHint to a type is purely local to that case.  An amateur might be tempted to write the CString * line at the top of the function, but the meaning of the pHint cannot be determined until the specific lHint is decoded, and therefore it would make no sense to assume that it is a CString *.  There is no point to declaring the CString * variable at the top of the function because it is not needed until the lHint is decoded.  Note now that each handler is completely self-contained.
• The use of the braces is mandatory because of C/C++ syntax rules inside switch statements.  I recommend a good book on C/C++ to understand the details of how this control structure works; for now, take it for granted the braces are required.  If you don't have them, you will get a compilation error that suggests it is possible to bypass the declaration.
• Because the code is completely self-contained, it is trivial to convert it to a function and simply call that function.

Note that the code is written as being totally self-contained,  the transformation to a function call is as shown below.  Note that the actual code from the case is simply cut-and-pasted without change (except for indentation):

     case CMyDocument::RetrieveText:
          OnUpdateRetrieveText(pHint);
          break;
...
void CMyView::OnUpdateRetrieveText(LPVOID pHint)
   {
    CEditCtrl & ctl = GetEditCtrl();
    CString * s = (CString*)pHint;
    ctl.GetWindowText(*s);
   } // CMyView::OnUpdateRetrieveText

You could even create a macro

#define DISPATCH_UPDATE(x) case CMyDocument::x: OnUpdate##x(pHint); break

and then write it as

switch(lHint)
    { /* lHint */
     DISPATCH_UPDATE(GetText);
     DISPATCH_UPDATE(RetrieveText);
    } /* lHint */

When you only want the active view

Unfortunately, this technique does not work well in one case, which is the case shown above.  If there are multiple views, and each view has an edit control, the operation is meaningless; every view would respond, and it would be indeterminate which view won.  But only one of those views is the one the user is currently editing.

I handle this by punting to the main frame, where I can determine the active view by using GetActiveView, or the active document, by using GetActiveDocument.

CWnd *  mainframe = AfxGetMainWnd();
CString s;
mainframe->SendMessage(UWM_RETRIEVE_TEXT, (WPARAM)&s);

Then in the mainframe handler, do

ON_REGISTERED_MESSAGE(UWM_RETRIEVE_TEXT, OnRetrieveText)

/**********************************************************************************************
*                     CMainFrame::OnRetrieveText  
* Inputs:
*      WPARAM: (WPARAM)(CString *): Place to put text
*      LPARAM: unused
* Result: LRESULT
*      Logically void, 0, always
* Effect:
*      Forwards the message to the active view
**********************************************************************************************/
LRESULT CMainFrame::OnRetrieveText(WPARAM wParam, LPARAM)
   {
    CView * view = GetActiveView();
    view->SendMessage(UWM_RETRIEVE_TEXT, wParam);
    return 0;
   } // CMainFrame::OnRetrieveText

Now each view has a handler

/**********************************************************************************************
*                   CMyEditView::OnRetrieveText
* Inputs:
*     WPARAM: (WPARAM)(CString *): Place to put text
*     LPARAM: unused
* Result: LRESULT
*     Logically void, 0, always
* Effect:
*     Retrieves the text
**********************************************************************************************/
LRESULT CMyEditView::OnRetrieveText(WPARAM wParam, LPARAM)
   {
    CEditCtrl & ctl = GetEditCtrl();
    CString * s = (CString*)wParam;
    ctl.GetWindowText(*s);
    return 0;
   } // CMyEditView::OnRetrieveText

Another case arose in the code of the HTML Edit View: there are times when the HTML text is required, obtained from the HTML Edit Control which is embedded in the CHTMLEditView class, and times when the text from the source window is required.  Note that at the point where the decision is made, the view whose text is desired might not be the active view, so the code above would not work.  In this case, I handled it very simply by having two messages: UWM_GET_SRC_TEXT and UWM_GET_WEB_TEXT (the use of the word "Web" to mean "HTML" is pervasive in the example code, so I maintained this fiction).  When the document wants the text from a particular view, it notifies the mainframe via the appropriate message.

In the mainframe, I added two entries to the message table. 

ON_REGISTERED_MESSAGE(UWM_GET_SRC_TEXT, OnGetSrcText)
ON_REGISTERED_MESSAGE(UWM_GET_WEB_TEXT, OnGetWebText)

LRESULT CMainFrame::OnGetSrcText(WPARAM wParam, LPARAM)
   {
    SendMessageToDescendants(UWM_GET_SRC_TEXT, wParam);
    return 0;
   }

LRESULT CMainFrame::OnGetWebText(WPARAM wParam, LPARAM)
   {
    SendMessageToDescendants(UWM_GET_WEB_TEXT, wParam);
    return 0;
   }

Note this sends the Registered Window Message to all descendants.  It is exceptionally important that you use only a Registered Window Message, and not a WM_USER or WM_APP message!

This particular paradigm depends critically on the fact that we have here a multi-view SDI (not, that is not a contradiction!) where each view is a unique type, and there can be no more than one instance of each view.

When frames know far too much

Here's a couple sequences of code from the mainframe. In the original main frame, in order to determine if the font name or font size should be enabled, there was code like this:

void CMainFrame::OnUpdateFontName(CCmdUI* /*pCmdUI*/)
   {
    CComboBox *pCombo = &m_wndEditBar.m_wndFontNameCombo;
    ASSERT_VALID(pCombo);

    //If Source View, don't display the font name combobox
    if(m_dwCurrentView == ID_VIEW_SOURCE)
       {
        pCombo->EnableWindow(FALSE);
        return;
       }
    else
       {
        if(m_pWebView->QueryStatus(IDM_BLOCKFMT) & OLECMDF_ENABLED)
           pCombo->EnableWindow();
        else
           pCombo->EnableWindow(FALSE);
       }

   }

What's wrong with this code?  What's right with this code?  It knows that there are views, the types of the views, that the view pointers are in specific variables in the main frame, what types the views are, and a whole lot of stuff that the main frame ought not to know about what is going on.  However, look at the rewrite:

void CMainFrame::OnUpdateFontName(CCmdUI* /*pCmdUI*/)
   {
    CView * view = GetActiveView();
    BOOL enable = (BOOL)view->SendMessage(UWM_QUERY_FONT_ENABLE);
    m_wndEditBar.EnableFontName(enable);
   }

Which do you think is going to be easier to write and maintain?  Which is more flexible?  Which does not require intimate knowledge in the mainframe about the types of the views?  Which supports abstraction?

How badly can someone code a view and still pretend to be a programmer?

Which of these makes more sense?  In the view, we find the following code in the original:

void CCharacterFunctionsView::UpdateFontCombos()
   {       
    CString szResult,szCurFmt;
    int nSel;
    short nCurSize;

    CMainFrame *pFrame = (CMainFrame*)GetParentFrame();
    ASSERT_VALID(pFrame);


    //Set font face
    CComboBox *pCombo = (CComboBox*)pFrame->GetFontNameCombo();
    ASSERT_VALID(pCombo);
    pCombo->GetWindowText(szCurFmt);
    GetFontFace(szResult);
    //only update the combo it isn't in the dropped state
    //and the font name has changed
    if(!pCombo->GetDroppedState() && szCurFmt!=szResult)
       {
        nSel = pCombo->FindString(0,szResult);
        if(nSel != CB_ERR)
           pCombo->SetCurSel(nSel);
       }

    //Set font size
    pCombo = (CComboBox*)pFrame->GetFontSizeCombo();
    ASSERT_VALID(pCombo);
    short nSize = 0;
    nCurSize = 0;

    GetFontSize(nSize);
    //only update the size combo if it is not in the
    //dropped down state.
    if(!pCombo->GetDroppedState())
       {
        szResult.Empty();
        nSel = pCombo->FindString(0,_itot(nSize,szResult.GetBuffer(5),10));
        szResult.ReleaseBuffer();
        if(nSel != CB_ERR)
           pCombo->SetCurSel(nSel);
       }

   }

Look at this code! Can you find anything in it that could possibly make sense in a view? It knows the frame, the fact that font information is represented as combo boxes, all kinds of information that is none of the business of the view! Compare its complexity to this rewrite:

void CCharacterFunctionsView::UpdateFontInfo()
   {       
    ContextInfo info;
    info.FontSize = GetFontSize();
    GetFontFace(info.FontFace);

    AfxGetMainWnd()->SendMessage(UWM_CONTEXT_CHANGE, (WPARAM)&info);
   }

Which of these is more maintainable? Which decouples any dependence of the view from the frame, organization of toolbars, internal structure of the toolbars, etc.? Let's see the handler in the mainframe for this message.

Note that this view code doesn't care how the toolbar is implemented. The details of how the code is implemented is not in the mainframe, either.

The mainframe handler is also very simple: since the mainframe knows that layout parameters are managed by the edit toolbar (and, frankly, I question why this should be so; if I had more time, I'd just send the context-change message to every toolbar in the expectation that whatever toolbars care will update themselves), it simply does

LRESULT CMainFrame::OnContextChange(WPARAM wParam, LPARAM)
    {
     m_wndEditBar.SendMessage(UWM_CONTEXT_CHANGE, wParam);
     return 0;
    } // CMainFrame::OnContextChange

Note that this does not care that a combo box is being used, or where it is, or anything else along these lines.  It only knows (and again, I question if this is good design judgment) that the edit toolbar handles edit context change information.

Of course I know the index!

Here's another little gem:

 pFrame->m_wndEditBar.GetItemRect(2,rc);

This happens to appear in the view in the original code.  Of course, like most of the poor programming exhibited (the let's-sprinkle-includes-like-pixie-dust pattern), this has no reason to exist in the view; the view should not know anything about the structure of the frame, its members, toolbars, etc.  But why is it that the view knows that index position 2 of the toolbar is the menu item it wants?  This is another example of beginning programmers failing to understand the concept of abstraction.  Since CToolBar provides a function CToolBar::CommandToIndex there is no excuse for not using the abstract control ID.  But first, the code has to be removed from the view.

Here's the complete list where random pieces of the application "know" what the index is.  It is not a single oversight, or a slight glitch; it is a systematic failure to understand decoupling of interfaces.

In my rewrite, here are the same requests:

Note that all the operations take place in the EditToolbar.cpp file, and they all use symbolic values obtained from CommandToIndex.  Which do you think is going to be more maintainable?

Let's all do totally useless allocations!

Here's a piece of code found in the original:

    CRect *prc = new CRect;
    if(!prc)
       return;
    pFrame->m_wndEditBar.GetItemRect(6,prc);
    pFrame->m_wndEditBar.ClientToScreen(prc); 

This makes me wonder how many days the programmer who wrote this had been programming.  A blunder like this should not be made by a first-semester freshman.  A programmer with any kind of training would know never to do anything like this.  It probably deals with the fact that the programmer wants to pass a pointer-to-the-CRect, and is clueless about the & operator.  This is a beginner's error of the most obvious sort.

Here is the code in its entirety:

void CHTMLEdView::PopColorMenu()
{
	//get the rect for menu
	CMainFrame *pFrame = (CMainFrame*)GetParentFrame();
	ASSERT_VALID(pFrame);
	CRect *prc = new CRect;
	if(!prc) return;

	pFrame->m_wndEditBar.GetItemRect(6,prc);
	pFrame->m_wndEditBar.ClientToScreen(prc);

	CColorDialog dlg;
	dlg.m_cc.Flags |= CC_ENABLEHOOK;
	dlg.m_cc.lpfnHook = CdlgHook;
	dlg.m_cc.lCustData = (LONG_PTR)prc;

	if(dlg.DoModal()==IDOK)
	{
		CString szColor;
		COLORREF cr = dlg.GetColor();

		//change the COLORREF into an RGB.
		szColor.Format(_T("%.2x%.2x%.2x"),GetRValue(cr),GetGValue(cr),GetBValue(cr));
		SetForeColor(szColor);
	}
}

Can you find the delete that goes with the new?  I thought not.  It turns out it is in the hook function CdlgHook:

UINT CALLBACK CdlgHook(  HWND hdlg,UINT uiMsg,WPARAM /*wParam*/, LPARAM lParam)
   {
    if(uiMsg == WM_INITDIALOG)
       {
        CHOOSECOLOR *pcc = (CHOOSECOLOR*)lParam;
        CRect *rc = (CRect*)pcc->lCustData;
        if(rc)
           SetWindowPos(hdlg, HWND_TOP, rc->right, rc->bottom,
                        0,0,SWP_NOZORDER | SWP_NOSIZE);
        SetWindowText(hdlg, _T("Choose a Foreground Color")); // TODO: move this string to STRINGTABLE
        delete rc;
       }
    return 0;
   }

But why does it have to be deleted at all?  We have here a modal dialog box; the variables in the scope of the caller will remain valid until the DoModal returns!  There is no reason to allocate or free this object!  It can simply be a stack reference.

It is also worth pointing out the near-unreadability of the parameter list; random spaces where they make no sense (after the open-paren) and missing spaces where they are essential for readability (after each comma).  And it is not at all clear what the value of /*wParam*/ might be.

The sane approach in the above code would have been to write

    CRect rc;
    pFrame->m_wndEditBar.GetItemRect(6,&rc);
    pFrame->m_wndEditBar.ClientToScreen(&rc);
    ...
    dlg.m_cc.lCustData = (LPARAM)&rc;

Of course, there is also a question about why a view would know that a frame has a member called m_wndEditBar, or that it knows that the sixth element in is the menu item desired, but we've already established that the programmer had no concept of abstraction.  But to make a gross error like doing an allocation of a rectangle instead of using a simple declaration is probably not going understand fine points of interfaces.

When moved into the toolbar, where it belongs, the code is simply

CRect r;
GetItemRect(CommandToIndex(ID_BUTTON_COLOR), &rc);
ClientToScreen(&rc);

There's a hook to this!

Consider the grotesque code that was used to reposition the color dialog box. 

    CRect *prc = new CRect;
    if(!prc)
       return;
    pFrame->m_wndEditBar.GetItemRect(6,prc);
    pFrame->m_wndEditBar.ClientToScreen(prc);
    CColorDialog dlg;
    dlg.m_cc.Flags |= CC_ENABLEHOOK;
    dlg.m_cc.lpfnHook = CdlgHook;
    dlg.m_cc.lCustData = (LONG_PTR)prc;

    if(dlg.DoModal()==IDOK)
      ...
      
UINT CALLBACK CdlgHook(  HWND hdlg,UINT uiMsg,WPARAM /*wParam*/, LPARAM lParam)
   {
    if(uiMsg == WM_INITDIALOG)
       {
        CHOOSECOLOR *pcc = (CHOOSECOLOR*)lParam;
        CRect *rc = (CRect*)pcc->lCustData;
        if(rc)
           SetWindowPos(hdlg, HWND_TOP, rc->right, rc->bottom,
                        0,0,SWP_NOZORDER | SWP_NOSIZE);
        SetWindowText(hdlg, _T("Choose a Foreground Color")); // TODO: move this string to STRINGTABLE
        delete rc;
       }
    return 0;
   }

There are many things wrong here.  The grotesque allocation of a CRect, which is unnecessary.  The fact that the lCustData assignment is cast to a LONG_PTR, in spite of the fact that the documentation clearly states that the type is LPARAM.  The use of a hook when no hook should be used.  The need to fall back to the raw API when there is no need to leave the MFC environment.  The use of  literal English text string.  This is simply grotesque code.  Any programmer with pretensions to professionalism would be embarrassed by this piece of trash.  Where was the adult supervision?

But why such a complex and convoluted solution to what is fundamentally a trivial problem?  To get different behavior from a standard dialog, simply derive a subclass of it.  I called mine CHTMLColorDialog.  It has an OnInitDialog handler in it.  The rewritten code is

void CEditToolbar::PopColorMenu()
   {    CRect rc;
    GetItemRect(CommandToIndex(ID_BUTTON_COLOR),&rc);
    ClientToScreen(&rc);

    CHTMLColorDialog dlg;
    dlg.m_cc.lCustData = (LPARAM)&rc;

    if(dlg.DoModal()==IDOK)
       {
        COLORREF cr = dlg.GetColor();
        AfxGetMainWnd()->SendMessage(UWM_SET_TEXT_COLOR, (WPARAM)cr);
       }
   }

Note how much simpler this code is than the original code.

BOOL CHTMLColorDialog::OnInitDialog()
    {
     CColorDialog::OnInitDialog();

     CRect * rc = (CRect*)m_cc.lCustData;
     if(rc != NULL)
        SetWindowPos(&CWnd::wndTop, rc->right, rc->bottom,
                     0, 0,
                     SWP_NOZORDER | SWP_NOSIZE);
     CString caption;
     caption.LoadString(IDS_COLOR_CAPTION);
     SetWindowText(caption);

     return TRUE;  // return TRUE unless you set the focus to a control
                   // EXCEPTION: OCX Property Pages should return FALSE
    }

This also raises the design issue about why the color dialog is, in fact, a modal dialog.  In a rational world, the "standard dialogs" would not be dialogs at all.   The heart of them would be ActiveX controls that implemented the concepts of file open, printer setup, color selection, font selection, and so on.  The "standard dialogs" would be dialog wrappers on these classes.  Therefore, an ordinary programmer could plunk down a color selection control in a property page or a form view with no effort.  The fact that operations such as file open are modal dialogs imposes an unnecessary burden on the programmer who does not wish to see modal dialogs.

Yes, I used a modal dialog for column selection.  If I had a bit more time, I would have done that job right.  But at some point, a personal project has to simply say "enough!" and get put aside.  I tend to favor modeless dialogs for product code.

Let's have a moment for reflection

In the original code, there was a dropdown menu that listed all of the possible "block formats" (such as Normal, Formatted, etc.) that could exist in the HTML view.  For reasons that make no sense whatsoever, this menu was managed from within the view, which meant that the view had to know that the frame had a member called m_pEditBar and that within m_pEditBar the sixth index into the menu was the location of the dropdown button for the menu bar.  Say what?  What conceivable business does the view have in knowing how the menu system is implemented, or the absolute offsets should be?  The work was split up between two functions, one of which got the dropdown dispatch (thus, knowing that the menu style was a dropdown menu) and another which then did bizarre and peculiar things to the menu system. 

My first inclination was to move all this to the mainframe, where the toolbar was handled.  But upon reflection, I realized it is none of the mainframe's business how this toolbar is implemented!  Therefore, I moved the events into the toolbar.  The only interface that was required was a protocol that said "If you need the list of array elements filled in, you will provide a pointer to a CStringArray and send a message to the mainframe, which will undertake to fill that array in, using whatever means it feels appropriate".  Then, having filled in the menu list, the menu is popped down.  The menu event is routed as a WM_COMMAND message to the control bar, which will then compute the name of the environment and send a message to the main frame to cause it to be selected.  It leaves it up the main frame to route it to the correct handler, wherever that might be.  This seems to get the right isolation of concerns.

So I take as given that the correct approach is to put all of the menu manipulation into the toolbar class, where it belongs.  The obvious response to this is that a beginner might observe that the notifications from the toolbar are sent to the parent of the toolbar, and therefore the handling has to be there.  But this is not true.  While the menu notification is initially sent to the parent, using a technique called message reflection, the message can be handled in the subclass.  The original code in the view class was

void CHTMLEdView::OnEditBarNotify(NMHDR* pNMHDR, LRESULT* /*pResult*/)
{
	TBNOTIFY* pTBNotify = (TBNOTIFY*) pNMHDR;

	switch(pTBNotify->iItem)
	{
	case ID_BUTTON_BLOCKFMT:
		PopBlockFmtMenu();
		break;
	case ID_BUTTON_COLOR:
		PopColorMenu();
		break;
	default:
		break;
	}
}

void CHTMLEdView::PopBlockFmtMenu()
{
	//re-create the popup menu every time through here
	//in case block formats change.
	if(!m_pFmtMenu)
	{
		m_pFmtMenu = new CFormatMenu;
		if(m_pFmtMenu)
		{
			if(!m_pFmtMenu->CreatePopupMenu())
				return;
		}
		else
			return;

	}
	else
	{
		m_pFmtMenu->DestroyMenu();
		if(!m_pFmtMenu->CreatePopupMenu())
			return;
	}
	
	m_pFmtMenu->ClearStringArray();
	CStringArray &ar = m_pFmtMenu->GetStringArray();
	CString curFmt;

	if(!SUCCEEDED(GetBlockFormatNames(ar)) || !SUCCEEDED(GetBlockFormat(curFmt)) )
		return;
	int nFlags;
	int arSz = (int)ar.GetSize();
	for(int i = 0; i < arSz; i++)
	{
		nFlags = MF_STRING|MF_ENABLED;
		if(ar[i] == curFmt)
			nFlags |= MF_CHECKED;
		m_pFmtMenu->AppendMenu(nFlags,FORMATBASE+i,ar[i]);
	}

	//get the rect for menu
	CMainFrame *pFrame = (CMainFrame*)GetParentFrame();
	ASSERT_VALID(pFrame);
	CRect rc;
	pFrame->m_wndEditBar.GetItemRect(2,rc);
	pFrame->m_wndEditBar.ClientToScreen(rc);

	m_pFmtMenu->TrackPopupMenu(TPM_LEFTALIGN|TPM_LEFTBUTTON,
								rc.left+3,rc.bottom,this);

}			

What is possibly sensible about any of the above code?  Here we are, deep in the view, and yet it knows about the fact that the formatting is represented by a dropdown menu, the details of the dropdown menu structure, the fact that the mainframe has a member called m_wndEditBar, that this has a dropdown for the formatting codes in position 2...is there any place here where the concepts of "abstraction" and "interface" have ever been heard of, let alone implemented?  (I didn't bother showing the PopColorMenu function, which is every bit as bad; if you have a strong stomach you can read it yourself in the MSDN example).

My rewrite moved nearly all of this code into the toolbar.  What couldn't be moved to the toolbar was the actual code to get the block formats, which is represented as the methods GetBlockFormatNames and GetBlockFormat.  These are methods of the view's superclass, CHTMLEditView.

So the interface is defined as: the menu will send to the mainframe a request UWM_QUERY_FORMATS whose WPARAM is defined as an object of type BlockFormatQuery *.  The mainframe is responsible for delivering this request somewhere, but the toolbar doesn't care.  The mainframe handles this very simply by doing a SendMessageToDescendants and expects that if there is a descendant that can handle it, said descendant will fill the information in.  Note the separation of concerns here.

So the final code was all found in EditToolbar.cpp

ON_NOTIFY_REFLECT(TBN_DROPDOWN, OnEditBarDropdown)  

void CEditToolbar::OnEditBarDropdown(NMHDR * pNMHDR, LRESULT*)
    {
     TBNOTIFY * pTBNotify = (TBNOTIFY*)pNMHDR;

     switch(pTBNotify->iItem)
        { /* item */
         case ID_BUTTON_BLOCKFMT:
            PopBlockFmtMenu();
            break;
         case ID_BUTTON_COLOR:
            PopColorMenu();
            break;
        } /* item */
    } // CEditToolbar::OnEditBarDropdown 

void CEditToolbar::PopBlockFmtMenu()
    {
     // re-create the popup menu every time through here
     // in case block formats change
     if(m_pFmtMenu == NULL)
        { /* create new */
         m_pFmtMenu = new CFormatMenu;
         if(m_pFmtMenu != NULL)
            { /* create menu */
             if(!m_pFmtMenu->CreatePopupMenu())
                return; // faild
            } /* create menu */
         else
            { /* object creation failed */
             return;
            } /* object creation failed */
        } /* create new */
     else
        { /* menu already exists */
         // Destroy it and replace it with an empty one
         m_pFmtMenu->DestroyMenu();
         if(!m_pFmtMenu->CreatePopupMenu())
            return; // failed
        } /* menu already exists */

     m_pFmtMenu->ClearStringArray();
     CStringArray & ar = m_pFmtMenu->GetStringArray();
     
     BlockFormatQuery query;                                        
     query.ar = &ar;                                                
     AfxGetMainWnd()->SendMessage(UWM_QUERY_FORMATS, (WPARAM)&query);
                                                                    
     if(!query.success)                                             
        return;                                                     

     if(ar.GetSize() > FORMATLIMIT)                                 
        { /* too many */                                            
         ar.SetSize(FORMATLIMIT);                                   
        } /* too many */                                            

     for(int i = 0; i < ar.GetSize(); i++)
        { /* add to menu */
         int nFlags = MF_STRING | MF_ENABLED;
         if(ar[i] == query.curFmt)
            nFlags |= MF_CHECKED;
         m_pFmtMenu->AppendMenu(nFlags, FORMATBASE + i, ar[i]);
        } /* add to menu */

     CRect r = GetDropdownSite(ID_BUTTON_BLOCKFMT);
     ASSERT(!r.IsRectEmpty());
     if(r.IsRectEmpty())
        return;

     m_pFmtMenu->TrackPopupMenu(TPM_LEFTALIGN | TPM_LEFTBUTTON,
                                r.left + ::GetSystemMetrics(SM_CXEDGE),
                                r.bottom,
                                this);
    } // CEditToolbar::PopBlockFmtMenu 

There were several errors in the original code.  For example, instead of FORMATLIMIT, the constant 100 was hardwired in a variety of places.  In addition, if there just happened to be more than 100 defined formats, the program would exhibit random behavior, so I added code to truncate the list (perhaps I should have extended FORMATLIMIT to 1000, a very unlikely number to exceed, but I still think it is correct to make sure there is not a bounds overflow). 

Let's make the UI visible at every level

One thing I am always deeply suspicious about is when some low-level component "knows" the implementation of some other component.  For example, in the original code, the "block format" (paragraph format) was set by having the document know that the selection is done from a menu.  It is not clear why this assumption is made; I could have chosen to rework the interface to use a dropdown instead of a popup, and the implementation of the view would have to change.  But why should the view care about how the implementation details of a tool bar?   

So the handler for the popup menu is handled in the CEditToolbar class and it converts it to a UWM_SET_BLOCK_FORMAT message which is sent to the active view.  The view handles the notification to change the block format without any concern as to how the name was obtained.  I generally feel uncomfortable enough when this concept is applied to a single CFormView, CDialog or CPropertyPage-derived class, but when it starts crossing module boundaries into views, this goes beyond what I'm willing to tolerate. 

Then there was the context menu for right-click in the HTML view.  The code was

CMenu *pMenuMain = AfxGetMainWnd()->GetMenu();
CMenu *pPopup = pMenuMain->GetSubMenu(1);

This is another example of the irrelevant details of the user interface being visible at the wrong level.  This code presumes that the main menu has a menu in its [1] position that is an edit menu.  Well, it might.  Or it might not.  And the edit menu might well have other options added, such as Find, which may or may not make sense in a right-click.  So why is this presumption made?  I suspect that this was the programmer being both "clever" and lazy, and discovering that since there is a menu that at the current instant in time resembles the desired popup menu that this menu will exist, unchanged, into the foreseeable future, with those menu items and in that position.  Things like this scare me; the number of times I've had clients make changes is sufficiently high that I would not trust any such assumption.

The simplest thing for every popup menu is to create a named menu as a menu resource, and in each menu is precisely one popup menu which is used in a right-click context.  I made this change, and the resulting code became

CMenu EditMenu;
EditMenu.LoadMenu(IDR_CONTEXT_POPUP);
CMenu *pPopup = EditMenu.GetSubMenu(0);

There are no unwarranted assumptions here.  The design is that the menu called IDR_CONTEXT_POPUP holds exactly one menu, the menu that is supposed to be popped up.  This menu is unique for this purpose.  It is not likely to be confused with some other random menu in some other random part of the user interface.  It is dedicated to the purpose of being the popup menu for this context, and nothing else.

Code safely?  Why would anyone want to do that?

Look at this particularly horrendous example of poor coding:

        szResult.Empty();
        nSel = pCombo->FindString(0,_itot(nSize,szResult.GetBuffer(5),10));
        szResult.ReleaseBuffer();

It is clear that this code was written by someone who never learned to program safely. There is potential for buffer overflow. This is apparently one of those macho look-what-I-can-write-in-one-line kind of examples, so favored by beginners and wannabes, but it is not a piece of professional production code. It can be safely written very simply as

        CString s;
        s.Format(_T("%d"), nSize);
        nSel = pCombo->FindStringExact(-1, s);

Note this also fixes several bugs, such as the misuse of FindString when FindStringExact would be the correct choice, and the failure to supply the correct first parameter, -1, which results in erroneous code.  And, interestingly, the code takes the same number of lines, is much easier to write, easier to understand, and has no possibility of a buffer overrun.  So why would anyone write code as bad as the first example?  I have no idea!

Strictly speaking, if we are into code-line-minimization, I could have written using my ToString package as

        nSel = pCombo->FindStringExact(-1, ToString(_T("%d"), nSize));

which takes only one line, and is also buffer-overrun safe.

Let's toss in some unnecessary casts!

What about

CControlBar *pBar = DYNAMIC_DOWNCAST(CControlBar,&m_wndEditBar);
if(pBar)
   {
    ShowControlBar(pBar, (pBar->GetStyle() & WS_VISIBLE) == 0, FALSE);
   }

which seems a singularly clumsy way to write

ShowControlBar(&m_wndEditBar, (m_wndEditBar.GetStyle() & WS_VISIBLE) == 0, FALSE);

Why the DYNAMIC_DOWNCAST?  I have no idea.  I think this is someone who never understood C++ type hierarchy.  There is no need to do a cast to the superclass, since any method that accepts a CControlBar will, of course, accept any class derived from CControlBar, such as the CEditToolbar. 

The final interface

The final interface was message-based, and essentially there are no inclusions of header files that are not appropriate for the functioning of the code.  Module boundaries are preserved.  Frankly, I don't think I did as good a job as I would have if I had been writing this code from scratch, but it is orders of magnitude better than the crap that was displayed as an example of how to write code.  Code as bad as was used in the original example sets a poor model for program construction, encourages the use of poor patterns, and perpetuates the illusion that if you just sprinkle enough #includes around, you have produced a product.

By adding a dozen messages, the module structures are kept isolated.  Nobody cares who sends them a message to do something, or how the information was derived; all that matters is that in response to a clean, well-defined interface, which requires no knowledge of any implementation beyond the specification of the message, the requirements of the interface will be fulfilled.

#include list

Removed 20 unnecessary dependencies

Added 8 new dependencies, well-structured

Added 8 new modules to add functionality (2 of these are cosmetic)

The views expressed in these essays are those of the author, and in no way represent, nor are they endorsed by, Microsoft.

Send mail to newcomer@flounder.com with questions or comments about this web site.
Copyright © 2000, The Joseph M. Newcomer Co. All Rights Reserved.
Last modified: June 20, 2011