By using this site, you agree to our updated Privacy Policy and our Terms of Use. Manage your Cookies Settings.
448,838 Members | 1,668 Online
Bytes IT Community
+ Ask a Question
Need help? Post your question and get tips & solutions from a community of 448,838 IT Pros & Developers. It's quick & easy.

Request for Code Review

P: n/a
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!

If you reply, could you also mail me at benjamin dot hanson at swx dot ch

Thanks!
--
~ Samba, more than a low cost File and Printer server ~

-- Let us OpenSource --
-----= Posted via Newsfeeds.Com, Uncensored Usenet News =-----
http://www.newsfeeds.com - The #1 Newsgroup Service in the World!
-----== Over 100,000 Newsgroups - 19 Different Servers! =-----
Jul 22 '05 #1
Share this Question
Share on Google+
18 Replies


P: n/a
* 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.researc) and adding nothing more; one attempted virus
(automatically removed); one spam apparently from gl**********@jumpy.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::OnCheckMatchCase ()
{
m_pParentWnd->PostMessage (WM_NOTIFY_FIND_REPLACE, eMatchCase,
m_CheckMatchCase.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::OnInitDialog ();

m_CheckWholeWordOnly.SubclassDlgItem (IDC_CHECK_WHOLE_WORD, this);

if (m_bInitialRegEx)
{
m_CheckWholeWordOnly.EnableWindow (FALSE);
}
else
{
m_CheckWholeWordOnly.SetCheck (m_bInitialWholeWordOnly);
}

m_CheckMatchCase.SubclassDlgItem (IDC_CHECK_MATCH_CASE, this);
m_CheckMatchCase.SetCheck (m_bInitialMatchCase);
m_CheckRegEx.SubclassDlgItem (IDC_CHECK_REGEX, this);
m_CheckRegEx.SetCheck (m_bInitialRegEx);

if (m_IDD == IDD_FIND)
{
m_RadioUp.SubclassDlgItem (IDC_RADIO_UP, this);
m_RadioDown.SubclassDlgItem (IDC_RADIO_DOWN, this);
m_RadioDown.SetCheck (TRUE);
if (m_bInitialRegEx) m_RadioUp.EnableWindow (FALSE);
m_CheckCloseOnMatch.SubclassDlgItem (IDC_CHECK_CLOSE_ON_MATCH,
this);
}
else
{
m_EditReplaceText.SubclassDlgItem (IDC_EDIT_REPLACE, this);
m_CheckReplaceAllLikeNotepad.SubclassDlgItem
(IDC_CHECK_LIKE_NOTEPAD,
this);
}

m_EditFindText.SubclassDlgItem (IDC_EDIT_FIND, this);
m_EditFindText.SetWindowText (m_strInitialFind);
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::OnNotifyFindReplace. 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?
Jul 22 '05 #2

P: n/a
Thanks for the prompt reply!

The ScopeGuard tip alone deserves that I return the favour. Can you post a
URL or zip?

Cheers

Ben
--
~ Samba, more than a low cost File and Printer server ~

-- Let us OpenSource --
-----= Posted via Newsfeeds.Com, Uncensored Usenet News =-----
http://www.newsfeeds.com - The #1 Newsgroup Service in the World!
-----== Over 100,000 Newsgroups - 19 Different Servers! =-----
Jul 22 '05 #3

P: n/a
* "Ben Hanson" <benjamin dot hanson at swx dot ch>:
Thanks for the prompt reply!

The ScopeGuard tip alone deserves that I return the favour. Can you post a
URL or zip?


Using Google the first real reference I found was the Norwegian
[no.it.programmering.c++] FAQ (which I contributed to), at
<url: http://utvikling.com/cppfaq/04/04/02/>; see point D.

--
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?
Jul 22 '05 #4

P: n/a
Ben Hanson wrote:
I have created an open source Notepad program for Windows in C++


Sorry.
Your code does *not* comply with the ANSI/ISO C++ standard.
Try posting your request to an appropriate Microsoft Windows newsgroup.
Jul 22 '05 #5

P: n/a
Ben Hanson wrote:
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!

If you reply, could you also mail me at benjamin dot hanson at swx dot ch

Thanks!


This may be a minority view, but I prefer to put a lot more comments in my
code.
--
Chris Gordon-Smith
London
Homepage: http://graffiti.virgin.net/c.gordon-smith/
Email Address: Please see my Home Page
Jul 22 '05 #6

P: n/a
E. Robert Tisdale wrote:
Ben Hanson wrote:
I have created an open source Notepad program for Windows in C++


Sorry.
Your code does *not* comply with the ANSI/ISO C++ standard.
Try posting your request to an appropriate Microsoft Windows newsgroup.


Isn't this a rather narrow point of view? It seems to suggest that this
newsgroup is for an elite cognoscenti, and that anyone else should go away.

Perhaps that is not what you intended, but that is the impression that comes
across.
--
Chris Gordon-Smith
London
Homepage: http://graffiti.virgin.net/c.gordon-smith/
Email Address: Please see my Home Page
Jul 22 '05 #7

P: n/a
"Chris Gordon-Smith" <ad*****@homepage.net> wrote in message
news:2k************@uni-berlin.de...
E. Robert Tisdale wrote:
Ben Hanson wrote:
I have created an open source Notepad program for Windows in C++
Sorry.
Your code does *not* comply with the ANSI/ISO C++ standard.
Try posting your request to an appropriate Microsoft Windows newsgroup.


Isn't this a rather narrow point of view?


By necessity, the topic of this group is as 'narrow' as the
standard C language. If it were otherwise, there are so
many platform-specific and third-party libraries and interfaces,
etc. that it would be nigh impossible for those interested only
in the language (the topic here, after all) to locate posts
about it.
It seems to suggest that this
newsgroup is for an elite cognoscenti,
It's for those who discuss ISO standard C, regardless of their
knowledge and skill levels with it. If you consdier that group
to be 'elite', so be it.
and that anyone else should go away.
Anyone is welcome to read and post topical articles here.
Perhaps that is not what you intended, but that is the impression that comes across.


Robert appears to frequently annoy many folks here, but imo
this time he's right. Perhaps one might find his post lacks
tact.

But think about it. In order to intelligently discuss Ben's
code, one need to understand and discuss the Windows API,
which has *nothing* to do with ISO C. Ben would get much
better and high quality responses from Windows experts by
posting to a Windows group, e.g. comp.os.ms-windows.programmer.32
(I read that group fairly frequently, and there are a few imo quite
impressive 'gurus' that answer questions there.

If I want high quality information, advice, or assistance with
ISO C, I post here. Here is where the C experts discuss C
(not Windows or anything else).
-Mike
Jul 22 '05 #8

P: n/a
"Mike Wahler" <mk******@mkwahler.net> wrote in message news:dpmFc.2982>

.... some stuff.

Change all occurrences of C to C++.
Sorry about that.

-Mike
Jul 22 '05 #9

P: n/a
Chris Gordon-Smith wrote:
E. Robert Tisdale wrote:
Ben Hanson wrote:
I have created an open source Notepad program for Windows in C++
Sorry.
Your code does *not* comply with the ANSI/ISO C++ standard.
Try posting your request to an appropriate Microsoft Windows newsgroup.


Isn't this a rather narrow point of view?
It seems to suggest that this newsgroup is for an elite cognoscenti,
and that anyone else should go away.


The opposite is true.
Ben Hansen's implementation is restricted to "an elite cognoscenti"
consisting of Microsoft Windows C++ programmers.
No one else can actually compile his code much less comment on it.

Ben should submit his request for a code review to a forum
consisting of "an elite cognoscenti"
that specializes in Microsoft Windows C++.
Perhaps that is not what you intended,
but that is the impression that comes across.


Subscribers to the comp.lang.c++ newsgroup have wider interests
than Microsoft Windows C++.
Perhaps you don't mean to be inconsiderate or disrespectful of them
but that's the impression that comes across.
Jul 22 '05 #10

P: n/a
* Mike Wahler:
"Chris Gordon-Smith" <ad*****@homepage.net> wrote in message
news:2k************@uni-berlin.de...
E. Robert Tisdale wrote:
Ben Hanson wrote:

> I have created an open source Notepad program for Windows in C++

Sorry.
Your code does *not* comply with the ANSI/ISO C++ standard.
Try posting your request to an appropriate Microsoft Windows newsgroup.
Isn't this a rather narrow point of view?


By necessity, the topic of this group is as 'narrow' as the
standard C language.


Well, well, well.

Ben Hanson asked for review of the C++ aspects, not Windows programming,
not C programming.

Robert appears to frequently annoy many folks here, but imo
this time he's right. Perhaps one might find his post lacks
tact.
I think Robert-discussions are off-topic here.
But think about it. In order to intelligently discuss Ben's
code, one need to understand and discuss the Windows API,
Not at all.

By that argument no real code could be discussed in this group.
Ben would get much
better and high quality responses from Windows experts by
posting to a Windows group, e.g. comp.os.ms-windows.programmer.32
(I read that group fairly frequently, and there are a few imo quite
impressive 'gurus' that answer questions there.
Bah. I'm better than most of those top-posting idiots... ;-) But
in this group we should discuss C++, not system XYZ programming.
If I want high quality information, advice, or assistance with
ISO C, I post here. Here is where the C experts discuss C
(not Windows or anything else).


Again, Mike, "here" is [comp.lang.c++], no cross-posting.
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?
Jul 22 '05 #11

P: n/a
Crossed wires there!

I found http://www.cuj.com/documents/s=8000/...r/alexandr.htm
yesterday and as I say just this tip alone deserves I return the favour and
review your code.

Do you have a URL or zip for _your code_, so I can review it?

Thanks

Ben
--
~ Samba, more than a low cost File and Printer server ~

-- Let us OpenSource --
-----= Posted via Newsfeeds.Com, Uncensored Usenet News =-----
http://www.newsfeeds.com - The #1 Newsgroup Service in the World!
-----== Over 100,000 Newsgroups - 19 Different Servers! =-----
Jul 22 '05 #12

P: n/a
Hi there,

I'd like to present my own view.

Yes, I deveop on Windows and yes, all the GUI parts of the code are Windows
specific. However, C++ interests me as a language and the cross platform
possibilities have always interested me, even though I've not had the
occaision to port my code to Linux etc.

All the points Alf raised are interesting and he really caught my interest
with his comment on try-catch and ScopeGuard. This is a perfect example of
how the old Microsoft style can be replaced with a modern C++ style. This
is of great interest to me.

Originally the code used a C++ wrapper to Henry Spencer's regular expression
library and some C code i wrote to do string matching for non-regex
searching. The search routine was very MS like and not particularly
readable. Since then I have switched to the boost library for regex support
and std::search from the C hack I was using previously. If nothing else
these two techniques are platform independant.

I've contacted both Bjarne Stroustrup and John Maddock (boost regex) and
they've both given me great advice more than once. I can tell you that
Bjarne was very concerned when I was having problems with
std::reverse_iterator in Visual C++ 2002. Cross platform compatibility is
one of his highest priorities and he tests the Microsoft compilers for
compliance as well as all the GNU stuff etc. If it's good enough for him...

And finally, I've worked with Microsoft zealots that bitched and moaned
about C++ and complained that the C++ Standard Library was too hard, too
buggy, too slow, pointless etc. (Gosh it wasn't written by MS, so it must
be rubbish, right?). You won't be surprised to learn those guys switched to
VB, and good riddance. I imagine a new generation will be delighted to
switch to C# - I'm not one of those people either. I won't even get into
Java, or we'll be here all day/night!

Forgive my choice of platform, but I really wanted comments on my C++, not
my choice of Windows API calls... It's not fun to be "shot by both sides",
both by the frothing MS zealots on the one hand and the GNU/Linux/ANSI C++
(please delete as appropriate) crowd on the other. Apparantly, even
releasing open source just isn't good enough for some people if it's not for
their approved platform.

Well I guess by now you realise how I feel about this issue..!

Kind Regards

Ben Hanson
--
~ Samba, more than a low cost File and Printer server ~

-- Let us OpenSource --
-----= Posted via Newsfeeds.Com, Uncensored Usenet News =-----
http://www.newsfeeds.com - The #1 Newsgroup Service in the World!
-----== Over 100,000 Newsgroups - 19 Different Servers! =-----
Jul 22 '05 #13

P: n/a

I like to do the following:

namespace Windows { #include <windows.h> }

that's keeps all of their crap away from your stuff, unfortunately though,
the macros still get through.
-JKop
Jul 22 '05 #14

P: n/a
* "Ben Hanson":

Do you have a URL or zip for _your code_, so I can review it?


Thanks, mailed it (it's not for public consumption, at least not yet).

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?
Jul 22 '05 #15

P: n/a
* JKop:

I like to do the following:

namespace Windows { #include <windows.h> }

that's keeps all of their crap away from your stuff, unfortunately though,
the macros still get through.


Unfortunately you'll have to add to your work-around definitions every
time you use something new from the API (relative to earlier work).

Yes it can be done.

No, in my experience it's not really worth it; it's a lot of work.

--
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?
Jul 22 '05 #16

P: n/a
Alf P. Steinbach posted:
* JKop:

I like to do the following:

namespace Windows { #include <windows.h> }

that's keeps all of their crap away from your stuff, unfortunately
though, the macros still get through.


Unfortunately you'll have to add to your work-around definitions every
time you use something new from the API (relative to earlier work).

Yes it can be done.

No, in my experience it's not really worth it; it's a lot of work.


namespace Windows { #include <windows.h> }

int main()
{
Windows::MessageBox("Hello World", ..... );
}
Seems simple enough to me!
Jul 22 '05 #17

P: n/a
* JKop:
Alf P. Steinbach posted:
* JKop:

I like to do the following:

namespace Windows { #include <windows.h> }

that's keeps all of their crap away from your stuff, unfortunately
though, the macros still get through.


Unfortunately you'll have to add to your work-around definitions every
time you use something new from the API (relative to earlier work).

Yes it can be done.

No, in my experience it's not really worth it; it's a lot of work.


namespace Windows { #include <windows.h> }

int main()
{
Windows::MessageBox("Hello World", ..... );
}
Seems simple enough to me!


It isn't.

Wrapping library headers in a namespace nearly always means you have to
add your own work-around definitions of various things, and all that work
is for nothing when the next version of the library is released.

For the header in question there are zillions such things.

A naive first attemp can look like the code below (btw. now that I see that
code again I'm a bit ashamed of having written such thing: it does not
properly check pre-conditions and it fails to #define the relevant macro
to obtain standard library compatibility from that header).

Then you'll find more and more and more that requires "intervention" (C++
legality aside)...

// Tabs = indents = 4

//////////////////////////////////////////////////////////////////////////////
//
// Module CPP_WINDOWS
// Interface
//
//
// Wraps the [windows.h] header file, providing C++ compatible declarations.
// Also, places [windows.h] declarations in namespace "Api".
//
// Copyright (c) 1998 Alf P. Steinbach
//

#ifdef CPP_WINDOWS_H
#error [cpp_windows.h] included twice.
#else
#define CPP_WINDOWS_H
#endif


//------------------------------------------ Dependencies:
#ifndef FRAMEWORK_MACROS_H
#include <misc/framework_macros.h>
#endif

#ifdef _INC_WINDOWS
#pragma message( COMPILE_MESSAGE_WARNING "[windows.h] is already included." )
#else
// Ensure C++ compatible declarations.

#define STRICT
// Include all standard C library headers used by <windows.h> in the
// global namespace. Including them in the "std" namespace is incompatible
// with <windows.h> anyway.

#ifndef _INC_STDARG
#include <stdarg.h>
#endif

#ifndef _INC_STRING
#include <string.h>
#endif

#ifndef _INC_CTYPE
#include <ctype.h>
#endif

#ifndef _INC_STDLIB
#include <stdlib.h>
#endif
// Place <windows.h> declarations in namespace "Api".

BEGIN_API_NAMESPACE
#include <windows.h>
END_API_NAMESPACE
// Redefine macros that refer unqualified to types defined in namespace.
// E.g., IDI_WINLOGO is defined in terms of MAKEINTRESOURCE.

#undef MAKEINTRESOURCEA
#undef MAKEINTRESOURCEW
#undef MAKEINTRESOURCE

#define MAKEINTRESOURCEA(i) (Api::LPSTR)((Api::DWORD)((Api::WORD)(i)))
#define MAKEINTRESOURCEW(i) (Api::LPWSTR)((Api::DWORD)((Api::WORD)(i)))
#ifdef UNICODE
#define MAKEINTRESOURCE MAKEINTRESOURCEW
#else
#define MAKEINTRESOURCE MAKEINTRESOURCEA
#endif // !UNICODE

#endif



//------------------------------------------ Code:

// None.

--
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?
Jul 22 '05 #18

P: n/a


Once again, macros are the devil.
-JKop
Jul 22 '05 #19

This discussion thread is closed

Replies have been disabled for this discussion.