Naming and Layout/Style Proposal

[meleu]

From @SyrianBallaS on April 3, 2018 19:25

Preface

OK, so I noticed the .editorconfig file got accepted. That will definitely help a little bit, but ultimately not solve the root issue of inconsistent style. It is understandable that inconsistencies are undesirable and wish to propose a solution. Some of these come from the reviews in pull request #49, others from myself, and, isocpp. For some insight, I professionaly program in ASP.NET Core so some stylings may differ from isocpp.

On another note (somewhat related), I propose using a package.config/project.json file for binaries to prevent changes to library files (plus it will save space in the repo). Had one a long time ago but deleted the repo that was in it because it was too far gone (incorrect).

The lead engineer (I'm assuming @ScottFromDerby) will have the final say.

This is just a proposal, what's actually going to be used should be formalized in the wiki or a CONTRIBUTING file. This is just to give some ideas.

Naming

For a lot of these, you can easily change them using ctrl+alt+f.

Data Members

I would prefer data members be in snake-case with a leading underscore (i.e., some_var_) as encoding type information is redundant. However, if the majority strongly desire Hungarian Notation that's fine too.

Methods

• For this one I'm not too sure on what we should do but I'll propose something any way.

Event Handlers

• Given my background, I propose we use Pascal case for these.
• An event handler for us is like the Message Handlers in my fork.

Other Member functions, internal functions(helpers), and global functions

• These should be in snake-case

Classes/Structs

• These should be in snake-case, we could make the first letter capital.
• In my fork I wrapped all the new stuff in a namespace (ra) but put a macro _RA to make it more consistent. It's defined like this: #define _RA ::ra::

Enums

• Should be changed to "enum class", this prevents conflicts and disables implicit conversion to int.
• snake-case
• I made a helper function etoi (made an alias to_integral) to convert any enum to its underlying_type.
• I did put two concepts (type_traits) is_equality_comparable and is_lessthan_comparable to validate something is comparable it looks for the equality and lessthan operator. It will be important for some enums. If those validations pass in static_assert you can use using namespace std::rel_ops (suggest at function scope) to have the rest of the operators done for us. Some other operator overloads might be necessary as well but it's trivial.

Layout

More Formatting (not covered by .editorconfig)

To check/uncheck, go to Tools->Options->Text Editor->C/C++->Formatting->(*something). *something will depend on what we're trying to do. Some of these might be checked/unchecked already.

To save time I'll just put a screen shot

I'd suggest for everything besides multiline functions and long loops (the initialization part) put a brace on the same line with one space before it.

Limit Macros

Unacceptable Uses

• Symbolic Constants (ironic)
• Sometimes a macro is the only solution; yet, if used for a global constant there is another way.
• That other way is use the keyword constexpr(wasn't fully implemented (still isn't, no constexpr if) by Microsoft until C++17 but did exist in C++14).
• Example: instead #define SOME_STRING "a string", consider inline constexpr const char* some_string{"a string"}
• constexpr if`is short for "constant expression" and can only be used on types that can be resolved at compile time.
• Example: You can use constexpr on std::array but not std::vector, std::string, std::map, etc.
• For the strings we should const char* since arrays aren't null terminated. In the Microsoft's version of the GSL they have something called zcstring which is a wrapper for const char* which is bounded.
• Aliasing
• For functions your can just use std::function with std::bind. (creates a function object instead of a pointer, which automatically deallocates).
• Another way is to use a function pointer alias.
• Example:
  int some_fn(char c, std::string str);
using some_fn_t = decltype(&some_fn);
const auto& another_fn{static_cast(some_fn_t)(some_fn)};
  It didn't like the angle brackets.
• Now another_fn is an alias to some_fn; Did some extra steps because that's for overloads. Overloaded template functions needs parameter forwarding but we'll worry about that later.
• For member functions, the same as the above but you have to provide an object. If it's in the same class using this, should be good enough.
• A better way to alias a type without declaring it is to use using. Instead of typedef int some_type ; consider using some_type = int;.

Acceptable uses

• Required by a dependency, such as HANDLE_MSG.
• Pretty much anything that is not used in execution (seldom few exceptions like above).

Copied from original issue: RetroAchievements/RASuite#73

[meleu]

From @Jamiras on April 7, 2018 3:0

I was disappointed by the fact that .editorconfig basically only handles tabs to spaces and indent levels, but it's a start.

I agree with most of your suggestions, including all of the editor settings (which I believe are the default) and macro usages.

I prefer camelCase over snake_case for variables and class fields, and PascalCase for class, enum, and method names (whether global or not).

I don't really have a preference on whether or not class fields should be prefixed (i.e. field vs. _field), but I'm not fond of using a suffix (i.e. field_).

The Hungarian variable names and much of the current formatting style are from before my time. If we wanted to change things to enforce a standardized style across the codebase, I don't think there would be much push back.

The codebase has been fairly stable for the last month or so, it seems like it would be a good time to attempt something like this. I'd recommend limiting the first pass to whitespace, and tackling names later.

[meleu]

From @SyrianBallaS on April 9, 2018 19:4

I'm honestly fine with anything as long as everyone's on the same page. Was just giving some insight based on my experience.

[Jamiras]

I've been looking at the code, and have the following proposal:

1. Class names should be PascalCase
2. Struct names should be PascalCase
3. Enum names should be PascalCase. Enum items should also be PascalCase
4. Method names (global or class methods) should be PascalCase
5. Constant names should be ALL_CAPS
6. Variable names (local or class members) should be camelCase
7. Global variables names should be camelCase with a g_ prefix: g_camelCase
8. Namespace names should be snake_case. The top level namespace will be ra.
9. Pointer and reference markers should be with the type: char* ptr, not char *ptr.
10. Braces should be on a separate line:

if (a == b)
{
    for (int i = 0; i < 10; i++)
    {
        do_something(i);
    }
}
else
{
    do_something_else();
}

not

if (a == b) {
    for (int i = 0; i < 10; i++) {
        do_something(i);
    }
} else {
    do_something_else();
}

11. Braces may be omitted from ifs and elses only if they can be removed from all branches:

if (a == b)
    do_something();

if (a == b)
    do_something();
else
    do_something_else();

not:

if (a == b) {
    do_something();
} else
    do_something_else();

12. Loop and condition clauses should have spaces between the keyword and the clause, but not around the clause: if (a == b) not if( a == b )
13. Function calls should not have spaces between the function name and parameters or around the parameters: do_something(a, b) not do_something( a, b ) or do_something ( a, b )
14. Single line if statements are not allowed:

if (a == b)
    do_something();

not

if (a == b) do_something();

15. Do not use using for namespaces. using may be allowed for items in a namespace: using ra::data::Achievement instead of using ra::data.
16. Use struct for data containers. A struct should not have any methods beyond setters, getters, and comparison wrappers. Use class for anything requiring additional functionality.

• Most of the code already conforms to 1, 2, 3, 4, 5, 9, 10, 11, 14, and 16.
• For 6 and 7, most of the code uses hungarian notation: nVal and m_nVal.
• 8 does not currently apply.
• 12 and 13 are currently mostly spaces-inside, but I'd like to switch to spaces-outside.
• 15 currently only applies to rapidjson, and is currently implemented by using the entire namespace contrary to the proposal.

Once a proposal is agreed upon, we can define VS settings to enforce most of the proposal and autoformat the codebase, then use this ticket to commit the whitespace changes. Non-whitespace changes (like updating variable names) should not be made as part of this ticket. New code should follow any accepted styles, but existing code will be grandfathered in.

[SyrianBallaS]

Just a quick question about the namespaces. Did you want the sub-namespaces to hide implementation or expose it? If it's the later they should be inline namespace. Instead of doing using ra::data::Achievement you could just do using using ra::Achievement even though it's in another namespace. Otherwise it should just be a regular namespace.

Everything else seems acceptable. Though I personally disagree with some of it, but it's OK.
Edit: Another thing I noticed. The .editorconfig actually has to be in the solution/project to be used. At least as a "solution file".

Edit: @Jamiras I'm gonna hold off on doing anything on that #23 PR until I have a better understanding on unicode conversions.

[Jamiras]

From what I understand, inline namespaces are meant for versioning. If you put something in a namespace for any other reason, it shouldn't be inline. Namespaces should be used to group similar classes and keep them out of the current scope unless explicitly required. With proper segregation, it' takes more effort to create inter dependencies. Classes in ra::data shouldn't be talking directly to classes in ra::ui. As soon as you realize you have to explicitly reference the ui namespace, you'll take a moment to consider if it's the right thing to do.

[GameDragon2k]

Majority of these are in line with standard coding conventions. Don't have an issue with that for the most part. I do have comments on 6, 7, 11, and 14.

For 6 and 7, I personally prefer Hungarian notation. Just helps me to identify things faster and more concisely.

As for 10 and 11, I agree completely with braces on separate lines, but I'm not too onboard with removing braces only if no branches require them. I would prefer to put braces where they are needed and exclude them when they aren't.

if (a==b)
{
    do_something();
    do_another();
}
else
    do_else();

14 is a big issue for me. Single-line ifs are something I use a lot depending on the length of the statement.

[Jamiras]

Since most of the code already uses Hungarian notation, I'm fine changing 6 and 7 to support that.

I'm strongly against 14. Putting the if statement on the same line as the code to be executed reduces readability, couples the two statements in a diff if either changes, and makes it harder to put a breakpoint that's only hit when the condition is met. I will never do it, but if you feel that strongly about it, I'm fine with not requiring it.

Regarding 11: Symmetry improves readability, but I'm flexible.

I've drafted an updated proposal here: https://github.com/Jamiras/RASuite/blob/master/CONTRIBUTING.md

Are there any other changes or additions that are desired?

I'll look at creating a VS settings file and create a PR to globally reformat the code (whitespace only) next weekend.

[GameDragon2k]

14: From my pov, it increases readability, as it generally makes blocks smaller and requires less scrolling.

11: Flexible on this one as well, was just speaking what I typically do. My argument is mostly the same as my above statement. I just like smaller blocks.

[SyrianBallaS]

I'm fine with most of the proposal but I think the braces on control flow
statements should be on the same line and if one branch is a one liner it
shouldn't really need braces.

I'll try to tweak my prs to conform to this standard when I can.

Edit: Not a big deal now, but I think if we absolutely must use pointers (from what I could tell it can be easily avoided, directly at least, Window Handles would need custom deleters because handles don't have destructors) then we need to use ownership semantics via unique_ptr a small demo of it can be seen in #30
File: RA_Dlg_MemBookmark.h.
Function: Dlg_MemBookmark::ImportFromFile
Line: 805

if you are worried about size, the size of the unique_ptr is the same size as the pointer it holds. You just can't copy it, you can only move it. I think for most of the time, the parameters of functions can be pointers but immediately owned by another unique_ptr in the calling function. I say that because I noticed that pointers need to leave functions but usually don't and leak if not explicitly handled. Another way would be to pass the unique_ptr by reference (not const, it's incorrect) and it'll leave the function.

Also I noticed the .editorconfig isn't added as a solution/project file, that'll handle the indent spacing, adding the last new line (GitHub complains about that) automatically. That's something to consider when you autoformat.

[SyrianBallaS]

Not sure if it's common knowledge, but I think you should state that this repository should be a remote when forking when new stuff is added here so the forked repo can pull in the new changes.

[SyrianBallaS]

@Jamiras I think you should mention in the CONTRIBUTING file that people should back up their Visual Studio settings and import the provided Visual Studio settings in the repository if they want an eaiser time using the formatting conventions..