Naming and Layout/Style Proposal about raintegration

[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..