Set and enforce a consistent code style

just-ero commented 1 year ago

Arch is currently using unconventional code styles in a lot of places. I would like to propose some more consistent styles, as well as some other things to potentially look at. Enforcing a consistent code style allows for a better overall coding and reading experience.

Major things this project needs to tackle:

Misspellings in both documentation and code.
Using one file for multiple types.
Misuse of the in modifier.
Overuse of the MethodImpl attribute.
Difficult readability because of single-line statements.

Below, I've listed some conventions this project can enforce. These are up for discussion though. I'm happy to help implementing this.

The below can also be found here as an .editorconfig file.

Naming Conventions and Accessibility Modifiers

private, protected, private protected, and protected internal fields should be _camelCase. The same applies when they're static.
internal and public properties should be PascalCase. The same applies when they're static.
const locals and fields should be PascalCase.
Parameters should be camelCase.
Methods, enums, structs and classes (and their record equivalents), and namespaces should be PascalCase.
Type parameters should be TPascalCase.
Interfaces should be IPascalCase.

Fields should never be internal or public. Properties should never be private, protected, private protected, or protected internal.
Accessibility modifiers should be required at all times.
readonly should be added whenever possible.

`var` Preferences

Do not use var anywhere.

.NET Conventions

System using directives should always be sorted first.
.this qualification should never be used.
Predefined types (int) should always be used over framework types (Int32).
Parentheses should be used for clarity:
- In arithmetic operators: a + (b * c)
- In other binary operators: a || (b && c)
- In relational operators: (a < b) == (c > d)

Expressions

Should prefer index and range operators.
Should prefer simple default expression.
Should prefer object and collection initializers.
Should prefer conditional expression (?:) over if with assignments and returns.
Should prefer local function over anonymous function (lambda).
Should prefer implicit object creation when type is apparent (target-typed new).
Do not use inferred tuple element and anonymous type member names.
Do not use expression body for constructors.
Do not use expression body for local functions.
Do not use expression body for methods.
Do not use expression body for operators.
Do not use expression body for indexers.
Do not use expression body for properties.
Should prefer expression body for accessors when on single line.
Should prefer expression body for lambdas when on single line.

Pattern Matching and `null` Checking

Should prefer pattern matching when possible.
Do not use throw-expression (obj ?? throw new ArgumentNullException(nameof(obj));).
Should prefer conditional delegate call.
Should prefer coalesce expression.
Should prefer null propagation.
Should prefer is null over ReferenceEquals or == null.

New Line and Braces Preferences

Do not allow multiple blank lines.
Do not allow embedded statements on same line.
Do not allow blank lines between consecutive braces.
Do not allow statements immediately after block.
Do not allow blank line after colon in constructor initializer.
Should prefer opening brace after if, else, case blocks.

andreakarasho commented 1 year ago

Fields should never be internal or public.

Except for structs!

genaray commented 1 year ago

Hey ero ! :)

Thanks for pointing this out, i could actually need every help i can get with this. I already updated the official readme yesterday to fix some misspellings, however theres still a lot more to it. I also havent looked at the code yet, probably my rider code settings are messed up. Thats the first think im gonna check.

Fields should never be internal or public

Thats really something i use quite frequently. I actually did that since in some cases fields are actually being faster compared to properties. Especially when the propertys get is not being marked inline. So the question here is actually what to prefer, however such cases are pretty rare.

Overuse of the MethodImpl attribute.

Understandable, how would you decide whether its necessary or not ? Unfortunately we can not test the performance effect on every single method. I think atleast query related methods should keep that since it lowers the cache miss in such cases. I already benchmarked this months ago and it was the case for such scenarios.

However the effect on other methods is still unknown, based on experience methods marked with inline always performed better which is kinda important for this lib. So probably we should just ignore that inline is being spammed everywhere, you can see similar decisions in other ECS libs like "Default.ECS".

Do not use var anywhere.

Why is that ? I can understand that it might make the code harder to understand at first, however its way more convenient to use. Probably we should focus on ensuring that the code is readable so that everyone automatically understands what type the var is e.g. : var entity = world.CreateEntity(...); instead of Entity entity = world.Create(...);. ( furthermore it makes refactoring easier )

I agree on the rest however and could need some help here. Thanks for the editorconfig, just saw that. Im gonna use it :)

EDIT : Most misspellings ( readme, wiki ) are fixed now, used grammarly. However, there may still be some funny-sounding sentences.

just-ero commented 1 year ago

Fields should never be internal or public

[...] in some cases fields are actually being faster compared to properties. Especially when the propertys get is not being marked inline.

The big issue here is really the ref modifier for struct fields. I'm unsure of why it's used as much as it is (I just genuinely lack the knowledge), but unfortunately there's not really a way around this one at all. Properties simply can't use the ref modifier even if they're auto-properties.

Properties' getters and setters can be marked inline just like any other method:

public int Foo
{
  [MethodImpl(MethodImplOptions.AggressiveInlining)] get;
  [MethodImpl(MethodImplOptions.AggressiveInlining)] set;
}

Overuse of the MethodImpl attribute.

Understandable, how would you decide whether its necessary or not?

This is all very difficult to test.
Generally, you want to be entirely sure that the method to be inlined is either very small (properties, indexers, constructors) or on a hot path. You want to verify that inlining generally generates smaller code.
To really make sure the attribute is necessary at all, you'd want to compare the generated JIT ASM before and after inlining and make sure from there that the newly generated code is worth it at all.

Slapping the attribute on every method you feel like can actually have negative effects. Think about it this way: if you inline a method which is on a path that's hit rarely, then the code generated is going to be bloated with unnecessary instructions that don't get executed.

Do not use var anywhere.

Why is that?

All of these were merely suggestions. I like being explicit personally. If you'd like to keep using var, then you can, of course. It's just important to be consistent: either use var everywhere, or nowhere (in my opinion).

genaray commented 1 year ago

The big issue here is really the ref modifier for struct fields. I'm unsure of why it's used as much as it is (I just genuinely lack the knowledge)

Well ref fields are great for various purposes, for example to reduce copies... or to return a ref which is kinda important for the ECS since its more efficient to write directly to a adress :

var references = entity.Get<Position, Velocity>();  // Returns public struct References{ ref Position t0, ref Velocity t1 }, one single operation...
references.t0.X += references.t1.X;
references.t0.Y += references.t1.Y;

alternative

ref var pos = ref entity.Get<Position>();       
ref var velocity = ref entity.Get<Velocity>();   

// 2 Operations to receive a ref to the structs we wanna modify... two lookups instead of 1 = bad for hotspots

It might violate several code conventions, but is extremely usefull and more performant since it basically batches operations ^^

Properties' getters and setters can be marked inline just like any other method: In this case we should probably use this more often, there is a need for action here :D

Generally, you want to be entirely sure that the method to be inlined is either very small (properties, indexers, constructors) or on a hot path.

Thanks, im gonna take a look at that... most methods are actually being used in hotspots (e.g. all Chunk and Archetype related methods, they will end up in the game loop for iterations and are mostly branchless). However probably other methods like Create, Destroy,... could be non inlined. I think i will test this with a benchmark in the next few days ^^

andreakarasho commented 1 year ago

Properties are just syntax sugar of: get_PropertyName() and set_PropertyName(value). I think Arch needs some code refactor, but I suggest to do not touch structs fields

genaray commented 1 year ago

but I suggest to do not touch structs fields

I support this, structure fields should not be touched or converted to properties. I also suggest that they stay public, this depends on the struct itself. However there a lot of usecases for properties inside classes. But theres a lot of other refactoring to do... im currently fixing more misspellings in the World.cs file.

just-ero commented 1 year ago

Do you have some branch you're using to work on this?

genaray commented 1 year ago

Do you have some branch you're using to work on this?

Yep, its still local however ^^ Unfortunately I'm not home today, but any other branch will also do the trick.

just-ero commented 1 year ago

I've been doing some work over on just-ero/Arch#experimental-code-style. Here's a summary of what I did:

bbdb94eaddffd44e3733245a8183a5d0d306cff0:

Create src directory, move project folders there.
- Rename Arch.Benchmark to Arch.Benchmarks and adjust namespaces accordingly.
- Rename Arch.SourceGenerator to Arch.SourceGen and adjust namespaces accordingly.
- Rename Arch.Test to Arch.Tests and adjust namespaces accordingly.
Create (slightly edited) .editorconfig file.
Clean up all .csproj files.
- This could be a breaking change. I'm not really sure how much of the NuGet package information needs to be in the project file. It doesn't seem like a place where you'd want to keep that.
- Change elements (where applicable):
- Add <EnforceCodeStyleInBuild>true</EnforceCodeStyleInBuild>.
- Add <Nullable>enable</Nullable>.
- Remove <ImplicitUsings>true</ImplicitUsings>.
  - Use <ItemGroup/> with <Using/> elements instead.

914697f1299ccb43f9c7ecbeadd754a71b653b6e:

Enforce .editorconfig:
- Whitespace changes (remove trailing whitespace, add spaces for operators, add final new line, etc.).
- Add readonly modifier where possible.
- Add nullable reference types where applicable.
- Breaking?
- Add braces to blocks.
- Change variable names to fit naming convention.
Enforce member order:
- Fields -> Constructors -> Properties -> Methods.
Use raw string literals (applies heavily to Arch.SourceGen).
Use Assert.That over other assertions, as recommended by NUint.
Use is null and is not null.

188eabee0a296eaf7f56f9dbd4faa299590f246b:

Clean up comments.
Address a lot of documentation comments:
- I started to become lazy and burnt out here. At some point I just removed all the doc comments, replaced them with the empty template, and slapped // TODO: Documentation. on top. I did this because a lot of them simply didn't explain much of anything, some were just copy-pasted, and others had an incorrect order for the parameters.
- Use some consistency for certain things:
- Types ("The TypeName class/struct/interface/enum", followed by some description):
```
/// <summary>
///     The <see cref="TypeName"/> class
///     has some purpose.
/// </summary>
```
- Constructors ("Initializes a new instance of the TypeName class/struct", followed by potential parameters):
```
/// <summary>
///     Initializes a new instance of the <see cref="TypeName"/> class
///     with some parameters.
/// </summary>
```
- Properties ("Gets", "Sets", "Gets or sets"):
```
/// <summary>
///     Gets or sets some value.
/// </summary>
```
Add a bunch of annotations (TODO, FIXME, NOTE) for potential enhancements and other things to look at.

genaray commented 1 year ago

I've been doing some work over on just-ero/Arch#experimental-code-style. Here's a summary of what I did:

Took a quick look at it, looks promising :) Thanks ! The next few days are quite busy for me, but i will merge it.

Just a small side note, SparseArray is intended to be non generic and I'm aware of this ^^ A lot of low-level stuff or ECS stuff is probably not valid with code convention but its actually intended and useful.

The SparseArray aswell as the Chunk are non generic since making them generic is not really possible. Well it is possible, but it would require reflection which is slow and a no go. So the easier and more efficient way is to use the current approach, its probably harder to understand, but requires no reflection, is fast and multi purpose oriented. Just a quick side note :D

Since this is mostly low-level and high-performance, we often need to decide between efficiency and code conventions... in this case we should always focus efficiency.

genaray commented 1 year ago

I've been doing some work over on just-ero/Arch#experimental-code-style. Here's a summary of what I did:

Sooo looks great :) Thanks ! I already pulled it into the main repo and recently pushed the documentation for the chunk. You may want to look at it, hopefully its better and more streamlined now. https://github.com/genaray/Arch/blob/feature/code-style/src/Arch/Core/Chunk.cs

I also moved the properties directly behind the constructor. Somehow they were not moved. Other files will follow soon.

genaray commented 1 year ago

Archetype now also received documentation... i also made some fields private and moved them above the constructor to make them follow the order :) https://github.com/genaray/Arch/blob/feature/code-style/src/Arch/Core/Archetype.cs

genaray commented 1 year ago

Documentation is finished and was merged with 7f2b07c :)

Now I'm gonna work on the notes and other mentioned code style issues.

genaray / Arch