Coding Conventions

[Paolo]

In the following thread
http://celestiaproject.net/forum/viewtopic.php ... c&start=25
Christophe started an important issue.

I'm asking to the development team if is time for a more strict coding convention, specially now that many contributors are submitting patches and are working on experimentations for further improvements.

I would like to suggest the following:

- Class prefixes:
- for instance Objects in library CELUTIL should be called ut<Name>
- for instance Objects in library CELENGINE should be called en<Name>
- and so on.
Instead of class prefixes should be extended the use of namespaces:
- for instance objects in CELUTIL should be referendes as Util::<Name>
- and so on.

I haven't tested yet Doxygen with latest Celestia 1.3.1 but I think that the two classes named Parser in Celengine and CelX should give some problems.

The private member of classes (both data and methods) should have a prefix. The naming convention of Delphi is the prefix "f". Any other alternative is welcome.

The objects that require explicit destructor management should have a prefix. I suggest "dyn" (dynamic allocation).

The function parameters in function implementation should have a prefix. I suggest "_".

The function parameters names has to be reported both in the declaration and in the implementation or only in the implementation.

The function parameters name of objects passed as reference and not as copy should have a prefix . I suggest "ref" or "r".

The temporary object variables in a method o in a function should have a prefix. I suggest "tmp" or "t".

The static objects in a method or function call should have a prefix. I stuggest "sta" or "s".

And so on. Further suggestions and comments are welcome.
Perhaps many of you won't agree, but if you feel involved please send your opinion.

- Paolo

[Paolo]

I would ask an advice to experienced C++ developers.

What is your opinion about class data member encapsulation?

Do you suggest strict encapsulation implementation (all data members as defined as private or protected and accessed through Get and Set member functions), or you can admit some public data members in order to allow more readable, fast and compact coding.

I'm a little bit in trouble about this. I prefer the second option because actually I don't see any contraindication.
In CELUI I'm using a lot struct instead of class and many of the class data members are public.

Do you know if this approach can lead to problems?

Christophe in the link referenced in the previous post has spoken about spaghetti code problems. I thought that spaghetti code disappered when OOP was introduced.

I can't figure problems that cannot be solved with a correct and strict naming convention.

Probably the question is silly. Anyway thank you in advance for any answer.

- Paolo

[Christophe]

Hi Paolo,

In regards to your first message, I agree that a stricter coding convention would be a plus, and the use of namespaces would also help produce documentation. Separating the public part of headers would be nice too.

Defining and agreeing on a given standard is the easy part, refactoring all the code would be a major task, you'd need some good refactoring tool to undertake it. Celestia stands at arround 100 Klines, refactoring it is not something you can do over the week-end :-) you'll also need the commitment of all involved.

If you want to take that direction I suggest you formalize your proposed standard in more details, starting from the existing coding-standards.html file, then submit it to the developers list. Such a document would be the good place to specify the comment format to use for proper documentation generation with Doxygen.

I would ask an advice to experienced C++ developers.

What is your opinion about class data member encapsulation?

Encapsulation certainly is good coding practice. All members should be private and have accessor methods,

Do you suggest strict encapsulation implementation (all data members as defined as private or protected and accessed through Get and Set member functions), or you can admit some public data members in order to allow more readable, fast and compact coding.

I'm a little bit in trouble about this. I prefer the second option because actually I don't see any contraindication.
In CELUI I'm using a lot struct instead of class and many of the class data members are public.

Do you know if this approach can lead to problems?

Christophe in the link referenced in the previous post has spoken about spaghetti code problems. I thought that spaghetti code disappered when OOP was introduced.

That's the theory, the problem is that if the interfaces between your objects are not clearly defined, you tend to make all methods (and sometimes members) public, your code is then not strictly OO anymore, it's a mix of procedural and OO code.

That's more or less what's starting to happen in Celestia, just look how the CelestiaCore, Renderer, Observer and Universe classes are used. CelestiaCore, Renderer and Universe should be converted to singletons, and their interfaces need to be reviewed.

Another example is the CelestiaCore::charEntered method, 800 lines! All the 'case' should call just one method, which would be directly useable by the scripting and GUI code, at the moment some scripting or GUI functions call appCore->charEntered('key'), while others duplicate code from charEntered.

I can't figure problems that cannot be solved with a correct and strict naming convention.

Peace on Earth? ;-)

Probably the question is silly. Anyway thank you in advance for any answer.

Not at all, you're welcome.

[chris]

I don't care for any of the naming conventions you suggested. I think that they just add clutter and make the code less readable. The only exception I can think of is some sort of prefix for member variable names--"m_" is what I've jused in the past.

Function parameter names in declaraions: in general, I've omitted them when they're obvious, and put them in when they're useful as documentation.

I do like the idea of using namespaces for the different modules, but I don't think it's essential.

Rather than a mass renaming, efforts in cleaning up the code would be better spent in documenting class interfaces and eliminating some cruft.

I don't believe in adhering to a strictly procedural or object-oriented model. In particular, I'm opposed to shoehorning everything into the OO paradigm. Some things are more naturally exposed as simple functions.

Singletons: it probably makes sense to make CelestiaCore into a singleton. There could be times when having more than one Renderer would be useful, but the class needs some cleanup first. Universe was never intended to be a singleton, and I don't believe there's anything preventing you from creating multiple instances of Universe now, even Celestia uses just one.

Encapsulation:
While I generally favor using accessor methods for class data members, I don't do it dogmatically. With the Vector and Point classes, it would be unfortunate to have to write vec.getX() instead of vec.x.

charEntered() is a mess . . . Having scripting and GUI methods call charEntered is a big mistake, as it turns the peculiarities of the UI into an API. Bad, and it's entirely my fault . . . It's especially bad when scripts rely on particular keystroke bindings--this is roughly analgous to using formatting instead of semantic markup for documents.

--Chris

[Paolo]

If you want to take that direction I suggest you formalize your proposed standard in more details, starting from the existing coding-standards.html file, then submit it to the developers list. Such a document would be the good place to specify the comment format to use for proper documentation generation with Doxygen.

I've taken the Celestia Coding-standards.html and I've introduced my modifications. Doxygen compiant comments are included in the examples.

You can find it at the following address:
http://members.xoom.virgilio.it/pangeli ... tions.html

- Paolo

[jasonvene]

Paolo, chris and others:

I?ve recently been fascinated with Celestia 1.3.0 (now upgrading to 1.3.1). I was a member of Cfront (in 1986-87 when C++ was only a front end for a C compiler), and I?ve developed applications ranging from robotic control systems for manufacturing, 3D plugins for MAX, AutoCAD and others.

I haven?t studied the Celestia source, so my reply is more from the C++ development standpoint than with regards to specifics about your work. I?m astonished to discover, by the tone of this thread, that Celestia isn?t already an OOP work, but then I?ve been biased toward OOP thinking and working for many years so I realize I?m prone to unjust prejudice.

Chris has my empathy and agreement regarding certain code conventions, especially those introduced by Microsoft (the use of m_ for members, C to start all classes, lpsz in prefix to certain strings). I know their history and justification, but in the teams I?ve managed, and in my own writing, I?ve never found it useful. The practices of OOP development lend themselves to correcting the problems these inventions were designed to solve, and they were implemented at a time when Microsoft was clearly steeped in C, not C++ development. The ?m_? for member is particularly humorous to me, because I so rarely have any variable that is NOT a member of an object, and it doesn?t serve to answer ?a member of what,? which is most often the more valuable point, usually solved by the form ?objptr->value.? I do find myself partial to a simpler convention, whereby the member variable name's first letter is capitalized, while temporary local variables are not. Note, too, that in his books, Stroustrup never used these conventions in his examples. The strong type checking will cause the compiler to make certain you?ve not mixed a string and double where parameters are required, and derivation from a common base class is more effective at enforcing issues like dynamic allocation management, than of a ?reminder? such as a prefix convention.

All this said, however, with the admission of the fact that I?ve written in C++ for so long, I don?t think I have it in me to migrate from C (or C oriented project). I would have some difficulty citing experience of value in that particular regard.

However, one point Paolo brings up is within my scope; the use of private data members. As chris pointed out, it can be a hindrance to conversion and writing to compare vec.GetX() as opposed to vec.x. There are a great many circumstances when insisting on private variables is nothing more than dogmatic adherence, and Stroustrup himself clearly warned about such tendencies. It is more important to orient yourself into thinking from an object standpoint, where the design of the object is functional on its own.

The choice of protection begins with this basic thought; does this variable require protection from the consumer of this object? That is, when this object becomes the member of a community of code, accessed by other people (and yourself) is access to the OBJECT sufficient on its own, such that the external community of developers would be entirely served if they never new this variable existed?

In this sense, the object is a part in a machine made of logic and words, and the dials, switches and knobs handed over to the consumers of this object (which, I admit, may as well be its author as anyone else) may be best handled by member functions. Or do they need direct access to this variable? In particular, for a value that must be validated, say like the angle expressed in radians, the ?knob? available to the public should most certainly be a member function, that performs validation, adjustment (wrap around of the radian, if that?s your solution), and quite possibly a wide range of controlled manipulations of the value (that is, direct setting, increment/decrement, resulting angle after rotation over an interval of time or the result of momentum, thrust, etc.)

This is wrapped up within the notion that it many cases, the intelligent manipulation of the values (of the member variables) is exactly the leverage you want to gain from the object paradigm. The decision should not rest entirely, but should include the consideration that ?vec.x? is more convenient to use than ?vec.GetX()?. If these variables are not manipulated exclusively by member functions (i.e. making them private variables), then what opportunity exists to provide knowledge within the object? The object itself should, most often, know what to do with these variables. Those which are not protected (as in private/protected members) are often best relegated to the status of a label (the name of a planet, for example). Where variables perform a vital function, and are the subject of a great many standardized manipulations, as is often the case in simulators, letting the object take care of itself is usually quite liberating to the developer, and to the remaining members of the team (and is independently more advantageous than simplifying vec.GetX() to vec.x). You can reduce the communication required among team members to the exposed member functions, instead of rules that explain what should be done with member variables like x, y and z.

I'll repeat that thought from a different perspective. If I consider the object the central figure, rather than the variables it contains, then this is no longer a glorified struct, but something beyond what C oriented thinking can provide. I create a machine component, give an interface to the public, provide the intricate details of its operating in the member functions withheld from the public (private/protected functions), and thereby limit what they must understand to use this object, while enforcing conventions in its use, providing leverage to them by migrating the elementary and volumous details I'd rather they not tinker with into its base and body, or when they do I confine that to a common base of code which, when debugged, lends new feature to the entire work on which I may depend.

This may run counter to performance coding issues, though, and depends on the structure of Celestia I?ve not reviewed. It can be quite convenient, even necessary, to situate the population for quick processing en mass, so that the penalty of calling functions is alleviated. There are solutions, there, too, but then I?m not certain of my audience, so I?ll keep what is already a lengthy post shorter by omission just now.

If it sounds like I?ve not given an answer, I have, but in the form of a proposed question. If you are asking a question of this level, it seems safe to assume that, from my viewpoint as a veteran of 18 years in C++, there are a great many OOP ideas that, as Stroustrup points out gently in his writing on the point, might not occur to you until you?ve used OOP for several months (18 in his suggestion, though I think many catch on in 8 or 12). That's not by the calender, but by the work-thought time.

By far the most powerful and important work, for anyone designing an OOP topology for a work such as Celestia, is to create an object design. This is the central command work. It?s far more important than details about coding conventions. If you are to gain leverage in the use of OOP, it will only be through an intelligent layout of objects. In the mind, they should be as real as IC chips, connectors, motors and motherboards. You should learn to see them as parts of a machine made of logic. While I agree with chris, a dogmatic adherence to some pet OOP concept isn?t going to return much benefit, an insistence on OOP thinking and practice will.

An instructive example regarding Yaw_angle may be in order to open your mind to another possibility. Yaw_angle is, I?ll assume (and based on a previous work I did some year ago) an angle in radians of the yaw of an object in the simulator. I have several such radians in use, and generally they all follow the same rules. I need them to remain within range of one radian, so they must wrap around when incremented beyond that limit. I have many methods which might change an angle, which include a span of time and a speed of rotation, possibly a thrust applied over a span of time (say I change the rotation of the space shuttle), or perhaps I need to set the angle which most closely matches a surface normal, perhaps an angle to a target, etc.

I might, at first, think a double representing this angle should be a member of an object, I?ll say Obj3d for discussion, which I?d call Yaw_angle, and keep it private with a set of member functions to control its operation. An object Planet might derive from Obj3d, as would SpaceCraft, Station or others, and all inherit the protected properties and intelligent operation of the Yaw_angle. There is, however, yet another approach.

Recognizing that if I were to point a weapon (I?m showing my game simulating experience here) to an object in 3d space, I need to orient not only the yaw, but a pitch as well (perhaps a roll, and there can be other relative rotations involved), I might be tempted to use a second double, Pitch_angle, and have yet another set of associated member functions to control it. I could say to Obj3d, PointTo( Obj3d &) (a member function, you understand), and it would set Pitch and Yaw for me. So far so good.

Now, however, we know, too, that sometimes you?ll have yet another angle of rotation in, perhaps, the Spacecraft. It may be to express that its current direction of travel is different than the angle in which it?s facing (perhaps the poor ship?s engines have failed, it hit a meteor and is tumbling slowly while falling toward a planet ? if you?ll pardon the fiction). In any event, I see that I need the same features repeated. The word repeated should ring a bell in any OOP thinker. You never repeat code unless you have no other option.

Any time something should be repeated, that suggests an object should be present, and you should think of this as a plug in component. Perhaps it would be best, then, to create an Angle object, which is used instead of the double, which contains all protective features you require of an angle. You may still, therefore, have the benefit afforded to a protected (i.e. private) member variable, yet still reference the value of that angle in the form preferred by chris. That is;

Class Angle
{
private:
double AngleValue;
public:
// members and such follow
};

class Obj3d
{
public:

Angle Yaw, Pitch;
};

chris will be satisfied to know that he may now invoke;

Obj3d rock;

rock.Yaw = temp_yaw;

Or whatever use of Yaw he desires, while still knowing that all manipulations of Yaw are performed inside of the Angle object, under the protection of its ?knowledge? of angle usage in the simulator.

I know, this opens a whole range of things that most be done I?ve not listed. That is, the Angle object must be told just how it behaves as a double when called upon to do so. This is handled by conversion functions and assignment operators, which themselves invoke standardized means of manipulating the private variable, which is no longer a dumb double but an intelligent angle in radians. This takes the place of the simpler ?Get/Set? pair often associated with this concept.

Now, too, everywhere I use angles I have leverage of that knowledge. Obviously this works upward into the concept of the vector, which may invoke two angles and a distance, or whatever you like for an expanded vector feature set. Now I might, for example, say that an object is incremented by a vector;

Obj3d rock;
Vector3d v;

rock += v;

Obviously this implies that Obj3d has an operator function for += accepting a Vector3d reference. If I know that Vector3d handles all of the protection features I require of its values, I need not concern myself with making it a private variable when it is a member of another object. It has an interface to the world, so I need not provide another one that isolates it.

Have I offered any consult of value?

[Paolo]

Hi Jasonvene

Thank you for you explanations and for sharing your experience.
Your explanations and examples about encapsulation cleared my ideas.
Now I know when and how is better to expose data members as public in C++.

The second part about the use of objects that manage complexity inside themselves and the use of operation overloading was very interesting too.

Bye - Paolo

[Paolo]

As I wrote somewhere else I'm a Delphi amateur, so I come from a slightly different OOP perspective.

So I would like to submit some considerations.

In Delphi there is a naming convention very useful that suggest to use the "f" prefix for private and protected member functions ant the "T" prefix for every user defined type and specially for classes. These are two of the things that I miss much programming in C++.
These conventions improves readability of the code very much. So starting from this point this is the reason why I would like to have a more complete naming convention for Celestia. For celui I've used my own that is similar to the Delphi one.

Regarding encapsulation there is another beautiful construct in Delphi that misses in C++. It is the "property" statement. This is the other thing that misses me a lot.
Using C++ syntax in Delphi you can write something similar:

Code: Select all

class TColor
{
 public:
   property byte red : read fr write fSetR;

 private:
    byte  fr, fg, fb, fa ;
   
    inline void fSetR(float _v)
    {
        if (_v > 1) { fr = 255 }
        else if (_v < 0) {fr = 0}
        else {fr = floor(255 * _v) };
    };

};


And perform bounding or validation checking very friendly, because the property statement exposes a data member like name "red" in the object interface that hides the validation functions applyed to the private data "fr". Who uses the object can interact with it with a cleaner approach than "Get" and "Set" C++ like member functions.

- Paolo

[Paolo]

I've started this thread to define if Celestia needs an extension to the coding convention in order to:
- govern the new patch and contributions submission,
- improve the readability of the code,
- start with code documentation;
- use a tool like Doxygen to create code documentation in a cross platform html “browsable” format.

Doxygen should build automatically and give immediately a clear vision of the class hierarchy of the project, helping in clearing the object interfaces and hopefully keep under control the future growth of the project. you can have an idea of It taking a look to the CELUI code documentaion on my web site

So I'm asking to the development team to continue the discussion and arrive to a decision:
a) The coding and naming convention will be changed and the whole Celestia code will be updated where necessary;
b) Everything will remain the same as now.

Thank you in advance!

- Paolo

[chris]

I've started this thread to define if Celestia needs an extension to the coding convention in order to:
- govern the new patch and contributions submission,
- improve the readability of the code,
- start with code documentation;
- use a tool like Doxygen to create code documentation in a cross platform html “browsable” format.

So I'm asking to the development team to continue the discussion and arrive to a decision:
a) The coding and naming convention will be changed and the whole Celestia code will be updated where necessary;
b) Everything will remain the same as now.

- Let's keep the naming conventions as they are now.
- Some description of how to create Doxygen style comments should be added to the coding standards document.
- Existing comments should be converted to the Doxygen styles (where appropriate)
- The class interfaces need to be reviewed and made more consistent with each other; we should work to eliminate legacy cruft or otherwise broken methods. Documentation and revision of the class interfaces should occur simultaneously.

--Chris

[Paolo]

Hi Chris. Thank you for the answer.

To be able to generate the HTML code reference using Doxygen it is not difficult.

It is only needed to add comments to the code using the the following markers:

//! Instead of simple // is used for the comments that contain a short or brief description in a single line.

/*! ... */ Instead of usual /* ...*/ for the comments that contain large and detailed descriptions in more than one line.

//!< Is used for the short descriptions of data members and function parameters

An axample is the following:

Code: Select all


//! The brief description explains the purpose of the object
/*! The detailed description can extend over more than one line and
 *  should report many informations even on the implementation.
 */
class MyObject
{
 public:

    int myMember;           //!< The brief description of the data member

    //! A Brief description of the method.
    /*! A detailed description of the method
     */
    int myMethod(
        int myParameter      //!< The brief description of the parameter
        );

};



Usually brief descriptions are sufficent. Sometimes brief and detailed descriptions are reported both. The brief description will be reported in the summary of the HTML file and the detailed one will be reported below with other informations.

Doxygen allows a large variety of markers and tokens that are useful to format the comments. In my experiments I haven't used them. It is necessary to evaluate if some of them should be useful.

A beautiful feature of Doxygen is that it recognizes the names of the objects in the comments and where possible creates automatically the hyperlinks.

You can have an idea of what you can obtain taking a look to the preliminary
CELUI documentation. The code of the commented headers file is included so you can take a close look to how apply the above described rule.

- Paolo