Guidelines for refactoring

[ghost]

Game is a masterpiece but its code is twisted as hell.

I think it would be a good thing for health of the project to refactor stuff when we have opportunity to.

However the project is currently lacking guidelines about this. This ticket is to discuss and debate about these guidelines. Here's some example points :

• is refactoring allowed at all ?
• what can(not) be refactored (what is worth refactoring, what must not be refactored not to break things etc..)
• maximum size of refactoring PRs
• is refactoring can be in the same PRs/commits as bugfixes/feature add ? or should be completely alone ?
• are refactorings must be at all times first be approved in a ticket dedicated for this ?
• minimum code quality for it to be considered as a refactor
• other stuff

Once these guidelines completed, that will prevent people from working hours on a PR that will get refused for X reason, as well as motivate people to refactor code because they will be aware of what they are getting into.

[ghost]

• is refactoring allowed at all ?

It is encouraged. Really, please, do as much as possible, we need help!

• what can(not) be refactored (what is worth refactoring, what must not be refactored not to break things etc..)

I'm sad to say that, but usually, everything that touches memory management is hard to refactor. Also, struct ident and correlated things are dangerous. I may add: avoid touching, for now, buffer stuff, or you'll need a major refactoring to keep current behavior.

*maximum size of refactoring PRs

None? As long as you keeep single patches as easy to read as possible? As usual, atomic commits are the easier ones, especially for refactoring.

• is refactoring can be in the same PRs/commits as bugfixes/feature add ? or should be completely alone ?

It would be really appreciated if you could have those separated. Really.

• are refactorings must be at all times first be approved in a ticket dedicated for this ?

As far as I'm concerned, I don't enjoy github. So, if I can reduce my interactions with it, it's better. We are also not a lot to contribute to this project, and IRC is sometimes easier to use that "login>>write>>oh this changed, rewrite>> oh someone replyed>>etc".... you got the idea. We have peer review and CI for that after all. And we still intend to stay RE1.x compatible. We, on the other hand, have a forum on https://blue-nebula.org feel free to join us :)

• minimum code quality for it to be considered as a refactor

What do you mean?

[ghost]

I think the project should encourage refactoring because :

• adding features in debted code often adds more technical debt
• sometimes, most efficient way to fix bug is to refactor twisted code
• could attract devs who want to do refactoring for fun (I do).
• clean code will attract more contributors, which will make game better
• clean code will help to keep current contributors

Things that could be somewhat quick, easy and worth to refactor are :

• replacing magic values with constants
• giving better names to variables and functions
• replacing widespread identical huge function inputs with structs
• flatten block nesting
• use enums instead of enumish-integers
• replace huge similar ifs conditions with well named functions
• replace new variables with auto if possible
• formatting lines that are more than X character length

When all these things done, it will be way easier to refactor the architecture itself.

I would submit 1 PR per refactored feature, any amount of commit; for example to prevent spending 1 week refactoring 200 lines in 5 different files, then pushing, then have to deal with lots of code conflicts (refactor changes the code a lot), that would frustrate everyone. Also quicker to review and test, less scary to merge.

Before adding a feature / fixing a bug, I would :

1. check if its worth refactoring first
2. ask permission to refactor in the ticket
3. refactor
4. push refactor PR and wait for it to be merged
5. add the feature / fix bug
6. push PR

I think refactors should be approved first to avoid spending too much time on something that risk being declined, demotivating all sides.

Minimum code quality for it to be considered as a refactor : if someone is going to refactor something, he better doing 100%, and not partially, because it would cause even more complicated code by having more ways of doing the same thing.