Let's talk about the inconsistencies in the code-style

[YeldhamDev]

Ok, I will try to tackle the issue without sounding like a nitpicky asshat (no promises):

The code-style in Godot's code base is kinda all over the place. Yes, we do have clang-format that makes sure some rules are obeyed, like starting brackets be in the same line as the if statement for example, but that are a lot of other things that don't have a proper rule on how they should be formatted, which results in very different code-styles in different files (sometimes even in the same file!).

Here a list of the most occurred inconsistencies in the code:

If's:

• Some begin with a new line, others don't.
• Some use brackets for single line statements, others don't.
• Suggestion: Drop the new lines but always use brackets, they improve the visibility and makes it easier in case you have to add something more.

Functions:

• While majority of time they start with a new line, there are some cases that they don't.
• Suggestion: As they are the majority, always use new lines.

Switches:

• Majority use brackets, but some don't (I don't know why, though).
• Hell, there are some cases that long lists of if/else's are used instead!
• Suggestion: While they are the majority, I don't see why brackets are used in these, so I don't know.

Comments (Yeah, yeah, getting into really deep nitpick territory, but stick with me):

• Majority of times they're written completely in lowercase and without end punctuation, but there are cases that differ from that.
• Suggestion: I personally think it looks much more professional and cleaner if they are capitalized and with the end punctuation.

And that's mostly it, but there are probably some others thing that I either forgot or didn't notice.
I'm not saying that anyone should go hunting for these, but trying to maintain some consistency while writing/editing new code could be a good idea.

[LinuxUserGD]

This could make the code more human readable but I think that it is more important to optimize the code in matters of performance (e. g. link-time optimization) rather than reaching a more aethetical look.

[YeldhamDev]

@LinuxUserGD I don't see how the suggestions above would harm performance, they are purely aesthetic as far as I know.

Besides, I don't think anyone should go straight optimization > readability, but instead, a fine balance between the two.

[groud]

Honestly this is not going to happen. Let just people use the code style they want provided that they respect the given clang formatting.

We're a community driven project that involves a lot of different people with different tastes regarding code style. Let's not force people to adopt one as it does not really impact the tm project maintainability.

[YeldhamDev]

@groud

We're a community driven project that involves a lot of different people with different tastes regarding code style. Let's not force people to adopt one as it does not really impact the tm project maintainability.

Following that logic, we should just drop clang-format and let people write however they like as long as it's legible and compiles.

[groud]

Well, following yours, we should start including a code quality static checks. But we're not making a plane onboard controller, we're making a game engine.

IMO, the workflow is good as is. We ensure that merging is not getting messy with too many aesthetic changes while still letting people make their own decision on the code they are working on... It's a question of balance between code quality and being welcoming to developpers that have different tastes regarding code style.

Also, to illustrate my point, I generally don't agree with your proposals. They are some places where brackets are just too verbose, one-line functions are approriate (in header files sometimes). Also, regarding the comments, I actually don't care. So I would not force people to adopt a rule on that. It's already hard to make good code I'm not going to bother people so that they put a capital and a point in their comments.

[mhilbrunner]

But we're not making a plane onboard controller, we're making a game engine.

Both are quite complex and need to be readable, maintainable, because they need to function long-term.
Sure, lives hopefully don't depend on Godot in the same way, but peoples livelihood may if you expect users to use Godot for actually earning their income.

It's a question of balance between code quality and being welcoming to developers

Agreed. But:

that have different tastes regarding code style

As a potential new contributor, "we have no style" sounds more offputting to me than "we enforce a style that you don't like". I can live with doing formatting according to your whims, if it is consistent, and actually would prefer that over a wild mix of styles all over the place.

And I disagree strongly on the braces. I already saw bugs that wouldn't have been possible with braces in conditionals, and I already debugged one I caused myself because I modified existing code and missed the missing braces. Comments and stuff are nitpicking, yes, but for braces, always having them trades a little more verbosity in some cases against eliminating a whole class of bugs. Far worth it, IMO.

[vnen]

I think we can enforce a more strict style, we already do to some extent (more than the clang-format, PR-reviewers usually point other things too).

Regarding single line ifs I prefer no braces if it can be on the same line, otherwise always use braces even if it's a single line. E.g.:

// Simple enough to be a single line.
if (!is_inside_tree()) return;

// Not on the same line, use braces.
if (something && something_else || something_other) {
    do_something_more();
}

[groud]

Why not if we can automate the formating / testing for that.
Personnaly, I won't ask people to amend a PR for such code style issues, unless it really hurts readability (which is basically what we're already doing now).

[mhilbrunner]

Ideally, you don't have to ask them because the CI already fails if code isn't formatted correctly.

[LikeLakers2]

I'd like to throw one under the comments header. The engine code needs quite a bit more commenting in the source code, so new people (we're aiming to allow anybody to contribute, right?) can look at a section of code and go "That's what that's doing". Right now though, I've noticed (and maybe I'm not looking enough, but) a big lack of comments within the code describing what everything's doing, which forces people reading the code to have to look at it and make a guess as to what's happening.

Now I won't say from now on everything has to be commented, but I'd like there (at least) to come times where we go through and see what needs some in-depth comments. And no, I don't believe self-documenting function names are enough in this case.

Just to clarify, this is in regards to the engine's source code, not the documentation for GDscript over at godot-docs. I'd like to see at least simple one-liners describing certain sections of code.

[Warlaan]

Switches:
Majority use brackets, but some don't (I don't know why, though).
I am not 100% sure I understand you correctly, but if I do this should clarify:

A switch is similar to making use of gotos. If you are writing code like

switch(x)
{
   case 1:
      // do something;
      break;
   case 2:
      // do something else
      break;
}

you are basically telling the compiler to jump to the line "case 2" if x is 2. This works fine with imperative code, but it is a problem with declarative code like this:

switch(x)
{
   case 1:
      int thisIsWrong;  // and not allowed
      break;
   case 2:
      thisIsWrong = 666;  // because this might happen
      break;
}

That's why some people (including me) like to write switches like this:

switch(x)
{
   case 1:
   {
      int thisIsFine;
      break;
   }
   case 2:
   {
      thisIsFine = 666;   // This is obviously a compiler error since the scope of "thisIsFine" has clearly ended.
      break;
   }
}

because now you are allowed to declare temporary variables inside a case since every case has its own scope.

In theory it would be fine to only use braces when you do use temporary variables, but most people think of braces as a formatting element rather than a scope definition. I mean it's very rare that braces are added just to limit a scope, usually they directly follow a "header line" like a function signature, an if, a while etc.

[neikeq]

If's:

• Some begin with a new line, others don't.
• Some use brackets for single line statements, others don't.
• Suggestion: Drop the new lines but always use brackets, they improve the visibility and makes it easier in case you have to add something more.

I agree we should drop new lines for blocks inside functions. However, I think we should allow single line statements to be bracket-less. If one of the blocks in an if-else requires braces, then the others should add them too.

Functions:

• While majority of time they start with a new line, there are some cases that they don't.
• Suggestion: As they are the majority, always use new lines.

IMO we should follow K&R's style of placing the opening brace of functions in a new line. If we are going to add a new line any way, better do it in a way that improves readability.

Example:

void Foo::bar()
{
    if (cond) {
        a();
    } else {
        X res = x();
        b(res);
    }

    if (another_cond)
        c();
}

[aaronfranke]

Ifs: Agree, braces are nice.

Functions: I have to agree with @neikeq, if we're going to take up that vertical space anyway, we should use it! I haven't raised an issue about this before because I figured it'd be a lot to change.

Comments: Capitalization is good. Additionally, they should always start with spaces. // Test instead of //Test. This helps differentiate commented out code from actual English comments. I don't personally care about having punctuation or not.

Comments would probably be easiest to fix without upsetting any contributors who prefer their code looks a certain way. I think everyone would prefer either // Test or // Test. over //test

[Calinou]

There's also a small inconsistency when it comes to property hints. Some of them use spaces after commas, but most don't:

godot/core/io/file_access_network.cpp

Line 515 in 5441aaf

ProjectSettings::get_singleton()->set_custom_property_info("network/remote_fs/page_size", PropertyInfo(Variant::INT, "network/remote_fs/page_size", PROPERTY_HINT_RANGE, "1,65536,1,or_greater")); //is used as denominator and can't be zero

godot/main/main.cpp

Line 783 in 5441aaf

ProjectSettings::get_singleton()->set_custom_property_info("network/limits/debugger_stdout/max_chars_per_second", PropertyInfo(Variant::INT, "network/limits/debugger_stdout/max_chars_per_second", PROPERTY_HINT_RANGE, "0, 4096, 1, or_greater"));

We should probably pick one style or the other. I'm not sure which style would be best; they're more readable when separated with spaces, but this makes them look like an actual string that would be displayed to the user. On the other hand, not using spaces makes it clearer that the property hint string is only used for internal purposes.

This inconsistency is also present in FileDialog filters (notice the space before the semicolon):

godot/editor/editor_feature_profile.cpp

Line 897 in 5441aaf

import_profiles->add_filter("*.profile; Godot Feature Profile");

godot/editor/project_manager.cpp

Line 376 in 5441aaf

fdialog->add_filter("*.zip ; Zip File");

[bojidar-bg]

About property hints: I would suggest spamming warnings when the "wrong" style is used, so that contributors have a reason to switch to the "right" style.
Personally, I prefer spaces after commas and semicolons, but no spaces before them.

[akien-mga]

There's also a small inconsistency when it comes to property hints. Some of them use spaces after commas, but most don't:

I tend to prefer the compact form without spaces, which makes it clear that the string is a property hint, and not several extra arguments to the ADD_PROPERTY() or set_custom_property_info() method call. But it's true that it's an inconsistency with the style enforced within C++ code, so if we want to enforce a space after comma in property hints, that's fine with me too.

[Calinou]

In addition to C++ style, we should also adopt some rules for texts that appear in the editor. Currently:

• Some tooltips and error messages end with a period, but not all.
• Most buttons, menu actions and undo/redo actions are written "Like This", but a few use casing "Like this".
• Some texts use contracted forms ("won't"), some don't ("will not"). I think using the contracted form is better, as it's shorter.
• Some texts use simple quotes, others use double quotes. I think double quotes make more sense, as they're the de facto character to quote things in English.

In addition to the current doc writing guidelines, we could also take inspiration from the Fluent writing style guidelines.

[dreamsComeTrue]

While majority of time they start with a new line, there are some cases that they don't.

I am wraping my head around and trying to understand - can someone explain what are the origins of offsetting function's/method's body by one line? I can't find similar code style in any other open source software and I am mostly confused while skimming the code (but I am gradually accustomed to it).

[neikeq]

@dreamsComeTrue I think it's for readability. If that's the case, it doesn't make much sense; better place the brace in a new line, right?

[akien-mga]

About comment style, see also #21696. This might be worth moving to a dedicated proposal that we can agreed upon and enforce for 4.0+.

Some of the items listed here are now tracked in #33027. Others might still need to be ported there, and we can then close this issue.

[akien-mga]

Closing as superseded by #33027. If some items mentioned here are not reflected in the other issue, please add a comment there.