Examples of the inconsitency in PenguinPlay

["Bjarke Hammersholt Roune" <asmild@post6.tele.dk>]

>Ok.
>Coding convention: It exists (see www/PenguinDoc/coding.html in cvs) and
>most of the code adheres to it. We conciously made it quite minimal -
>strong enough to make the code consistent and weak enough to avoid
>harassing the developers.
>
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.

Another of my points was that the initial "harassing" of developers is
merely a short transient thing, and the benefits are anything but transient.

>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
>context, and it should never live so long that you lose the point of
>declaration out of sight. And it makes coding really cumbersome and core
>really look ugly.
>
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.

> Some people do not realise this, but an API is also an User Interface,
>
>Can you please summarize your point here in one or two short sentences?
>I found myself unable to distill it out of the following pages. Do you

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

-Faster development of client programs
-Faster development of PenginPlay itself
-Reduction of complexity
-Greater chances of people and companies actually using PenguinPlay
-Greater chances of attracting serious developers
-Increased flow of constructive comments from visitors
-Decreases the amount of skill nessecary for utilizing PenguinPlay
-Strengthens the image of PenguinPlay as a neat, effecient and easy-to-use
library
-Strengthens the line of thougth that the PenguinPlay projects have some
kind of connection that makes it possible for them to combine effortlessly
to provide the client with a fully grown and viable one-stop solution. (as
long as the point is stressed that the different projects do not NEED each
other, this should not be a problem)

Having a well thougthout coding convention that is followed is also a way of
saying "we mean business", and yes, even if money is not involved :)

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

In the third part, I show how the disadvantages of coming up with,
implementing and geting used to a coding convention can be overcome, aswell
as what properties suchs a coding convention must have to really give an
advantage to PenguinPlay. I think I forgot one, though: unambiquity.

>mean the PPlay API is inconsistent (you said "watching PenguinFile" - do
>you mean PFile in particular or PPlay as a whole?)?
>
Well, everybody seems to have their own coding convention they for the most
part adhere to. This means that if you look at it extremely locally, only
look at one developers code, it will usually turn out to be reasonably
consistent, but with a range of sight any greater than that, I'd even go so
far as to say that PenguinPlay is rampantly inconsistent in the way the
implementation is done, and more than acceptably inconsistent in API itself
(things represented to a user that does not delve into the source).

>> 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 (?).

>> (perhaps more, perhaps less, I don't know). This means, with the
>> current state of affairs, that we will also have 14 different coding
>> conventions utilized within the same general area of code. This could
>
>Do you have an example for this inconsistency?
>
I didn't make examples in my previous mail. I should've. Actually, looking
through all the code for this kind of thing, it's *much* worse than I
thought. Notice that these are just random files I opened up to look for
inconsistensies, so there's probably many more inconsistensies than this.

Notice that I'm not trying to say that people are doing some thing in a bad
way, I'm merely pointing things out. I'll try to be as neutral and
diplomatic while doing this, especially considering that there will probably
be good reasons for doing things that I am not aware of. Inconsistencies
towards other people's code does not mean that either did something wrong,
it just means that there is an inconsistency.

Also, I'm extremely sorry if I got some of the names wrong. However, this,
again, is not an effort to show that somebody's coding style is bad, just to
show the quite large amount of inconsistency.

*** -->>> Christian Reiniger (ppfGetC.cc) <<<-- ***

Notice that in all Christian Reinigers files, there is a header that clearly
states who has written it, who has last altered it, what project it is in
(PenguinPlay) and what sub-project it is in (in this case, PenguinFile)
aswell as what exactly each file is for.

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.

Here's some of the code:

extern "C"
{
 int ppfGetC (ppFILE *stream)
 {
  ppAssert (stream != 0);

  unsigned char ReadChar;

  if (!(stream->Attribs & ppfFA_readable))
   return EOF;

  if (ppfRead ((void *) &ReadChar, 1, 1, stream) != 1)
   return EOF;
  else
   return (int) ReadChar;
 }
}

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.

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

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,
it merely states the author's (I guess it must've been written by the other
author of coding.html) strictly personal opinion. I'm not trying to step on
somebody's toes here, but this, as of my knowledge (which has, btw, been
proven to be wrong in the past :) is the truth: coding.html is, for the most
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 more
of a user-interface convention to the API, but even then, the rules it
contains are not consistent ones and they are certainly not comprehensive.

*** -->>> Adrian Ratnapala (Event.cc) <<<-- ***
(the other author of coding.html, btw)

#include "Events_Private.h"

ppBool ppeCloseBindingMap(ppeEventType type)
{

  ppeBindingMap *&map = ppeBindingMap::binding_maps[type];
  if ( !map ) return true;

  if( --(map->refcount) == 0 ){
    delete map;
    map = NULL;
  }

  return false;
}

Notice how Adrian here makes spaces inside his if's.

Christian Reiniger:        if (some expression)
Adrian Ratnapala:         if ( some expression )

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.

Notice how the prefix -- operator is used rather than the postfix.

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.

*** -->>> Adrian Ratnapala (ppgColour.cc) <<<-- ***

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",
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.

ppgARGBMode::ppgARGBMode(const ppgARGBMode& other) : ppgColourMode(other)
{
  red_mask=other.GetRedMask();
  green_mask=other.GetGreenMask();
  blue_mask=other.GetBlueMask();
  alpha_mask=other.GetBlueMask();

  red_shift=other.GetRedShift();
  green_shift=other.GetGreenShift();
  blue_shift=other.GetBlueShift();
  alpha_mask=other.GetBlueMask();
}

If this had been coded by Christian Reiniger, all the values would have been
aligned, as in this example from his code:

ppAssert (stream != 0);
ppAssert (s           != 0);

int  ReadChars   =   0;
char CurrentChar = 0;

Also, there would not be underscores in the names of the variables.

void set_mask(ppgARGBPixel& mask_loc, int& shift_loc, ppgARGBPixel mask){
  int shift = 0;
  mask_loc=mask;
  for(; mask ; mask >>=1) shift--;
  shift_loc=shift;
}

Here Adrian Ratnapala places the opening brace on the same line as the
function definition. Notice that he has not done this anywhere else (I
assume it is a mistake).

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.

*** -->>> (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.

int __ppi_Send_Event(gii_event* ev)
{
  return(giiEventSend(giiInp,ev));
}

Here we have a double underscoare in front of a name. I haven't seen this
anywhere else. The indentation used here is two spaces for every level.

*** -->>> 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".

gkniPIXMessage::gkniPIXMessage               // gkniPIXMessage copy ctor
(
    const gkniPIXMessage &src
)
: _bufLen(0),
  _buf(0),
  _dataLen(0)
{
    if (src._buf && src._bufLen && src._dataLen) {
        _buf = new byte[src._dataLen];
        if (!_buf) {
            L_EMERG << "Can't allocate buffer space.  Aborting." << endlog;
            ::abort();
        }
        ::memmove(_buf, src._buf, src._dataLen);
        _bufLen = _dataLen = src._dataLen;
    }

    return;
}

Again, we have opening braces on the same line as the if, but not opening
braces on the same line as the function definition. Notice that there is not
space between the ( )'s in the if lines: it's not if ( blah ) but if (blah).
This is consistent with the way Christian did it, but not with the way
Adrian did it.

Indentation here is 4 spaces.

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

*** -->>> colinsky (pix_message.h) <<<-- ***

#ifndef GKNPIXMESSAGE_H
#define GKNPIXMESSAGE_H

Notice how there not a leading underscore in this symbol. Both Christian and
Adrian uses leading spaces in their inclusion guards. This practice is also
supported by coding.html.

class gkniPIXMessage {

public:


//////////////////////////////////////////////////////////////////////////
    // methods

//////////////////////////////////////////////////////////////////////////

    gkniPIXMessage(byte *data = 0, u32 dataLen = 0);
    virtual ~gkniPIXMessage();
    gkniPIXMessage(const gkniPIXMessage &src);
    gkniPIXMessage & operator = (const gkniPIXMessage &src);

protected:


//////////////////////////////////////////////////////////////////////////
    // methods

//////////////////////////////////////////////////////////////////////////


//////////////////////////////////////////////////////////////////////////
    // data

//////////////////////////////////////////////////////////////////////////

    u32          _bufLen;
    byte        *_buf;
    u32          _dataLen;

};

All member variables have a leading space. This is not the case for
Christian or Adrian (i think). The first things that gets defined in the
class is the public one's. This is the opposite of how it is in Christian's
code. Things are here clearly labelled "methods" and "data". This is not the
case for neither Christian nor Adrian.


*** -->>> "pcburns" ? (ppsSampleRaw.cpp) <<<-- ***

notice the use of the file type ".cpp". This is not used by anyone else
(besides me).

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
Christian, Adrian nor Colinsky.

//==========================================================================
==
// ppsSampleRaw::Load
//--------------------------------------------------------------------------
--
// assume the sample is 8bit unsigned
//
// we could change it so that it you have to set the type first and then
// load the sample ?
//==========================================================================
==
bool ppsSampleRaw::Load(const char *filename)
{
  ifstream fin;

  try
    {
      fin.open(filename);

      if (!fin)
        throw string("Error opening file");
      fin.seekg(0, ios::end);
      m_length = fin.tellg();

      fin.seekg(0, ios::beg);

      m_data0 = new unsigned char [m_length];
      if (m_data0==0)
        throw string("Out of memory!");
      fin.read((char*)m_data0, m_length);

      fin.close();

      // default to 8bit, mono at 11024 with no looping
      Set8Bit();
      m_frequency  = 11024;
      m_loop_start = 0;
      m_loop_end   = 0;
      SetName(filename);
      return true;
    }
  catch (string& message)
    {
      fin.close();
      return false;
    }
}

As we can see, there is never anything placed on the same line as an if
statement. Indentation is 2 spaces.

*** -->>> Derek Greene (ppsSample.hpp) <<<-- ***

public:
  // constructors and destructors

  ppsSample() {}
  virtual ~ppsSample() {}

  /// Load the sample file with the given filename.
  virtual bool Load(const char* filename) = 0;
  /// Save the sample with the given filename.
  virtual bool Save(const char* filename,
                    const ppsSample& sample) const = 0;


  /// return false if position is out of range
  bool            SetLoopStart(position_t start);
  ///
  bool            SetLoopEnd(position_t end);

  ///
  void            SetFineTune(finetune_t finetune);
  ///
  void            SetVolume(volume_t volume);
  ///
  void            Set8Bit();


protected:
  int             m_bits;
  Codec           m_codec;
  frequency_t     m_frequency;

  position_t      m_length; // length of m_data0

  unsigned char*  m_data0; // either all mono data or channel 0
  unsigned char*  m_data1;

  string          m_name;

This is unlike anything anybody else is doing (I checked here). All member
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