* Ben Hanson:
I have created an open source Notepad program for Windows in C++ that allows
search and replace using regular expressions (and a few other extras). It
is located at http://www.codeproject.com/cpp/notepadre.asp
I'm trying to use best practice in my C++ programming and would appreciate
any advice anyone can give. As code is far more than just a one page
sample, just a review of one of the source files (or even just a function or
two) is equally welcome!
I'm currently a bit depressed because some folks who promised to
review something for me haven't even replied that they've not found
the time (or whatever). So I'll take a look at this, just to prove
to myself that it's no big hassle to review something. Then I can
go from depressed to perhaps a bit angry.
If you reply, could you also mail me at benjamin dot hanson at swx dot ch
Nope (see the FAQ).
Okay, first problem: in order to download the code one must provide a
username and password. I can't for the life of me remember what I chose
last time at that ****ing site. But there is a quick-register option
where I only have to supply an e-mail address. OK! But alas, "The
email you supplied has already been registered".
Now checking the mail to see if they've sent me my old username etc...
Nope. One idiot-mail from someone quoting a usenet posting of mine (in
sci.physics.res earc) and adding nothing more; one attempted virus
(automatically removed); one spam apparently from
gl**********@ju mpy.it,
"Winning Notice", yeah right.
Okay, found option to have the site mail me my old username & password.
Now checking mail again...
Nope, nothing.
Now checking mail again...
Nope, nothing.
Now checking mail again...
Nope, nothing.
Now checking mail again...
Nope, nothing.
Now checking mail again...
Nope, nothing.
Well I ain't gonna create a new e-mail account just to download your
complete code, so I'll have to review just what you display on the page.
The first thing you state about the internals is that "Notepad RE is
CEditView based". Now what is "CEditView" ? Since this is Windows I
fire up my MSDN Library. Yeah, right, there is a CEditView class in
MFC. But, MFC? Why don't you use WTL?
* Recommendation: state first of all that this is an MFC app.
Some folks might say that has nothing to do with C++. But it has.
MFC is C in C++ disguise. WTL is at least template-based, although
it's Microsoft style with e.g. unsafe construction and all that.
The first bit of actual code displayed is
void CFindReplaceDlg ::OnCheckMatchC ase ()
{
m_pParentWnd->PostMessage (WM_NOTIFY_FIND _REPLACE, eMatchCase,
m_CheckMatchCas e.GetCheck ());
}
Now this is using a Windows API service in order to update (?) the
dialog owner window's state -- I think. But using that service
(message posting) seems to be unnecessary. And it introduces type
unsafety.
Better just call a method on the parent window (or a more restricted
interface) directly.
Next, we have
BOOL CFindReplaceDlg ::OnInitDialog( )
{
CDialog::OnInit Dialog ();
m_CheckWholeWor dOnly.SubclassD lgItem (IDC_CHECK_WHOL E_WORD, this);
if (m_bInitialRegE x)
{
m_CheckWholeWor dOnly.EnableWin dow (FALSE);
}
else
{
m_CheckWholeWor dOnly.SetCheck (m_bInitialWhol eWordOnly);
}
m_CheckMatchCas e.SubclassDlgIt em (IDC_CHECK_MATC H_CASE, this);
m_CheckMatchCas e.SetCheck (m_bInitialMatc hCase);
m_CheckRegEx.Su bclassDlgItem (IDC_CHECK_REGE X, this);
m_CheckRegEx.Se tCheck (m_bInitialRegE x);
if (m_IDD == IDD_FIND)
{
m_RadioUp.Subcl assDlgItem (IDC_RADIO_UP, this);
m_RadioDown.Sub classDlgItem (IDC_RADIO_DOWN , this);
m_RadioDown.Set Check (TRUE);
if (m_bInitialRegE x) m_RadioUp.Enabl eWindow (FALSE);
m_CheckCloseOnM atch.SubclassDl gItem (IDC_CHECK_CLOS E_ON_MATCH,
this);
}
else
{
m_EditReplaceTe xt.SubclassDlgI tem (IDC_EDIT_REPLA CE, this);
m_CheckReplaceA llLikeNotepad.S ubclassDlgItem
(IDC_CHECK_LIKE _NOTEPAD,
this);
}
m_EditFindText. SubclassDlgItem (IDC_EDIT_FIND, this);
m_EditFindText. SetWindowText (m_strInitialFi nd);
return TRUE; // return TRUE unless you set the focus to a control
// EXCEPTION: OCX Property Pages should return FALSE
}
The first thing I notice is inconsistent indentation size; previously it
was 4, now it's 2.
* Recommendation: use consistent indentation size.
The OnInitDialog mess is forced upon you by MFC; nothing much one can do
about that in an MFC-based application, but otherwise, use constructors
for construction.
* Recommendation: don't use MFC, it's not type-safe.
Next thing I notice is that there seems to be no failure checking. Not
even asserts. Perhaps that's because all those methods invoked are nice
enough to throw exceptions if they fail, but remembering what MFC was
like I rather doubt it.
* Recommendation: do check for failures.
Third thing I notice is that m_IDD == IDD_FIND is repeated later on in
other code.
This suggest that you should really have two different classes, perhaps
derived from a class that encapsulates the _common_ things.
* Recommendation: do use inheritance to factor out common things,
and do use different classes for objects with different
functionality.
New function, CNotepadreDoc:: OnNotifyFindRep lace. I'll not repeat the
code here, but it's one long if-then-elseif-elsif-elsif just checking
a variable's value against a set of possible (apparently) constant
values. Now that's perfectly OK technically, but it would be more clear
(especially that no other kind of condition is present) with a 'switch':
* Recommendation: do use 'switch' for 'switch' constructions.
New function, CNotepadreDoc:: OnOpenDocument.
Here you have some decidedly non-C++ exception handling based on _old_
MFC macros.
* Recommendation: do use modern C++ exception handling. Even
Microsoft says so in their documentation of MFC!
* Check out the ScopeGuard header (Google), use ScopeGuard for the
cleanup things you here use try-catch for, and in general (but not
in this specific case) use RAII (Google) rather than try-catch.
Now that's that, I think I've done enough to change my emotional state a
bit in the right direction, and in a short time!
Cheers,
- Alf
--
A: Because it messes up the order in which people normally read text.
Q: Why is it such a bad thing?
A: Top-posting.
Q: What is the most annoying thing on usenet and in e-mail?
