Re: Examples of the inconsitency in PenguinPlay

[Christian Reiniger <warewolf@mayn.de>]

Bjarke Hammersholt Roune wrote:

First off - the coding standard thing is on my TODO list now - but it's not
the top item (the website for example is far more important now). So things
will develop slowly here.

>One of my points was that it's not enough to create a formal convention for
>the names of public names. By "public names" I mean things like the name of
>header files, non-internal classes and functions, static variables... That
>kind of thing.

Ok, some stricter conventions for internals are good. We'll see how strict
(-> later).

>>Hungarian notation: IMHO if your code benefits from this, then you have
>>a real problem. The type of a variable should be obvious from its

>I wholly agree with you. However, if I only represented ONE side, the
>example I was trying to make would be quite unrealistic... I do believe
>there are situations in which Hngarian Notation makes sense. I've never seen
>one where I think a full-fledged notation of this kind makes sense, but I'm
>sure it is possible to find one.

not likely for OO languages - there's simply too many types ;)

[...]

>Well, the first part was a historic foundation that supports my claim
that a >coding convention is needed.
>
>In the second part, I write about the numerous benefits of using a coding
>convention, and why these benefits are not just something I'm making up
>because I think it's a cool idea:

Ok, that's clear. We just seem to disagree on how far this convention
should go.

>I think there are are more arguments in my origional mail, but I can't
>remember all of them.

You could reread it ;-)
(sorry ;)

>advantage to PenguinPlay. I think I forgot one, though: unambiquity.

Ok, no "standard paper" should be ambiguous.

>implementation is done, and more than acceptably inconsistent in API itself

That's the point I'm primarily interested in - API inconsistency that can
be corrected by coding standards.

There's also API inconsistency in terms of design, mainly as a result of
the fact that the different parts are largely independent, but I don't know
yet how to overcome that (and whether it is neccessary). -> later.

>>> Now, on some of the teams in PenguinPlay, there are 6-7 people (?).
>>
>>Unfortunately not. Rather 1-2 developers.
>>
>I seem to remember the webpage saying something else somewhere (?).

contact.html, right?
I just updated it to say the correct things.
In general - don't trust the website until it will have been rebuilt from
scratch.

>A quote from coding.html:
>
>"Ok, the easiest is to name each header after the primary class declared in
>it (e.g. ppgGC.h contains class ppgGC) or after the part all things in the
>header belong to (e.g. ppfURLProcess.h contains the URL processing functions
>of PenguinFile). And as all our public headers are in <PenguinPlay/> the
>"pp" prefix can be omitted."
>
>So, the "pp" is optional. This means you have to look each and every time
>you want to include a file to see wheter it includes the "pp" in it's name
>or not. I would call that higly inconsistent aswell as porpuseless: we need
>to gree on either the one or the other. There's not that much of a
>difference on wheter or not to include this, but we need to all do the SAME.
>Consistency.

Ok. good point. Possible solution (more or less the current implicit
convention): The primary header of each Part (subpart), i.e.
Penguin2D.h, PenguinFile.h, ...  - no pp prefix (The Penguin... is enough
and it's clearer).
All others: pp<partletter> prefix (i.e. "ppf" for PenguinFile, "ppg" for
PenguinGraphics, "pps" for PenguinSound).

Adrian, Derek, Peter, Christof, Stephane, your voices?


[...]
>We can see here that the indentation is done with a single tab for each
>indent. Well, actually, you can't, because my mail program does not seem to
>be able to handle tags coming from the clipboard, but in the source, these
>ARE tabs.

[...other code ...]

>As the last thing, know that what is used for indentation here is spaces,
>not tabs. Christian Reiniger used a single space, Adrian Ratnapala uses 2
>spaces.

Doesn't matter. The important things are:
* when things are indented (i.e. parts after opening braces etc)
* that the indentation size/way (n tabs or spaces) is consistent within
each source file. Reading a source file with indentation width 8 (1 tab, my
setting) and immediately afterwards reading one with indentation width 2 (2
spaces, Adrian's setting) is no problem.

>Another quote from coding.html (which Christian Creinig helped write
>himself):
>
>"One thing though. I don't like tabs, they only cause trouble for people
>with different tab stops on thier editors, no matter how hard you try to get
>it right. Even if you don't agree with this, what I find intolarable is
>indentation done with a mixture of spaces and tabs, when you see code like
>that it's unreadable till you guess the tab spacing. I seem to remember
>Emacs doing this."

Remark on this one: it essentially says: indent in such a way that someone
with a different editor, with different tab size still sees nicely
formatted code.

>This kind of thing shouldn't be in a coding convention document. It's a
>higly personal remark that doesn't say anything about what people should do,
[...]
>part, not a coding convention document. A coding convention document must
>be *formal* in the way it *dictates* how people *must* do things. This is

Agreed. more or less. coding.html will be made more formal. -> later

>Notice how Adrian here makes spaces inside his if's.
>
>Christian Reiniger:        if (some expression)
>Adrian Ratnapala:         if ( some expression )

There's no problem with having both forms - even intermixed. Both are
readable, both are "consistent" in themselves (i.e. there's either no space
around the expression or one space at each side of it).

Don't make things consistent for the sake of consistency alone. The goal is
readability, and readability is not harmed by this.

>Also note the way that the opening brace is positioned on the SAME line as
>the if statement. Then notice how the opening brace that starts the function
>definition is NOT on the same line as the function definition.
>
>Also notice how the "return true" is placed on the same line as the "if
>(!map )". This would seem to be consistent with placing the opening brace on
>the same line as the if statement.

Again - the goal is readability. I often mix these forms, depending on
what's more readable in the specific situation.
For example, this:

-----
int x = strcmp (String1, String2);

if      (x > 0) return String1;
else if (x < 0) return String2;
else            return 0;
------

is certainly more readable (through the tabular  "if <-> then" look and
because the parts are not as close to each other) than this:

-----
int x = strcmp (String1, String2);

if      (x > 0)
        return String1;
else if (x < 0)
        return String2;
else
        return 0;
-----

and this (where the reader almost gets lost in the spacing):

-----
int x = strcmp (String1, String2);

if      (x > 0)
{
        return String1;
}
else if (x < 0)
{
        return String2;
}
else
{
        return 0;
}
-----

>Notice that the way Adrian Ratnapala writes comments in the header of his
>files is different from the way Christian Reiniger does it. Also note that
>the spelling "colour" is used. I *think* the standard is to use "color",

color is US english, colour is british&australian AFAIK.

>atleast that is the case in Win32. This is a point that HAS to be agreed: if
>anybody want to use the word "color/colour", should it be spelled color or
>colour.

Right.

>ppgColourMode& ppgIndexedMode::Clone()const
>{
> return *new ppgIndexedMode(*this);
>}
>
>Here, the const is places directly after the "Clone()" : no space. I haven't
>actually looked for this in other places, but if *I* had coded this, there
>would have been a space, so I would have been inconsistent with him.

Sorry, that's nitpicking.

>*** -->>> (ppi_gevents.c) <<<-- ***
>
>The header in this file didn't mention who wrote the file. This is different
>from the way both Adrian and Christian did it. Also notice that the type of
>this fie is ".c", not ".cc" as Christian and Adrian had it.

"*.c" are plain C source files, "*.cc" are C++ sources.

>*** -->>> colinsky (pix_message.cc) <<<-- ***
>
>Notice that in every one of Colinsky's files there is a quite large piece of
>text detailing the legal state of the information in the file. There is also
>author and last modification in all files. Notice that this file has not
>been prefixed with "pp".

but with "gk".
the *pix* stuff was written long ago - before PenguinPlay was named
PenguinPlay (back then it was called the "Linux GSDK" - hence the "gk"
prefix) and long before any conding standards were created.

It's not maintained anymore and should be considered "historic".

>Colinsky seems to put "::" before calling any global function. This is done
>by neither Christian or Adrian.

but will be done where needed once we use namespaces.

>*** -->>> "pcburns" ? (ppsSampleRaw.cpp) <<<-- ***
>
>notice the use of the file type ".cpp". This is not used by anyone else

Correct. Peter, Derek, can you stick to the ".cc" we use? Not urgent, but
you know... Also ".h" for headers.
BTW using something other than ".cc" or ".C" for C++ sources effectively
disables some of make's mechanisms (automatically compiling all sources of
known type).

>ppsSampleRaw::~ppsSampleRaw()
>{
>    delete [] m_data0;
>    m_data0 = 0;
>    delete [] m_data1;
>    m_data1 = 0;
>}
>
>here, all members that are deleted are also set to 0 (delete should already
>have done this, btw), even in the constructor. This is not done by

According to my docs delete isn't required to set the pointer to 0.

>Christian, Adrian nor Colinsky.

It's done by me where it makes sense (have a look through ppf_DirTree.cc
for example). 

>variables are prefixed with "m_". To declare a pointer type, the * is placed
>together with the rest of the type specifies, like so:
>
>char* string
>
>rather than:
>
>char *string

That's misguiding, as the "*" binds to the respective variable, not the
type.


	Christian
--

Drive A: not responding...Formatting C: instead