Proposed changes to coding style

Updated 3년 ago

    How to use this page

    Feel free to edit this page to add proposals or add new information to existing proposals. Any discussion should take place in the forums or Telegram chat. Links to the relevant discussions can be placed under each proposal.

    You can also add yourself as a supporter or opponent of each proposal, with an optional clarifying statement. You can also stay neutral.

    Support
    - @user1
    - @user2, providing that we...

    Neutral
    - @user3

    Oppose
    - @user4

    The poll exists to give an indication of the level of support: it does not guarantee that a particular proposal will be adopted/rejected.

    Code formats

    The formats currently in use:

    • C++ (*.cpp, *.h), with or without Qt extensions, and the few pure C (*.c) files
    • CMake (CMakeLists.txt, *.cmake)
    • JavaScript (*.js), including Qt QML (*.qml)
    • Python (*.py)
    • Shell (*.sh), including Bash and other Unix shells. Excludes Windows CMD and Powershell.
    • CMD (*.bat, *.cmd)
    • XML (*.xml), including MSCX (*.mscx), Qt UI (*.ui), SVG (*.svg)
    • JSON (*.json)
    • YAML (*.yml, *.yaml)

    You can use the following command to print a list of the different file extensions used in the repository, along with the number of files with that extension:

    git ls-files | sed -E -e 's|.*/||' -e 's|.*\.|.|' | sort | uniq -c | sort -k1,1r -k2,2df | less

    Sample output (annotated and abbreviated):

       1679 .h
   1607 .mscx
    851 .cpp
    550 .xml
     28 .in       # includes '.h.in', '.xml.in' and similar (CMake configured files)
     16 .mscz
     16 README    # file has no extension
     12 .bat
     12 LICENSE
      4 Makefile

    Use the following command to fold .*.in configuration files into the parent extension while keeping pure .in files separate:

    git ls-files | sed -E -e 's|.*/||' -e 's|.*(\..+)\.in$|\1|' -e 's|.*\.|.|' | sort | uniq -c | sort -k1,1r -k2,2df | less

    Now file names ending .h.in will be included in the count for .h, etc., while the count for .in will only include files that have no other extension.

    Multi-format proposals

    The following proposals affect more than one type of file.

    CI tests to enforce coding rules

    Where possible, code rules should be enforced by passing code through a formatting tool as part MuseScore's continuous integration tests. The test should fail if the code is not formatted correctly.

    If a style is strictly enforced then developers can pass code through Git's smudge and clean filters to cause it to be formatted in their preferred style during checkout, and back to the standard style during checkin, as demonstrated here. It doesn't matter which style is used in the repository as long as it is enforced properly.

    Primarily affects C++ files, but could potentially include other formats too.

    Update: MuseScore's coding style is now supported by Uncrustify. Autoformatting to other styles is demonstrated here.

    PROS
    - Code would be formatted consistently
    - Easier to read and maintain, etc.
    - Fewer nitpicking comments during pull request reviews

    CONS
    - Loss of flexibility (formatting tools are extremely pedantic)

    Support
    - @shoogle
    - @igor.korsukov
    - @Jojo-Schmitz, might be difficult to impossible for third-party code though. Also this would need to allow for occasional exceptions.
    - @Spire42

    Neutral
    - @dmitrio95, I am not sure that any formal coding style would be the best choice in all situations.
    - @marcsabatella, if it's proven flexible enough to be viable I'd support

    Oppose
    - @mirabilos
    - @Howard-C, it can sometimes expect very ugly results

    No substantial changes to coding style

    MuseScore currently uses 6 space banner-style indentation. While this may not be what most programmers are used to, the cost of changing it now far outweights the benefits.

    Developers who don't like this style can pass code through Git's smudge and clean filters to cause it to be formatted in their preferred style during checkout, and back to MuseScore style during checkin, as demonstrated here.

    void myFunc(bool b)
      {
      if (b) {
            // do something
            // ...
            }
      else {
            // do a different thing
            // ...
            }
      }

    Note that support for this proposal does not imply opposition to smaller changes to coding style, or to better enforcement of the current style.

    Update: MuseScore's coding style is now supported by Uncrustify. Autoformatting to other styles is demonstrated here.

    PROS
    - Nothing to do!
    - No need to rebase hundreds of pull requests
    - Preserve Git history and blame
    - Works with autoformatting tools like Uncrustify

    CONS
    - Not what programmers are used to
    - IDEs not setup to use this style by default

    Support
    - @Jojo-Schmitz, can I have more than one vote please? ;-). There are other code styles that may look better, but none is worth the effort and pain that comes with changing to it. I don't particularly love the current style, but it has its benefits and I got used to it eventually )and it didn't take too long). Everyone else should be able to get used to it too...
    - @mattmcclinch, I actually like this coding style, believe it or not. It was not what I was used to, but I became accustomed to it very quickly, and changing it now would actually decrease my productivity.
    - @mirabilos (first choice), I hate the current style but keep it anyway, changing is error-prone and makes merging back and forth very problematic
    - @marcsabatella, this is my vote. Independently of what my own personal preference would be, I see far more downside than upside to any change. FWIW, I was not familiar with this style at first and while I don't love the six space indents, I have come to actually prefer the brace style over other alternatives. Still, my personal preferences aren't the point. Soapbox: there are many different aspects of coding style that we as developers need to adapt to - OO versus procedural paradigms, interpreted vs compiled, syntax differences between compiled OO languges or even versions of a single language, actual use of multiple inheritance, accessor methods & pointers & references & and other language features, different UI frameworks, differences between Qt lists vs C++ vectors, iterators vs traditional loop structures, etc. Adapting to different paradigms, languages, versions, usages, frameworks, and so forth is part of being a successful developer, particularly in a team environment. And really, the skills needed to keep track of these sorts of details (and here I included indent and brace style) are not so different from the skills needed to keep track of what the code is actually doing and why, the sorts of skills needed to make sure you understand how your changes will affect other code, etc I have faith that any developer who has the skills needed to actually understand, maintain, and improve the code itself can adjust to different indent and brace styles as surely as they can handle the other aspects of coding style that are different from project to project.
    - @Howard-C

    Neutral
    - @shoogle, while I prefer K&R, consistency matters more, and other (less disruptive) proposals are more important to me than indentation style.

    Oppose
    - @dmitrio95, as the current style has certain practical disadvantages which make the code less convenient to work with. Most prominently, 6-space indents make code lines be too long to fit to the "comfortable" size limit of 80-100 characters. Also certain practical consequences seem to be shown in the relevant studies: in McConnel's "Code Complete" book refers to studies of a dependency of code readability on indentation size. According to the conclusions presented in the book, there is no real difference between 2- and 4-space indents in this regard, but 6-space indents showed worse performance for the ability of programmers to understand the code. I could try to find the references from the book and put it here.
    - @ypoissant2, I know I'm very late in this poll and decisions have already been taken, but I wanted to add my comment. I'm learning MuseScore internals by following the code, mainly in rendermidi and related classes, And one thing I observe, is the prominent use of the "continue" and "break" statements. Not using "continue" would force indentations to go too far off to the right in many situations because of the 6 space indents. However, the usage of continue and break obfuscates the function logic and make it hard to understand, at least for me. My conclusion is 6 spaces is a too wide indent and prevents proper logical formatting.

    4 space indentation with K&R braces

    We should switch to a more popular coding style, with one of the most popular styles being 4 space indentation with K&R-style braces.

    void myFunc(bool b)
{
    if (b) {
        // do something
        // ...
    } else {
        // do a different thing
        // ...
    }
}

    Primarily affects C++ files, but could potentially include other formats too.

    PROS
    - Used by many projects, including Qt
    - More familiar to most developers, especially those new to MuseScore
    - Supported by IDEs (often the default!)

    CONS
    - Big change increases repository size
    - Spoils git blame (certainly on GitHub, possible workaround for offline git)

    Support
    - @shoogle
    - @igorkorsukov
    - @dmitrio95, but the caution should be taken to check how changing braces style works with --ignore-rev option of Git. Unlike whitespaces changes, changing braces style will change a number of lines, so it might be needed to do braces style change in a separate commit for --ignore-rev/--ingore-revs-file solution to work correctly.
    - @mirabilos, second choice

    Neutral
    - @Spire42, This is better than banner style, but I strongly prefer Allman style instead.

    Oppose
    - @Jojo-Schmitz, 4 spaces are fine by me, brace style is not. Although my opposing isn't really strong, I could certainly adjust and get used to it. If (and only if!) it can get done without screwing up git blame etc. And without needing to rebase some 250 open PRs!
    - @marcsabatella, I oppose change for the sake of change. Until someone proves there is a real benefit to offset the VERY real and enormous cost - or can demonstrate a proven method to negate the cost - any such proposal is DOA to me. And in any case, if that were to happen, this would not be my choice of what to change to.
    - @Howard-C, don't like putting the right brace and else on the same line.

    4 space indentation with Allman braces

    Another popular style is 4 space indentation with Allman-style braces.

    void myFunc(bool b)
{
    if (b)
    {
        // do something
        // ...
    }
    else
    {
        // do a different thing
        // ...
    }
}

    Primarily affects C++ files, but could potentially include other formats too.

    PROS
    - Used by many projects
    - More familiar to most developers, especially those new to MuseScore
    - Supported by IDEs

    CONS
    - Big change increases repository size
    - Spoils git blame (certainly on GitHub, possible workaround for offline git)

    Support
    - @Spire42

    Neutral
    - @shoogle, better than banner style, but I prefer K&R style above.
    - @igor.korsukov better than banner style, but I prefer K&R style above, because in K&R style there is a begin, a body, an end, and in Allman style there are as if two begins
    - @Howard-C

    Oppose
    - @Jojo-Schmitz, 4 spaces are fine by me, brace style is better than K&R and the one I used in a past life. Here too my opposing isn't really strong, I could certainly adjust and get used to it. If (and only if!) it can get done without screwing up git blame etc. And without needing to rebase some 250 open PRs!
    - @marcsabatella, I oppose change for the sake of change. Until someone proves there is a real benefit to offset the VERY real and enormous cost - or can demonstrate a proven method to negate the cost - any such proposal is DOA to me. But, that said, if it were somehow proven that there was a benefit to changing that was greater than the enormous cost, this is what I'd vote for as what to change to.
    - @mirabilos, strongly oppose, ugly brace style
    - @dmitrio95, for the brace style unnecessarily taking a lot of vertical space.

    6 space indentation with K&R braces

    This provides all the benefits of the familiar K&R-style braces with none of the drawbacks of changing indentation level. Implementing this proposal would result in a tiny change to braces at the end of functions and statements, but there would be no changes to any lines of actual code so tools like git blame would continue to work as expected.

    void myFunc(bool b)
{
      if (b) {
            // do something
            // ...
      } else {
            // do a different thing
            // ...
      }
}

    Primarily affects C++ files, but could potentially include other formats too.

    PROS
    - Similar to what is used by other projects, including Qt
    - More familiar to most developers, especially those new to MuseScore
    - Supported by IDEs (close to the default!)
    - Works with auto-formatting tools like Uncrustify
    - Enables developers to use their preferred style via Git's smudge and clean filters
    - Doesn't affect git blame
    - Some people prefer 6 space indentation (more indentation makes levels easier to see, but 8 is too many)

    CONS
    - 4 spaces is more popular (but MuseScore already uses 6)
    - Some work rebasing PRs that make changes near ends of functions / statements

    Support
    - @shoogle

    Neutral
    - @marcsabatella, I'm still not seeing the point of putting so much effort into the idea of change, but I will say, if we have to change to something and wish to minimize the damage, this is clever. But I do have to laughingly agree with the assessment I saw on Telegram of this being the "worst of both worlds", at least when considered on its own merits. I dislike K&R braces, and I dislike 6 space tabs, but somehow the combination comes closest to what we have now among any of the proposals that anyone might see as an improvement.
    - @Jojo-Schmitz, but I am still not convinced about any need for a coding style change

    Oppose
    - @Howard-C, don't like putting the right brace and else on the same line.
    - @dmitrio95, as I mentioned above, 6-space indents are the more important issue than braces style.

    6 space indentation with Allman braces

    This provides all the benefits of the familiar Allman-style braces with almost none of the drawbacks of changing indentation level. Unlike K&R-style above, Allman style would require a small change to braces at the beginning of statements as well as at the end, but no other lines of code would change.

    void myFunc(bool b)
{
      if (b)
      {
            // do something
            // ...
      }
      else
      {
            // do a different thing
            // ...
      }
}

    Primarily affects C++ files, but could potentially include other formats too.

    PROS
    - Similar to what is used by many projects
    - More familiar to most developers, especially those new to MuseScore
    - Supported by IDEs
    - Works with auto-formatting tools like Uncrustify
    - Enables developers to use their preferred style via Git's smudge and clean filters
    - Some people prefer 6 space indentation (more indentation makes levels easier to see, but 8 is too many)

    CONS
    - 4 spaces is more popular (but MuseScore already uses 6)
    - Some work rebasing PRs that make changes near ends of functions / statements (a bit more work than for K&R)
    - Affects git blame (but not very much)

    Support

    Neutral
    - @shoogle, I prefer K&R
    - @marcsabatella, although I personally prefer Allman over K&R when writing code in the privacy of my own bedroom, the "K&R+6" proposal would destroy the git history by only half as much I guess, so there is that. Still, I could be OK with this more than most of the others. If it were up to a vote, I'd still pick "no change" in a heartbeat, but if it becomes clear that change is coming anyhow, and no one comes up with a really brilliant solution to allow 4 space tabs (which I do personally prefer, but not at the cost of adversely affecting git history), I will side with whichever 6-space proposal seems more likely to defeat any coalition of 4-space supporters. Just putting that out there :-)
    - @Howard-C

    Oppose
    - @dmitrio95, as I mentioned above, 6-space indents are the more important issue than braces style.
    - @Jojo-Schmitz, that 'combining the worse of both worlds" was me, regardless that this style (except for using Tabs instead of spaces) was the style I used prior to MuseScore. It does affect all block starts in `git blame, which is bad and doesn't help with the number of spaces.

    Qt Coding Style

    Why stop with indentation and braces? Let's adopt all of the style recommendations made by the Qt Project.

    PROS
    - Tried and tested rules
    - Consistency with Qt

    CONS
    - Big change to current style
    - Spoils history, git blame, etc.
    - Arguably bool* ptr; is better than bool *ptr; because ptr is not a bool, it is a pointer to a bool.

    Support
    - @shoogle
    - @mirabilos, third choice (though there’s not much difference to K&R AFAICT?)
    - @igor.korsukov Absolutely yes. The point is not in the Qt, but in having a source of truth and a reference. Instead of Qt, for example, we can to accept Google C++ Style Guide, which also explains why so, and not otherwise.

    Neutral
    - @dmitrio95, although I don't see a reason to adopt references and pointers declaration style from Qt.

    Oppose
    - @marcsabatella, I oppose change for the sake of change. Until someone proves there is a real benefit to offset the VERY real and enormous cost - or can demonstrate a proven method to negate the cost - any such proposal is DOA to me. If we have to change, I strongly encourage keeping the changes as minimal as possible in order to still receive the alleged benefit. Change to the pointer declaration in particular seems even more gratuitous than changes to indent or brace style.
    - @Howard-C, I don't think it's necessary to adapt to a specific coding style just because we use a gui framework that uses it. For example, I don't like the idea of changing pointer declaration to T *x at all.
    - @Jojo-Schmitz, esp. that pointer type thing

    One list item per line, alphabetical

    When defining a list or similar structure that contains (or may eventually contain) more than 2 items, write each item on a separate line. Items should be ordered alphabetically unless a different ordering makes more sense for the given situation (e.g. chronological, size-order, grouped by meaning, etc).

    Applies to all file formats. Example in CMake:

    # Do this
add_files(
    file1.h
    file2.h
    ...
    )
 
# Not this
add_files(file2.h file1.h ...)

    For a real-life example of where this rule would make a difference, see this list in icon.h which is supposed to correspond 1 to 1 with the list in icon.cpp. See also the mess that is mscore/CMakeLists.txt.

    PROS
    - Less sideways scrolling
    - Items aligned vertically
    - Diffs are smaller and easier to read
    - Merge conflicts are easier to resolve

    CONS
    - More vertical scrolling
    - Larger file size (though smaller changes)

    Support
    - @shoogle

    Neutral
    - @marcsabatella, doesn't affect as much code as the other changes so I'm less worried about the cost. I might still think short lists are fine to put on line, though, at least sometimes. So I'd prefer we be flexible on this.
    - @mirabilos
    - @Howard-C

    Oppose
    - @igor.korsukov (I am for adding to the column, but not alphabetically, but grouping by meaning (and for сmake need to do a restructuring in order to group files by folder structure and use file (GLOB ...) )
    - @Jojo-Schmitz

    C++ proposals

    The following proposals only affect C++ files (*.cpp and *.h) and the few pure C (*.c) files in the repository.

    Remove function name comments

    These don't provide any information that is not available in the function definition.

    //--------------------------------------
//   MyClass::myFunc
//--------------------------------------

    Note that documentation-style comments are useful and should be kept, but there is no need to include the function name in such comments.

    PROS
    - Save writing the same thing twice
    - No opportunity for comment to get out-of-sync with code
    - Nobody else does it

    CONS
    - Slightly harder to see where functions begin and end

    Support
    - @shoogle
    - @igor.korsukov
    - @mirabilos

    Neutral
    - @Jojo-Schmitz
    - @dmitrio95, these comments have a lot of information which would be useful to be collected by Doxygen into an auto-generated documentation to the code. The current structure of these comments makes it easy to convert them to Doxygen-style docstrings (by starting the comment lines with /// instead of //), but moving such documentation comments to header files should be fine as well. Apart from documentation purposes, I see no reason to preserve these comments.
    - @marcsabatella, I am not crazy about the way these comments are formatted, but I do like the visual separation when browsing the code; still, if we're to spend time making changes, I'd rather see something really constructive, like actually adding meaningful comments to each function

    Oppose
    - @Howard-C, the border lines help me a lot when browsing through code. As for the duplicated names themselves, I think I don't have much feelings about them, so it's OK to remove them. But OK doesn't necessarily mean we should remove. Even if I directly look at the function names the majority of time, I sometimes still look at the comments to search for function names. They're useful and helpful, that's all for me. Don't know why we have to change this.

    Documentation comments

    Where appropriate, functions should be preceded by a comment that explains what the function does and how to use it (i.e. a 'docstring').

    /// Draws the element so that it fills as much of the given QRect
/// as possible without changing the elements's aspect ratio.
 
void Element::paint(QPainter& p, QRect& r)
      {
      // code
      }

    Notice the use of three slashes (///) to expose the docstring to Doxygen.

    There is no need for the docstring comment to include the name of the function or a special border (think //--------, etc.). If such things are included then they should be on a line with two slashes (//) to avoid them being exposed to Doxygen.

    PROS
    - Makes functions easier to understand and use
    - Makes it easy to see where one function ends and the next begins, which could be important if the function name comments are removed as proposed above.

    CONS
    - More work in writing and maintaining comments.

    Support
    - @shoogle
    - @dmitrio95, another options which could be considered are using another style of comments (/** comment */) and moving documentation comments to header files. Any of the options (including three-slashes comments listed here) would be fine for me.

    Neutral

    Oppose
    - @Howard-C, if we add explanatory comments to all classes/functions then it's all right. But if not, this proposal suggests those who don't have explanatory comment won't have border lines and name comment as well, which I oppose. Border lines are very helpful to me as I've said above, I don't see why they're considered useless.

    CMake proposals

    The following proposals only affect CMake build files, i.e. CMakeLists.txt and *.cmake.

    Empty closing statements in CMake files

    Do this:

    if (CONDITION)
    # do something
endif()

    rather than:

    if (CONDITION)
    # do something
endif(CONDITION)

    Both are valid syntax that will not result in errors or warnings when you run CMake. Warnings occur if you try to specify a closing condition that doesn't match the opening condition, but an empty closing condition is fine and is recommended by many style guides, including KDE's guide.

    PROS
    - Less work to read, write & maintain
    - Like every other language!

    Support
    - @shoogle

    Neutral
    - @Howard-C
    - @igor.korsukov
    - @Jojo-Schmitz
    - @mirabilos, keeping them sometimes helps in better overview, but we don’t do it in C++ either, so no biggie
    - @marcsabatella, cmake is still a foreign language to me, as is virtually everything about the build process, so I defer to others

    Oppose
    - @dmitrio95, with large blocks of code this syntax provides a natural way to show which exactly conditional block ends on this line. It might be a better choice not to write such block in the first place, but I would suggest not adding any rule on that to coding style recommendations and allowing to choose the endif() statement format depending on a particular context.