Re: What do other's make of this code?
From: Steven T. Hatton (susudata_at_setidava.kushan.aa)
Date: 06/01/04
- Next message: tom_usenet: "Re: Templates, syntax and allocation!"
- Previous message: tom_usenet: "Re: RTTI typeinfo pointers"
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
Date: Tue, 01 Jun 2004 12:24:01 -0400
Bob Hairgrove wrote:
> ...(my comments are interspersed)...
>
> On Fri, 28 May 2004 05:34:10 -0400, "Steven T. Hatton"
> <susudata@setidava.kushan.aa> wrote:
>
>>Steven T. Hatton wrote:
>>
>>
http://developer.kde.org/documentation/library/cvs-api/kdevelop/html/appwizarddlg_8h-source.html
>>>
>>http://developer.kde.org/documentation/library/cvs-api/kdevelop/html/appwizarddlg_8cpp-source.html
>
> Aside from the Qt idioms, with which I have never worked before, I
> find the code highly structured and easy to follow.
>
The Qt stuff in this particular code (and in most cases) probably does about
what you would expect by looking at the name.
> >The HTML
> documentation is lovely, with all the links. You can find almost
> anything in any header by clicking on the symbol or name.
They did a good job dressing up the doxygen. It's easy to produce
documentation with doxygen, but to make it really pretty is an art.
> However, there are other issues. Not having a lot of time to invest in
> this code critique, here is just one item which I noticed immediately:
>
> I would highly recommend against using literal hard-coded values,
> replacing those with constants. There are string literals sprinkled
> throughout the code, but also numeric literals -- e.g. the switch
> statement in the implementation of the
> AppWizardDialog::licenseChanged() function. Code such as this is a
> nightmare to maintain, for example, if the text needs to be translated
> into another language.
I came back to this message because this statement about translation stuck
in my mind. AFAIK, the original programmer is a native Germans speaker. I
appreciate your point about hard coded constants in general. Before
rereading your comments I had understood you to be saying the use of the
hardcoded text (the actual license boilerplates) was questionable. I'd have
to go 50/50 on that as a general rule. That is, hard coding the text into
the source code, rather than, say, putting it in a configuration file, is a
hard call. It requires more up-front work to set up the config file, and
make sure that resource gets managed in the future. OTOH, if the content
of the text is voletile, you don't want to recompile a lot.
The reason this whole subject stuck in my mind is that, as a general rule,
the KDE is produced in several different natural languages, and they use a
fairly effective approach to accomplish. The hard-coded text in the files
under discussion is an exception, rather than the rule.
> At one time, I also didn't like the "m_" prefix. However, it is very
> desirable to distinguish between member names and local names within
> the body of a member function's implementation code, and it is
> certainly a lot less effort to type "m_" instead of "this->" in front
> of the name.
I don't see either as much effort. My biggest reservation about using
this-> comes from situation where I am using a lot of member variable in an
invocation. That can get long in a hurry.
> At the company where I used to work, I got involved in refactoring our
> C++ programming guidelines. The guidelines recommended using an
> underscore prefix for member data names which goes against the rule
> which reserves such names for implementations of the C++ compiler and
> the standard libraries. So we changed this to the "m_" instead of
> leading underscore and everyone was pretty happy with it.
>
> One of our developers didn't use *any* prefix. He insisted on writing
> stuff like this:
>
> class foo
> {
> private:
> std::string name;
> std::string addr;
> std::string city;
> std::string state;
> public:
> void init();
> /* etc. */
> };
>
> foo::init(const char * name,
> const char * addr,
> const char * city,
> const char * state)
> {
> this->name = name;
> this->addr = addr;
> this->city = city;
> this->state = state;
> }
>
> Is this a recipe for disaster or not? At the very least, it is highly
> confusing, even if it might actually work.
I disagree. I do that regularly in Java. At first I held your position,
but after the exercise of inventing variable names for the (conceptually)
same variable, I came to believe it makes more sens to use the above
approach.
>>3) This is the biggest issue I have with the AppWizardDialog class: there
>>is
>>a Favo(u)rites class yearning to stand on its own. I put a little effort
>>into trying to refactor the code by creating a separate Favourites(sic)
>>class. It is not straight forward at this point to do so. In the current
>>situation, the AppWizardDialog seems unnecessarily cluttered with code
>>used to manage the favourites list.
>
> Funny, I couldn't find the class you refer to...
I hadn't realized the links above are to the current production code which
doesn't have support for favourites. This is the development source.
http://lxr.kde.org/source//kdevelop/parts/appwizard/appwizarddlg.cpp
http://lxr.kde.org/source/kdevelop/parts/appwizard/appwizarddlg.h
Unfortunately the CVS API documentation server has been taken offline for
the time being.
>>And it would have helped if the programmer
>>had spelled *_favorites_* correctly! ;-)
>
> "Favour" and "favourite" are both correct, as are "favor" and
> "favorite". One is the British spelling, the other is the U.S.
> American spelling. Same holds true for "colour" and "color" as well as
> scores of other words.
Yes, I am aware of that. I actually had a lot of problem with that. My
formal education as a child was very limited. Before I began college, I
undertook a selfdirected remedial education. Part of that consisted of
reading a lot of Bertrand Russell's writing. I based a lot of my spelling
on what I saw in _A_Histor_of_Western_Philosophy_, etc. In some cases I was
aware of the differences, but often I found myself a bit confused as to why
I thought a word should be spelled differently than what I encountered in
American texts. I was merely try to inject a bit of humour into the
conversation.
-- STH Hatton's Law: "There is only One inviolable Law" KDevelop: http://www.kdevelop.org SuSE: http://www.suse.com Mozilla: http://www.mozilla.org
- Next message: tom_usenet: "Re: Templates, syntax and allocation!"
- Previous message: tom_usenet: "Re: RTTI typeinfo pointers"
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
Relevant Pages
|
