Difference between revisions of "Coding Standards"

From MythTV Official Wiki
Jump to: navigation, search
(Sections headers should start at level 2. MythTV:Manual of Style)
m (Protected "Coding Standards": This generally isn't for user-editing ([edit=autoconfirmed] (indefinite) [move=sysop] (indefinite)))
(38 intermediate revisions by 15 users not shown)
Line 1: Line 1:
== Coding Standards ==
+
= Design guidelines =
  
The rules of thumb on this page are meant to help developers ensure that their code fits in with the normal conventions in the MythTV source.  These standards are mostly voluntary, and there's plenty of existing code that doesn't follow these standards, but you can more easily produce readable and understandable code if you keep these items in mind.
+
Here's some advice to keep in mind while designing or updating code:
  
For more of an overview of the development process for Myth, see the [[MythTv Development How To]].
+
* '''Use C++.''' Avoid outdated C constructs like TRUE/FALSE macros, C-style casting etc. One or two C-isms are preferred, like using NULL over the C++ preference of 0 (see this [http://svn.mythtv.org/trac/ticket/5777 ticket]). Myth supports gcc 2.95.x and above.
 +
* '''Encapsulate Well''' Make a Get method const. Return a copy and not a reference. Use Set methods, when defined in the header these entail no efficiency loss and allow you do add debugging code when necessary. Make local functions static and constants static const.
 +
* '''Use Qt for non-GUI tasks.''' Don't design your own helper classes or use platform-dependent code when Qt already offers a class.  Strings, containers (lists etc) and mutexes are good examples of Qt's generic functionality. The current development version of MythTV requires Qt 4.8, so check your calls against the [http://qt-project.org/doc/qt-4.8/classes.html Qt 4.8 documentation] so you don't break things.
 +
** We have our own replacement for QProcess called MythSystem. QProcess has had an unfixed deadlocking bug in Qt 4.0 through the time of the writing of this guide (4.7).
 +
** We have our own logging macro LOG, use this instead of the Qt debugging output macros and cerr.
 +
* '''Use [[MythUI]] for GUI tasks.''' We are currently working on a UI layer that's independent of Qt ([[MythUI]]), and patches that use native Qt widgets or the old libMyth code will be rejected. Use libmythui/* instead.
 +
* '''Stay platform-independent.''' Use Qt instead of #ifdefs where possible. If you must add something that only works on one platform, add a compile-time flag so that it's only built where it works.  See settings.pro for several examples.
 +
* '''Avoid dependencies.''' Dependencies are added very rarely to Myth, so think carefully before you suggest a new item.  If you want to link to a library that's small and has unique functionality, you may be able to get it added to the core Myth build.  In general, though, avoid external dependencies if at all possible.
 +
* '''Use private classes to keep ''popular'' header files stable.'''  This design pattern is used in MythContext and others; most of the private data and methods are stored in a private class, defined in the .cpp file.  A pointer to this class is stored as a member variable in the public class.  This way, the private data can change without changing the header file, which prevents everyone from having to rebuild most of the source tree for a popular header like mythcontext.h.
 +
* '''Don't use assert() or abort()''' Crashing is never good. Use proper error handling in place of assert. This doesn't mean you can't use asserts during development, just that by the time the code is ready for commit there should be no need for them.
 +
* '''Don't use C++ exceptions''' Unlike say Java, C++ doesn't have any way to tell the user of a method what exceptions must be caught, but gcc does let you mark a return value as something that must be examined. To deal with exceptions that can occur during object construction use either two part initialization or a static factory method that returns NULL when it fails.
 +
* '''Try to keep style coherent''' If a class prepends all it's member variables with _ rather than the "m_" don't add a variable with "m_", use the class style unless you are doing a major rewrite an renaming all the existing variables.
 +
* '''Make large style changes in a separate patch from functional changes''' This makes it easier for us to comb through the history looking for the commit that introduced the bug.
 +
* '''QString''' and other Qt container classes are not thread-safe. You can not use them in multiple threads without locking as you might a "const char*" string or a std::string. As of Qt4 assigned copies can be used in another thread, so something like this is now safe "QString MyClass::GetString(void) const { QMutexLocker(&m_lock); return myString; }" You may run across code like this from the Qt3 days, "QString MyClass::GetString(void) const { QMutexLocker(&m_lock); QString tmp = myString; tmp.detach(); return tmp; }" The detach() is no longer required but is a relic of our semi-automated conversion from Qt3 to Qt4.
 +
* '''Don't use Qt signals and slots in backend code''' There are a number of gotchas with signals and slots and we're decided to use them only in UI code as a result of dealing with all the hard to debug crashes that have resulted.
 +
* '''Locking order''' Most classes with more than one lock/mutex/semaphore will have [[Locking_Order|locking order]] comment. You must follow this locking order to avoid A,B;B,A deadlocks. If you create a class with more than one lock, document your locking order.
 +
 
 +
For more of an overview of the development process for Myth, see [[MythPlugin Architecture]].
 +
 
 +
= MythTV Style =
 +
 
 +
The rules of thumb on this page are meant to help developers ensure that their code fits in with the conventions used in the MythTV source. The goal is to enhance readability. After your code becomes a part of MythTV many other hands are likely to touch it and they will be following these conventions so if you apply some other convention the code will over time become more difficult to read. Do not apply these conventions to externally maintained code, such as ffmpeg, try your best to follow their conventions and whenever possible get your changes to the external code applied upstream as soon as possible.  
  
=== Spaces and braces ===
+
== Spaces and braces ==
  
 
The one hard and fast rule of Myth development: don't use tab characters.
 
The one hard and fast rule of Myth development: don't use tab characters.
Line 20: Line 41:
  
 
<pre><nowiki>
 
<pre><nowiki>
enum MyEnum {
+
typedef enum my_enum {
     TEST_ONE,
+
     kTestOne,   ///< This represents blah
     TEST_TWO,
+
     kTestTwo,   ///< Widget Blah
     TEST_THREE
+
     kTestThree, ///< Blah Blah
};
+
    kTestSentinel,
 +
} MyEnum;
  
static inline void runThreeTimes(QStringList strings,
+
static inline void run_three_times(const QStringList &strings, const char *buffer)
                                                  char *buffer)
+
 
{
 
{
 
     for (int i = 0; i < 3; i++)
 
     for (int i = 0; i < 3; i++)
 
     {
 
     {
         result = MyFunc(i, strings);
+
         MyEnum result = my_func(i, strings);
         if (result == TEST_ONE ||
+
         if (kTestOne == result || kTestTwo == result)
            result == TEST_THREE)
+
        {
+
 
             DoSomething();
 
             DoSomething();
             DoMore();
+
        else
 +
             DoTheOpposite();
 +
 
 +
        result = my_other_func(result, strings);
 +
        if (kTestOne == result || kTestTwo == result)
 +
        {
 +
            if (DoSomething())
 +
                DoMore();
 
         }
 
         }
 
         else
 
         else
 +
        {
 
             DoTheOpposite();
 
             DoTheOpposite();
 +
        }
 
     }
 
     }
 
}
 
}
 
</nowiki></pre>
 
</nowiki></pre>
 +
 +
Note: There are some things above that are not required by the coding standard. Using <em>result == kTestOne</em> is equally valid as <em>kTestOne == result</em>. The reason some developers prefer it is because it avoids introducing a "=" typo bug. If you run across the variation you don't prefer, leave it alone.
  
 
For Vi(m) users: the following options can be put into your ~/.vimrc file -  
 
For Vi(m) users: the following options can be put into your ~/.vimrc file -  
* set expandtab
+
<pre><nowiki>
* tabstop = 4
+
set expandtab
* shiftwidth = 4
+
set tabstop=4
 +
set shiftwidth=4
 +
</nowiki></pre>
  
 
And if it's not too intrusive, this line can appear in the source code, and Vim will apply the correct settings:
 
And if it's not too intrusive, this line can appear in the source code, and Vim will apply the correct settings:
Line 54: Line 86:
 
</nowiki></pre>
 
</nowiki></pre>
  
=== Class and variable names ===
+
== Class and variable names ==
  
Class names and enum names are written in [[Studly Caps|StudlyCaps]], with an initial capital letter and no underscores.  Methods are [[Studly Caps|StudlyCaps]], or [[lowerWithCaps]].  Variable names are all lowercase, or [[lowerWithCaps]]. Member variables in classes are usually prefixed with "m_".  Constants should be written with a "k" prefix and inner caps; the few global variables use a "g" prefix.  Enum values should be written in ALL_CAPS with underscores.  Hungarian notation is not generally used outside of Windows code.
+
Class names and enum names are written in [[Studly Caps|StudlyCaps]], with an initial capital letter and no underscores.  Methods are [[Studly Caps|StudlyCaps]].  Variable names are all lowercase, or [[lowerWithCaps]]. Non-static Member variables in classes should be prefixed with "m_"; other variables should <em>not</em> be preceded by "m_".  Constants should be written with a "k" prefix and inner caps.  The few global variables use a "g" prefix.
  
 
Make your variable names left-to-right specific.  Related variables should sort together alphabetically; <code><nowiki>audioId</nowiki></code> and <code><nowiki>audioMutex</nowiki></code> should stand apart from <code><nowiki>videoTimeRemaining</nowiki></code>.  The names should be compact, but be as self-explanatory as possible.
 
Make your variable names left-to-right specific.  Related variables should sort together alphabetically; <code><nowiki>audioId</nowiki></code> and <code><nowiki>audioMutex</nowiki></code> should stand apart from <code><nowiki>videoTimeRemaining</nowiki></code>.  The names should be compact, but be as self-explanatory as possible.
  
 
<pre><nowiki>
 
<pre><nowiki>
const int kAudioBuffers = 32;
+
enum AudioChannelEnum {
enum AudioChannels {
+
     kChannelLeft      = 0x01,
     CHANNEL_LEFT  = 0x01,
+
     kChannelRight    = 0x02,
     CHANNEL_RIGHT = 0x02,
+
     kChannelCenter    = 0x04,
     CHANNEL_ALL  = 0xff
+
    kChannelLeftBack  = 0x08,
};
+
    kChannelRightBack = 0x10,
 +
    kChannelBase      = 0x20,
 +
    kChannelAll      = 0xff
 +
} AudioChannels;
  
public class AudioOutput
+
class AudioOutput
 
{
 
{
public:
+
  public:
     // Adjust the volume by a delta, from +100 to -100.
+
     AudioOutput(const QString &device_name);
     void adjustVolume(int change);
+
 
protected:
+
    /// Adjust the volume by a delta, from +100 to -100.
     // The absolute volume, from 0-100.
+
     void AdjustVolume(int change);
     int m_volume;
+
    /// \return Device name
}
+
    QString GetDeviceName(void) const
 +
    {
 +
        QMutexLocker locker(&m_lock);
 +
        return m_deviceName;
 +
    }
 +
  protected:
 +
     mutable QMutex m_lock; // This is mutable so that
 +
    /// The absolute volume, from 0-100.
 +
     int m_volume; // protected by m_lock
 +
    QString m_deviceName; // protected by m_lock
 +
 
 +
    const static int kAudioBuffers;
 +
};
 
</nowiki></pre>
 
</nowiki></pre>
  
=== Comments ===
+
It's a good idea to follow [http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml Google's C++ standards] when they don't contradict our standards.
 +
 
 +
== Comments ==
  
 
Comments should be compact; you should spend your time making your code readable and your variable names self-explanatory.  Comments should give necessary overviews of functionality, or document strange cases, ranges, etc.
 
Comments should be compact; you should spend your time making your code readable and your variable names self-explanatory.  Comments should give necessary overviews of functionality, or document strange cases, ranges, etc.
Line 85: Line 134:
 
Description of variables' use and purpose should accompany the declaration.  For class variables, a rough outline of how they fit into the algorithm should be given and any assumptions or notes on valid ranges should be given.
 
Description of variables' use and purpose should accompany the declaration.  For class variables, a rough outline of how they fit into the algorithm should be given and any assumptions or notes on valid ranges should be given.
  
The majority of your comments should be in header files, where the public interface to functionality is exposed. If your design is reasonable and straightforward, you shouldn't have much that needs explaining in the .cpp file.
+
Doxygen comments are encouraged in the code. But keep them brief and don't use them when they don't add
 +
information not already obvious from the variable or function name.
  
Doxygen comments are accepted in the code, but you must keep them brief -- a few lines maximum.  Again, don't add redundant information.
+
= MythWeb Style =
  
=== Design guidelines ===
+
MythWeb is written in PHP and javascript so a different style makes sense.
  
There's more to programming than typing; here's some advice to keep in mind while designing or updating code:
+
== Comment Style ==
 +
Outdented, meaning one indent level less then the code it's talking about, and above the code block, not below.
  
* '''Use C++.''' There's a [[[[MythTV]] Development How To/C Plus Plus Tips|collection of tips]] to avoid outdated C constructs.  Myth supports gcc 2.95.x and above.
+
== White Space Style ==
* '''Use Qt for non-GUI tasks.''' Don't design your own helper classes or use platform-dependent code when Qt already offers a class.  Strings, lists, and mutexes are good examples of Qt's generic functionality. Myth supports Qt 3.1 and above, so check your calls against the [http://doc.trolltech.com/3.1/classes.html Qt 3.1 documentation] so you don't break things.
+
Soft Tab style (spaces, not tabs, and 4 spaces to a tabstop)
* '''Use Myth for GUI tasks.''' Isaac is currently working on a UI layer that's independent of Qt, and patches that use native Qt widgets or extend the Myth wrappers around the Qt GUI will be rejected.  Use the existing items in <code><nowiki>mythwidgets.h</nowiki></code> and elsewhere to accomplish your GUI goals.
+
 
* '''Stay platform-independent.''' Use Qt instead of #ifdefs where possible. If you must add something that only works on one platform, add a compile-time flag so that it's only built where it works.  See settings.pro for several examples.
+
== Classes ==
* '''Avoid dependencies.''' Dependencies are added very rarely to Myth, so think carefully before you suggest a new item.  If you want to link to a library that's small and has unique functionality, you may be able to get it added to the core Myth build.  In general, though, avoid external dependencies if at all possible.
+
  * Autoload
* '''Use private classes to keep header files stable.'''  This design pattern is used in [[Myth Context]] and others; most of the private data and methods are stored in a private class, defined in the .cpp file.  A pointer to this class is stored as a member variable in the public class.  This way, the private data can change without changing the header file, which prevents everyone from having to rebuild most of the source tree for a popular header like mythcontext.h.
+
* [http://en.wikipedia.org/wiki/Multiton Multiton pattern]
  
 
[[Category:Developer Documentation]]
 
[[Category:Developer Documentation]]

Revision as of 22:41, 5 January 2013

Design guidelines

Here's some advice to keep in mind while designing or updating code:

  • Use C++. Avoid outdated C constructs like TRUE/FALSE macros, C-style casting etc. One or two C-isms are preferred, like using NULL over the C++ preference of 0 (see this ticket). Myth supports gcc 2.95.x and above.
  • Encapsulate Well Make a Get method const. Return a copy and not a reference. Use Set methods, when defined in the header these entail no efficiency loss and allow you do add debugging code when necessary. Make local functions static and constants static const.
  • Use Qt for non-GUI tasks. Don't design your own helper classes or use platform-dependent code when Qt already offers a class. Strings, containers (lists etc) and mutexes are good examples of Qt's generic functionality. The current development version of MythTV requires Qt 4.8, so check your calls against the Qt 4.8 documentation so you don't break things.
    • We have our own replacement for QProcess called MythSystem. QProcess has had an unfixed deadlocking bug in Qt 4.0 through the time of the writing of this guide (4.7).
    • We have our own logging macro LOG, use this instead of the Qt debugging output macros and cerr.
  • Use MythUI for GUI tasks. We are currently working on a UI layer that's independent of Qt (MythUI), and patches that use native Qt widgets or the old libMyth code will be rejected. Use libmythui/* instead.
  • Stay platform-independent. Use Qt instead of #ifdefs where possible. If you must add something that only works on one platform, add a compile-time flag so that it's only built where it works. See settings.pro for several examples.
  • Avoid dependencies. Dependencies are added very rarely to Myth, so think carefully before you suggest a new item. If you want to link to a library that's small and has unique functionality, you may be able to get it added to the core Myth build. In general, though, avoid external dependencies if at all possible.
  • Use private classes to keep popular header files stable. This design pattern is used in MythContext and others; most of the private data and methods are stored in a private class, defined in the .cpp file. A pointer to this class is stored as a member variable in the public class. This way, the private data can change without changing the header file, which prevents everyone from having to rebuild most of the source tree for a popular header like mythcontext.h.
  • Don't use assert() or abort() Crashing is never good. Use proper error handling in place of assert. This doesn't mean you can't use asserts during development, just that by the time the code is ready for commit there should be no need for them.
  • Don't use C++ exceptions Unlike say Java, C++ doesn't have any way to tell the user of a method what exceptions must be caught, but gcc does let you mark a return value as something that must be examined. To deal with exceptions that can occur during object construction use either two part initialization or a static factory method that returns NULL when it fails.
  • Try to keep style coherent If a class prepends all it's member variables with _ rather than the "m_" don't add a variable with "m_", use the class style unless you are doing a major rewrite an renaming all the existing variables.
  • Make large style changes in a separate patch from functional changes This makes it easier for us to comb through the history looking for the commit that introduced the bug.
  • QString and other Qt container classes are not thread-safe. You can not use them in multiple threads without locking as you might a "const char*" string or a std::string. As of Qt4 assigned copies can be used in another thread, so something like this is now safe "QString MyClass::GetString(void) const { QMutexLocker(&m_lock); return myString; }" You may run across code like this from the Qt3 days, "QString MyClass::GetString(void) const { QMutexLocker(&m_lock); QString tmp = myString; tmp.detach(); return tmp; }" The detach() is no longer required but is a relic of our semi-automated conversion from Qt3 to Qt4.
  • Don't use Qt signals and slots in backend code There are a number of gotchas with signals and slots and we're decided to use them only in UI code as a result of dealing with all the hard to debug crashes that have resulted.
  • Locking order Most classes with more than one lock/mutex/semaphore will have locking order comment. You must follow this locking order to avoid A,B;B,A deadlocks. If you create a class with more than one lock, document your locking order.

For more of an overview of the development process for Myth, see MythPlugin Architecture.

MythTV Style

The rules of thumb on this page are meant to help developers ensure that their code fits in with the conventions used in the MythTV source. The goal is to enhance readability. After your code becomes a part of MythTV many other hands are likely to touch it and they will be following these conventions so if you apply some other convention the code will over time become more difficult to read. Do not apply these conventions to externally maintained code, such as ffmpeg, try your best to follow their conventions and whenever possible get your changes to the external code applied upstream as soon as possible.

Spaces and braces

The one hard and fast rule of Myth development: don't use tab characters.

Indents are accomplished with 4 spaces. Please set your editor to convert tabs to spaces, or at least do the conversion before you submit a patch.

You should put 1 space between keywords like if or for and their parenthesized lists. By contrast, don't do the same for function names; there should be no space between the name and its opening parenthesis. There should be no spacing between parentheses and the items they contain.

Spaces are used between binary operators (meaning + or =).

Braces should go on their own line. The exception to this is the declaration of structs or enums, where the opening brace goes on the same line as the type declaration. One-line if or else bodies do not need braces.

Lines should not exceed 80 characters in width. If you have a long statement, indent the continuations so they line up vertically. Generally, if you indent past the last opening parenthesis of the previous line, you'll be in good shape.

typedef enum my_enum {
    kTestOne,   ///< This represents blah
    kTestTwo,   ///< Widget Blah
    kTestThree, ///< Blah Blah
    kTestSentinel,
} MyEnum;

static inline void run_three_times(const QStringList &strings, const char *buffer)
{
    for (int i = 0; i < 3; i++)
    {
        MyEnum result = my_func(i, strings);
        if (kTestOne == result || kTestTwo == result)
            DoSomething();
        else
            DoTheOpposite();

        result = my_other_func(result, strings);
        if (kTestOne == result || kTestTwo == result)
        {
            if (DoSomething())
                DoMore();
        }
        else
        {
            DoTheOpposite();
        }
    }
}

Note: There are some things above that are not required by the coding standard. Using result == kTestOne is equally valid as kTestOne == result. The reason some developers prefer it is because it avoids introducing a "=" typo bug. If you run across the variation you don't prefer, leave it alone.

For Vi(m) users: the following options can be put into your ~/.vimrc file -

set expandtab
set tabstop=4
set shiftwidth=4

And if it's not too intrusive, this line can appear in the source code, and Vim will apply the correct settings:

/* vim: set expandtab tabstop=4 shiftwidth=4: */

Class and variable names

Class names and enum names are written in StudlyCaps, with an initial capital letter and no underscores. Methods are StudlyCaps. Variable names are all lowercase, or lowerWithCaps. Non-static Member variables in classes should be prefixed with "m_"; other variables should not be preceded by "m_". Constants should be written with a "k" prefix and inner caps. The few global variables use a "g" prefix.

Make your variable names left-to-right specific. Related variables should sort together alphabetically; audioId and audioMutex should stand apart from videoTimeRemaining. The names should be compact, but be as self-explanatory as possible.

enum AudioChannelEnum {
    kChannelLeft      = 0x01,
    kChannelRight     = 0x02,
    kChannelCenter    = 0x04,
    kChannelLeftBack  = 0x08,
    kChannelRightBack = 0x10,
    kChannelBase      = 0x20,
    kChannelAll       = 0xff
} AudioChannels;

class AudioOutput
{
  public:
    AudioOutput(const QString &device_name);

    /// Adjust the volume by a delta, from +100 to -100.
    void AdjustVolume(int change);
    /// \return Device name
    QString GetDeviceName(void) const
    {
        QMutexLocker locker(&m_lock);
        return m_deviceName;
    }
  protected:
    mutable QMutex m_lock; // This is mutable so that 
    /// The absolute volume, from 0-100.
    int m_volume; // protected by m_lock
    QString m_deviceName; // protected by m_lock

    const static int kAudioBuffers;
};

It's a good idea to follow Google's C++ standards when they don't contradict our standards.

Comments

Comments should be compact; you should spend your time making your code readable and your variable names self-explanatory. Comments should give necessary overviews of functionality, or document strange cases, ranges, etc.

Description of variables' use and purpose should accompany the declaration. For class variables, a rough outline of how they fit into the algorithm should be given and any assumptions or notes on valid ranges should be given.

Doxygen comments are encouraged in the code. But keep them brief and don't use them when they don't add information not already obvious from the variable or function name.

MythWeb Style

MythWeb is written in PHP and javascript so a different style makes sense.

Comment Style

Outdented, meaning one indent level less then the code it's talking about, and above the code block, not below.

White Space Style

Soft Tab style (spaces, not tabs, and 4 spaces to a tabstop)

Classes

* Autoload
* Multiton pattern