What do other's make of this code?

[Steven T. Hatton]

This is something I've been looking at because it is central to a currently
broken part of the KDevelop new application wizard. I'm not complaining
about it being broken, It's a CVS images. Such things happen. The whole
subsystem is going through radical changes. I don't really want to say
what I think of the code just yet. That would influence the opinions of
others, and I really want to know how other people view these things,
without my influence on their opinions. I will say this much: I'm looking
at the overall structure of the files/class, and also at a few details such
as the for() loops.

This is the LXR:
http://lxr.kde.org/source/kdevelop/p...appwizarddlg.h
http://lxr.kde.org/source//kdevelop/...pwizarddlg.cpp

This is the API Doxygenese, which I find a bit easier on the eyes:

http://developer.kde.org/documentati...ardDialog.html

Particularly the source file presentation.

http://developer.kde.org/documentati...8h-source.html
http://developer.kde.org/documentati...pp-source.html

There are a couple structural issues which stand out as questionable in my
mind. At the same time, The code does(did) do what it's supposed to, which
is very important.

--
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

[Steven T. Hatton]

Steven T. Hatton wrote:

http://developer.kde.org/documentati...8h-source.html

http://developer.kde.org/documentati...pp-source.html
There are a couple structural issues which stand out as questionable in my
mind. At the same time, The code does(did) do what it's supposed to,
which is very important.

Due to the overwhelming response, I've decided to elaborate a bit on what I
see in this code. Before I do that, please allow me to explain a bit more
my reasons for presenting this code. The code represents real live code,
in a real application. It is non-trivial, and probably of comparatively
good quality. Among other things, I'm trying to determine how
'conventional' the code is, i.e., is this a 'normal' approach, with
'normal' idioms? I'm also interested in what others think about these
conventions (or unconventions, as the case may be).

I'll just list some observations.

1) the use of m_member_field is something I've seen a lot. I don't
particularly like it. My alternative is probably unattractive to most
programmers, and I will most likely have to capitulate to the majority on
this. The intent of the 'm_' is to distinguish the variable as a member
field. My response is, that is what /this->/ is for. I do tend to use
this->member_field when referring to members of the current class, though I
am moving away from the practice for the sake of conformity.

2) The use of /struct/s makes me a bit uncomfortable. There is a school of
thought that says anything of the nature of the /struct/s that appear in
this class should be treated as a class with private members accessed
through set and get methods. That is a very JavaBeans approach, and in
some areas, its probably a reasonable guideline to adhere to.

From what I've read in TC++PL(SE), Stroustrup would probably affirm the
approach used in the sample I've provided. From my perspective,
the /struct/s do add structure to the program without introducing a lot of
excess code consisting of one-liners. The alternative using the /struct/s
as classes with private or protected members would probably do little more
than clutter and obfuscate the code. OTOH, it requires (IMO), that the
programmer adhere to good manners, and only access the members of
the /struct/s as if they were private class members.

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. And it would have helped if the programmer
had spelled *_favorites_* correctly! ;-)

--
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

[Bob Hairgrove]

....(my comments are interspersed)...

On Fri, 28 May 2004 05:34:10 -0400, "Steven T. Hatton"
<su******@setidava.kushan.aa> wrote:

Steven T. Hatton wrote:

http://developer.kde.org/documentati...8h-source.html

http://developer.kde.org/documentati...pp-source.html

There are a couple structural issues which stand out as questionable in my
mind. At the same time, The code does(did) do what it's supposed to,
which is very important.

Due to the overwhelming response, I've decided to elaborate a bit on what I
see in this code. Before I do that, please allow me to explain a bit more
my reasons for presenting this code. The code represents real live code,
in a real application. It is non-trivial, and probably of comparatively
good quality. Among other things, I'm trying to determine how
'conventional' the code is, i.e., is this a 'normal' approach, with
'normal' idioms? I'm also interested in what others think about these
conventions (or unconventions, as the case may be).

Aside from the Qt idioms, with which I have never worked before, I
find the code highly structured and easy to follow. The HTML
documentation is lovely, with all the links. You can find almost
anything in any header by clicking on the symbol or name.

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'll just list some observations.

1) the use of m_member_field is something I've seen a lot. I don't
particularly like it. My alternative is probably unattractive to most
programmers, and I will most likely have to capitulate to the majority on
this. The intent of the 'm_' is to distinguish the variable as a member
field. My response is, that is what /this->/ is for. I do tend to use
this->member_field when referring to members of the current class, though I
am moving away from the practice for the sake of conformity.
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.

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.
2) The use of /struct/s makes me a bit uncomfortable. There is a school of
thought that says anything of the nature of the /struct/s that appear in
this class should be treated as a class with private members accessed
through set and get methods. That is a very JavaBeans approach, and in
some areas, its probably a reasonable guideline to adhere to.
It all depends IMHO on how the struct is used. Structs have their
place even within highly object oriented code.
From what I've read in TC++PL(SE), Stroustrup would probably affirm the
approach used in the sample I've provided. From my perspective,
the /struct/s do add structure to the program without introducing a lot of
excess code consisting of one-liners. The alternative using the /struct/s as classes with private or protected
members would probably do little more than clutter and obfuscate the code.
Agreed.
OTOH, it requires (IMO), that the
programmer adhere to good manners, and only access the members of
the /struct/s as if they were private class members.
Well, there is certainly nothing wrong with "good manners". <g>
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...
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.
--
Bob Hairgrove
No**********@Home.com

[Bob Hairgrove]

On Fri, 28 May 2004 14:33:46 +0200, Bob Hairgrove
<wouldnt_you_like@to_know.com> wrote:

Oops...

public:
void init();
/* etc. */
};
I forgot the arguments in the declaration, should have been:
public:
void init(const char * ,
const char * ,
const char * ,
const char * );
etc.

as well as the return type in the implementation:
void 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;
}

sorry.
--
Bob Hairgrove
No**********@Home.com

[Goran Sliskovic]

"Steven T. Hatton" <su******@setidava.kushan.aa> wrote in message
news:lq********************@speakeasy.net...

This is something I've been looking at because it is central to a currently broken part of the KDevelop new application wizard. I'm not complaining
about it being broken, It's a CVS images. Such things happen. The whole
subsystem is going through radical changes. I don't really want to say
what I think of the code just yet. That would influence the opinions of
others, and I really want to know how other people view these things,
without my influence on their opinions. I will say this much: I'm looking
at the overall structure of the files/class, and also at a few details such as the for() loops.

....

I don't think this is the right group for this, you should try in
comp.object. Code is broken because it does not obey basic rules in OO
engineering (but then again, OO may be broken also - not saying that it is
the best way). It's class metrics are poor - low cohesion, high coupling,
lot of dependencies. It tries to do too much in one place, taking care of
unrelated stuff at the single place (just count #include directives), mixing
processing, configuration, representatation etc...

Regards,
Goran

[Chris Johnson]

Chris Johnson wrote:
---8<---SNIP---8<---

blown class at this point. Classes in my opinion enforce rule
invariants of the object represented and resource allocation to
construct such an object and nothing more. Support functions, structs,

....resource allocation/deallocation to construct _AND_ destruct such...
-----= 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! =-----

[Chris Johnson]

Steven T. Hatton wrote:
---8<---SNIP---8<---

1) the use of m_member_field is something I've seen a lot. I don't
particularly like it. My alternative is probably unattractive to most
programmers, and I will most likely have to capitulate to the majority on
this. The intent of the 'm_' is to distinguish the variable as a member
field. My response is, that is what /this->/ is for. I do tend to use
this->member_field when referring to members of the current class, though I
am moving away from the practice for the sake of conformity.
You say this->foo, they say m_foo, and I say _foo
Consistency seems to be the prevailing force here that separates the
good from the bad. C++ is a strongly typed language so I don't need a
prefix for types or privileges. The compiler will kindly inform me when
I need to _read_ the code better.
2) The use of /struct/s makes me a bit uncomfortable. There is a school of
thought that says anything of the nature of the /struct/s that appear in
this class should be treated as a class with private members accessed
through set and get methods. That is a very JavaBeans approach, and in
some areas, its probably a reasonable guideline to adhere to.
That is one way to approach it. OOP, modular, procedural. One _could_
argue that you don't need to do one thing because you can use another.
More follows...
From what I've read in TC++PL(SE), Stroustrup would probably affirm the
approach used in the sample I've provided. From my perspective,
the /struct/s do add structure to the program without introducing a lot of
excess code consisting of one-liners. The alternative using the /struct/s
as classes with private or protected members would probably do little more
than clutter and obfuscate the code. OTOH, it requires (IMO), that the
programmer adhere to good manners, and only access the members of
the /struct/s as if they were private class members.
I agree with this philosophy. For example, if I have a name a string
will do. No invariants. If I have a name and an address, a struct will
do. Again, no invariants. If I have a name and an address that is
responsible for retrieving first names, last names, street, city, state,
world codes - again a struct. Use namespaces and operator overloading
etc for support. The more I add to that code the fatter and more
cluttered up I make the interface the less you _or_ I am likely to use
it in the future because it will become too specialized or complicated
to work with. If I have an employee record that will not warrant a full
blown class automatically. Now if this class, through design rules,
warrants opening it's own persistence, accessing of data & how, then it
now has all sorts of design invariants. I will typically make it a full
blown class at this point. Classes in my opinion enforce rule
invariants of the object represented and resource allocation to
construct such an object and nothing more. Support functions, structs,
etc are for that. My limited experience over the last 20 years on this
shows that is what works best for _me_. KISS
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. And it would have helped if the programmer
had spelled *_favorites_* correctly! ;-)

I have all kinds of production code in the wild at this point that I
would be absolutely be ashamed of taking ownership of in public. Some
where due to my lack of understanding, others because of political
constraints of the incompetent telling the competent what to do. I
don't think C++ is _the_ language to model as why OOP is "the one true
way." (As an example, no inference that is what you are doing) Again I
bring up the argument that OOP isn't needed because you can accomplish
the same thing with modular programming and that can be done with the
tried and true methods of procedural programming. Whichever method I
choose I can agree on one thing: I will be consistent with my choice
and that choice will be governed by modeling the problem domain at the
appropriate level. If it looks like a duck, sounds like a duck, it's
probably not a singleton representation of a widget factory that
responds to a command event object seen through an observer. I think
water is wet so what do I know?

BTW, this would probably be better suited to get more responses and more
active discussion from comp.programming. While technically using C++ as
an example the spirit of the discussion seems generically related to any
programming langauge. Just my _very_ humble opinion. As for whether
this breaks OOP principles, comp.object would be the place to ask; I'm
sure they have all sorts of opinions on the code sample provided. As for
the C++ issues of the code, I cant fault someones style. They may not
even be mandated by the author of the code; be it good or bad.

Regards,
Chris Johnson
-----= 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! =-----

[Bob Hairgrove]

On Fri, 28 May 2004 19:39:46 -0500, Chris Johnson
<li*******@itlnetUR2REMOVE.net> wrote:

---8<---SNIP---8<---
You say this->foo, they say m_foo, and I say _foo
---8<---SNIP---8<---

But the C++ standard says:

<quote>
17.4.3.1.2 Global names

Certain sets of names and fuction signatures are always reserved to
the implementation:

-- Each name that contains a double underscore (__) or begins with an
underscore followed by an uppercase letter (2.11) is reserved to the
implementation for any use.

-- Each name that begins with an underscore is reserved to the
implementation for use as a name in the global namespace.
</quote>

The footnote [65] elaborates further that "such names are also
reserved in namespace ::std (17.4.3.1)"

The "implementation" means the compiler itself or the standard
libraries (see section 2.10, paragraph 2).

You see, it's really not such a good idea to say _foo because your
compiler may have already declared this internally at global scope.
Whether or not you are treated to a diagnostic is unspecified AFAIK.
--
Bob Hairgrove
No**********@Home.com

[Bob Hairgrove]

On Sat, 29 May 2004 11:36:35 +0200, Bob Hairgrove
<wouldnt_you_like@to_know.com> wrote:

Oops again...

<quote>
17.4.3.1.2 Global names

Certain sets of names and fuction signatures are always reserved to

^^^^^^^

.... should read "function", of course ... my laptop keyboard has been
flaky recently.
--
Bob Hairgrove
No**********@Home.com

[Steven T. Hatton]

Chris Johnson wrote:

Steven T. Hatton wrote:
---8<---SNIP---8<--- [snip] You say this->foo, they say m_foo, and I say _foo
Consistency seems to be the prevailing force here that separates the
good from the bad. C++ is a strongly typed language so I don't need a
prefix for types or privileges. The compiler will kindly inform me when
I need to _read_ the code better.
There are a few advantages I find in my use of this->. For one it uses the
actual semantics designed into the language to express the same basic idea
expressed by m_foo, or _foo. It also can be informative under certain
circumstances where I don't correctly understand the context, or make a
careless mistake. And one big advantage for me is that it tends to prime
the code completion of some IDEs, to include Qt Designer, and KDevelop.

[snip] I agree with this philosophy. For example, if I have a name a string
will do. No invariants. If I have a name and an address, a struct will
do. Again, no invariants. If I have a name and an address that is
responsible for retrieving first names, last names, street, city, state,
world codes - again a struct. Use namespaces and operator overloading
etc for support. The more I add to that code the fatter and more
cluttered up I make the interface the less you _or_ I am likely to use
it in the future because it will become too specialized or complicated
to work with.
I think that really depends on the specifics. If I'm sharing the data
between classes in units which have an identifiable fixed set up data
fields, I believe a class is in oder. Sometimes breaking things down into
a lot of little pieces can lead to useless clutter, other times it can
provide us with a bunch of little 'orthogonal' building blocks with which
to implement many different designs. Sometimes what looks like it's going
to do the latter actually ends up doing the former, and vis versa.
If I have an employee record that will not warrant a full
blown class automatically. Now if this class, through design rules,
warrants opening it's own persistence, accessing of data & how, then it
now has all sorts of design invariants.
I believe you are using the term "invariant" differently than I do. But for
me, I'm a bit uncomfortable using the term in the context of computer
science. For me, an invariant is an attribute of a (mathematical) object
which is preserved under a set of 'natural' transformations. For example,
a (real) vector retains its magnitude under the group of orthogonal
rotations, and the group of translations.

Stroustrup provides the following which I can accept so long as it is
carefully adhered to when discussing computing:
http://www.giref.ulaval.ca/~ctibirna...trup-pwex.html

/************************************************
" Class Invariants

Consider a simple vector class:

class Vector {
// v points to an array of sz ints
int sz;
int* v;
public:
explicit Vector(int n); // create vector of n ints
Vector(const Vector&);
~Vector(); // destroy vector
Vector& operator=(const Vector&); // assignment
int size() const;
void resize(int n); // change the size to n
int& operator[](int); // subscripting
const int& operator[](int) const; // subscripting
};

A class invariant is a simple rule, devised by the designer of the class,
that must hold whenever a member function is called. This Vector class has
the simple invariant v points to an array of sz ints. All functions are
written with the assumption that this is true. That is, they can assume
that this invariant holds when they're called. In return, they must make
sure that the invariant holds when they return. For example:

int Vector::size() const { return sz; }

This implementation of size() looks clean enough, and it is. The invariant
guarantees that sz really does hold the number of elements, and since
size() doesn't change anything, the invariant is maintained."

************************************************/
I will add that his use of a C++ *vector* as an example of an invariant
seems potentially very misleading, though technically correct.

I'm not sure I agree with the his rule of thumb using invariance as the
determining qualification as to whether something should be a class, He
explains that here:

/************************************************

http://www.artima.com/intv/goldilocks3.html

Classes Should Enforce Invariants

Bjarne Stroustrup: My rule of thumb is that you should have a real class
with an interface and a hidden representation if and only if you can
consider an invariant for the class.

Bill Venners: What do you mean by invariant?

Bjarne Stroustrup: What is it that makes the object a valid object? An
invariant allows you to say when the object's representation is good and
when it isn't. Take a vector as a very simple example. A vector knows that
it has n elements. It has a pointer to n elements. The invariant is exactly
that: the pointer points to something, and that something can hold n
elements. If it holds n+1 or n-1 elements, that's a bug. If that pointer is
zero, it's a bug, because it doesn't point to anything. That means it's a
violation of an invariant. So you have to be able to state which objects
make sense. Which are good and which are bad. And you can write the
interfaces so that they maintain that invariant. That's one way of keeping
track that your member functions are reasonable. It's also a way of keeping
track of which operations need to be member functions. Operations that
don't need to mess with the representation are better done outside the
class. So that you get a clean, small interface that you can understand and
maintain.

************************************************/
I will typically make it a full
blown class at this point. Classes in my opinion enforce rule
invariants of the object represented and resource allocation to
construct such an object and nothing more.
I tend to think in terms of how helpful they are to the human reader in
communicating the ideas which are manifest in the program. I also often
think in terms of individual components communicating with eachother
through messages. That perspective seems to have become less emphasized in
the OO literature of the past decade than it originally was. Another
conceptual model i use is that of passing an object from one component to
another. There are thus client objects, server objects, and data objects.
It is common for an object to actually be both a client and serve (source
and sink, consumer and provider, etc.) It's less common that a server, or
client will also be the subject of a message, but it certainly happens, and
in some cases leads to very powerful solutions. But I digress...
Support functions, structs,
etc are for that. My limited experience over the last 20 years on this
shows that is what works best for _me_. KISS
I think in terms of vulnerability and dependence. That is, if I have a
reasonably restricted scope for a collection of data, and I can therefore
easily determine what operates on it, as well as what it operates on, there
is little reason to put a 'shell' around it by using private and protected
qualifers with the concomitant access methods. There are also issues of
event notification resulting from state changes. That is facilitated by
the use of access methods.

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. And it would have helped if the
programmer had spelled *_favorites_* correctly! ;-)

I have all kinds of production code in the wild at this point that I
would be absolutely be ashamed of taking ownership of in public.

I have to stress that I am not at all intending to shame the programmers who
wrote this code. As you can seem above, I will critically examine the
ideas of the most authoritative experts in a field. I emphasize: the code
accomplishes the intended purpose. Yes, the subsystem is currently
nonfunctional, but my car wouldn't run very well if someone were in the
process of replacing the head on the engine either.

I Again I
bring up the argument that OOP isn't needed because you can accomplish
the same thing with modular programming and that can be done with the
tried and true methods of procedural programming.
I'm not overly comfortable with any particular definition of "modular
programming". The way I learned things, modular programming was basically
the step beyond sequential programming with non-returning branch (goto)
statements. This is represented by languages such as C and Pascal.
Different authors seem to differ on what they intend by 'modularization'.
To Roman Mæder, modularity is something akin to Ada's modules (Mathematica
Packages to be precise.) K&R are a bit nebulous regarding the meaning. The
word modularization is referenced 6 times in the index of TCPL, but I don't
see it actually present in the referenced text. They do seem to
distinguish between module and abstract data type.

BTW, this would probably be better suited to get more responses and more
active discussion from comp.programming. While technically using C++ as
an example the spirit of the discussion seems generically related to any
programming langauge.
I really hadn't intended for it to take this theoretical direction. I was
really interested in how C++ should be used in this situation, and whether
others believe it is being used optimally.
I cant fault someones style. They may not
even be mandated by the author of the code; be it good or bad.

There is a _big_ difference, in my mind, between criticizing a particular
instance of colaboratively implemented code, and finding fault in the
people involved. These programmers have chosen a very difficult task, and
are doing wonderful things with their project. Sure it could be better,
care to help?

Back to my original objectives. I find the below use of a for statement to
be inelegant, and after looking it up in the Standard, I believe it is
either a violation of the standard, or at least undefined by the standard:

fileIt = m_pCurrentAppInfo->fileList.begin();
for( ;fileIt != m_pCurrentAppInfo->fileList.end(); ++fileIt)
{
kdDebug( 9000 ) << "Process file " << (*fileIt).source << endl;
if( m_pCurrentAppInfo->subMap[(*fileIt).option] != "false" )
{
if( !copyFile( (*fileIt).source, (*fileIt).dest,
m_pCurrentAppInfo->subMap, (*fileIt).process ) )
{
KMessageBox::sorry(this, QString( i18n("The file %1 cannot be
created.")).arg( (*fileIt).dest) );
return;
}
}
}

This is what the Standard specifies a for statement to be:
/************************************************** *
6.5.3 - The for statement [stmt.for]

-1- The for statement

for ( for-init-statement conditionopt ; expressionopt ) statement

is equivalent to

{
for-init-statement
while ( condition ) {
statement
expression ;
}
}

except that names declared in the for-init-statement are in the same
declarative-region as those declared in the condition, and except that a
continue in statement (not enclosed in another iteration statement) will
execute expression before re-evaluating condition. [Note: Thus the first
statement specifies initialization for the loop; the condition
(stmt.select) specifies a test, made before each iteration, such that the
loop is exited when the condition becomes false; the expression often
specifies incrementing that is done after each iteration. ]

-2- Either or both of the condition and the expression can be omitted. A
missing condition makes the implied while clause equivalent to while(true).

-3- If the for-init-statement is a declaration, the scope of the name(s)
declared extends to the end of the for-statement. [Example:

int i = 42;
int a[10];

for (int i = 0; i < 10; i++)
a[i] = i;

int j = i; // j = 42
--- end example]
************************************************** **/

This example, though linguistically correct borders on the obsene if not the
perverse:

/* Why is the definition of it outside the scope of the for loop? Does the
programmer intend to use it after the loop? */

QStringList::Iterator it;
for (it = m_templateNames.begin(); it != m_templateNames.end(); ++it) {
kdDebug(9010) << (*it) << endl;
//...
}

/* Uh, it would appear so, but the only valid justification for having
subsequently used the variable would have been to use the result of it's
have been modified in the for loop. */

categories.sort();
for (it = categories.begin(); it != categories.end(); ++it)
insertCategoryIntoTreeView(*it); // <- And I never ever, ever do that!
This example from the same file as the above, is correct (IMO).

for ( QStringList::Iterator it =
m_pCurrentAppInfo->openFilesAfterGeneration.begin();
it != m_pCurrentAppInfo->openFilesAfterGeneration.end(); ++it ) {
(*it).replace("APPNAMEUC", getProjectName().upper());
(*it).replace("APPNAMELC", getProjectName().lower());
(*it).replace("APPNAME", getProjectName());
}
--
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

[C Johnson]

Bob Hairgrove <wouldnt_you_like@to_know.com> wrote in message news:<ik********************************@4ax.com>. ..

On Fri, 28 May 2004 19:39:46 -0500, Chris Johnson
<li*******@itlnetUR2REMOVE.net> wrote:

---8<---SNIP---8<---
You say this->foo, they say m_foo, and I say _foo
---8<---SNIP---8<---
But the C++ standard says:

<quote>
17.4.3.1.2 Global names

I may be mistaken but if class bar::_foo is considered to be in the
global namespace I think I have much bigger problems at this point.

[SNIP]
You see, it's really not such a good idea to say _foo because your
compiler may have already declared this internally at global scope.
Whether or not you are treated to a diagnostic is unspecified AFAIK.

Fair enough but morality issues aside I am not sure that legal wrongs
have been made here. My understanding is that what namespaces,
struct/union/class/ and to a degree { //block } scopes are for and was
to prevent. Is my understanding incorrect? bar::_foo should not be a
problem, int _foo would be.

int _foo; // Bad, __very(pun intended) bad

class bar {
public:
int _foo; // This pollutes global namespace?
};

struct qux {
int _foo; // By that logic this should clash with bar for that matter
};

int main(int argc, char** argv)
{
bar b;
qux q;
b._foo = 4; // So this will result in undefined behavior?
q._foo = 2; // And this too?
_foo = 0; // This is just wrong on so many levels...
return 0;
}

I am genuinely interested if I have completely missed it here.
(Do keep in mind I don't use this style, it was only used for sake of
playing devil's advocate)

[Bob Hairgrove]

On 29 May 2004 08:15:36 -0700, de*********@excite.com (C Johnson)
wrote:

[SNIP]
I may be mistaken but if class bar::_foo is considered to be in the
global namespace I think I have much bigger problems at this point.
[SNIP]

Within the scope of your class bar's implementation code, I'll bet you
don't (need to, nor want to) write bar::_foo (also not this->_foo),
but just _foo. However, the *compiler's* _foo (if any) is already in
the global namespace. It will conflict with any unqualified _foo you
use within your class member functions' implementations. That is the
issue.

Perhaps there is a std::_foo somewhere, who knows? If you have "using
namespace std;" anywhere, such a _foo would also be vulnerable.

Hopefully you wouldn't ever need to refer to class member data outside
of the class implementation code, anyway, but are using getters and
setters (OK, "mutators" and "accessors") -- except for structs, of
course.
--
Bob Hairgrove
No**********@Home.com

[Steven T. Hatton]

Bob Hairgrove wrote:

...(my comments are interspersed)...

On Fri, 28 May 2004 05:34:10 -0400, "Steven T. Hatton"
<su******@setidava.kushan.aa> wrote:

Steven T. Hatton wrote:

http://developer.kde.org/documentati...8h-source.html

http://developer.kde.org/documentati...pp-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/...pwizarddlg.cpp
http://lxr.kde.org/source/kdevelop/p...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