Comments (11)
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.
from raintegration.
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.
from raintegration.
I've been looking at the code, and have the following proposal:
- Class names should be
PascalCase
- Struct names should be
PascalCase
- Enum names should be
PascalCase
. Enum items should also bePascalCase
- Method names (global or class methods) should be
PascalCase
- Constant names should be
ALL_CAPS
- Variable names (local or class members) should be
camelCase
- Global variables names should be
camelCase
with ag_
prefix:g_camelCase
- Namespace names should be
snake_case
. The top level namespace will bera
. - Pointer and reference markers should be with the type:
char* ptr
, notchar *ptr
. - 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();
}
- 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();
- Loop and condition clauses should have spaces between the keyword and the clause, but not around the clause:
if (a == b)
notif( a == b )
- Function calls should not have spaces between the function name and parameters or around the parameters:
do_something(a, b)
notdo_something( a, b )
ordo_something ( a, b )
- Single line
if
statements are not allowed:
if (a == b)
do_something();
not
if (a == b) do_something();
- Do not use
using
for namespaces.using
may be allowed for items in a namespace:using ra::data::Achievement
instead ofusing ra::data
. - Use
struct
for data containers. Astruct
should not have any methods beyond setters, getters, and comparison wrappers. Useclass
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
andm_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.
from raintegration.
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.
from raintegration.
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.
from raintegration.
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.
from raintegration.
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.
from raintegration.
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.
from raintegration.
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.
from raintegration.
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.
from raintegration.
@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..
from raintegration.
Related Issues (20)
- Inactivated Leaderboard with Value Alt Groups Crash
- [Suggestion] Remove/Alter Memory Bookmarks "Description Override" feature HOT 2
- [Feature Request] Add manual sorting option to Asset List
- UINT_MAX and Rich Presence HOT 3
- Attempting to "Publish All" while Assets List includes a modified Rich Presence causes Assertion Error
- Bug Report: Multi Select on Pointer Finder results
- Addresses in Memory boomarks should not be sorted alphabetically HOT 2
- 'Initial' filter removing valid results on aligned searches
- Comments cannot be added to the display string in RP, because of how empty lines are handled HOT 1
- Indirect Code Note Cutoff HOT 1
- Indirect Code Note Crash HOT 2
- Code Note Menu crashes when loading List with deleted Notes HOT 1
- Memory View flickers on drawing/clearing local code note text
- Double Input in Memory Bookmark value after invalid input HOT 2
- Achievement's icon in progress indicator widget is monochrome HOT 2
- Add developer mode that will not send unlock requests HOT 3
- No error message when attempting to overwrite code notes as a Jr Dev
- Delta issue when loading save states HOT 1
- Line-Breaks in Code Notes wrongly translated in Emulator HOT 2
- Wiped local code notes not processed upon re-opening. HOT 1
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from raintegration.