BannerlordCE / CEEvents

Captivity Events Source Code
https://www.nexusmods.com/mountandblade2bannerlord/mods/1226
MIT License
4 stars 6 forks source link

Development Branch Push #8

Closed TheBadListener closed 4 years ago

TheBadListener commented 4 years ago

Moved to development branch, master is now designated release branch. Need to test before pushing into development branch.

LogRaam commented 4 years ago

we can test it with a stopwatch.

Tested. It is indeed slower with Throws.

Tested with 1000000x

String 00:00:03.0920421 Throw 00:02:17.0254560

Would you like to replace Throws by return values? In CEEventChecker.FlagsDoMatchEventConditions() the Throw should happen only once if triggered. I'm waiting your input from this one before refac.

TheBadListener commented 4 years ago

Question is regards to using new class() vs using static functions.

In the instance of new CESubModule(), is it not more resource intensive to recreate a class constantly, rather then use static functions and variables?

LogRaam commented 4 years ago

We just have to set it at static, and ideally move it to the persistence class.

There is a small difference in performance due to boxing/unboxing. Here is the diff on my computer using PowerShell (C# is way faster):

image

The reason why it is a best practice to have instances instead of statics is because instances are testable. You can stub/mock them. They can be overloaded. They can have interfaces, and many more.

image

Statics are like the old plain GOTO; the code emerge from nowhere. Instances force you to design an OO architecture that have lots of benefits over statics. Statics in instanciable classes are confusing and generate artificial coupling.

image

..and still from the Clean Code convention:

image

So, if there is an issue with speed, static will give you a small boost (over millions of calls). Here is another test, each one was called a million times:

STATIC = + 00:00:03.2071035 Instanciate once = + 00:00:03.3034661 Instanciate each time = + 00:00:06.2622685

Instanciated once means that I instantiate an new object and use it 1000000 times. Instanciated each time means that I used a new instance a million time.

I personaly prefer not to use STATIC. If you think that STATIC are strategic in your project, then we can switch to them. But I believe we can achieve a better OO architecture without them. STATIC should be only persistence where you need to keep a state during the whole runtime of the app and you want to keep that persistence in RAM. The image seems to be a good case; it need to be in a persistent state because it is assumed that it's part of a current state in the game.

..there is more than speed and instances gives alot.

Here's a small talk about it: https://stackoverflow.com/questions/1530353/in-what-situations-is-static-method-a-good-practice

And this: https://softwareengineering.stackexchange.com/questions/232865/thoughts-and-best-practices-on-static-classes-and-members

and about testing: https://dzone.com/articles/why-static-bad-and-how-avoid

TheBadListener commented 4 years ago

Alright sounds good, just make sure the variables that were initially set static are not being wiped out, on-application tick.

Learning more everyday with how testing works in C# lol

LogRaam commented 4 years ago

The problem right now is that we don,t have tests. So refactoring are dangerous and have a high risk of breaking something. When the tests will be in place, they will prevent any regression bugs.

If you want to see the full potential of unit tests, take a look at NCrunch. This is an addon for Visual Studio that will run the tests in real time when you code. https://www.youtube.com/watch?v=ejuA0qbpcz0