-
Notifications
You must be signed in to change notification settings - Fork 84
[WIP] Refactoring #643
base: dev
Are you sure you want to change the base?
[WIP] Refactoring #643
Conversation
@mugabe, WOW! You've slain the beast! I've taken this up several times only to give up about a quarter in. I'm thoroughly impressed that you've managed to stick this out. I can't thank you enough.
This problem goes back to the genesis of the project both in its original intent and the technologies available at the time. I've touched on this a few times in previous issues. It's also some of those same early decisions that causing beast no. 2, precision issues.
At this point I think you've probably already done the bulk of the heavy lifting so the rest is just an investment of time as you mentioned. The only other conversion that may require some thinking is the way the add-ons are loaded. At this point, since we're moving to webpack, it makes sense to get rid of this and just handle this through bundling. Most people seem to load everything anyway. Additionally, a substantial number of users aren't heavy JS coders so we'd have to do a good job at documenting bundling.
Agreed. At some point I went back and cleaned up quite a few of these but some of the more complex ones still remain.
I personally think that this is unavoidable. Either way, maintainability takes precedence.
As I mentioned, THIS IS HUGE! I have no problem moving the changes from Thank you again for this awesome PR. |
Hello!
Few days ago I've tried to replace the parser by custom one with my own syntax.
However it turned out that it is not so easy 😀
First, I got lost in HUGE file. It's very hard to understand where is the parser begins and where it ends. It's everywhere! Then my IDE crashed, almost taking CPU to the next world.
At this point I almost gave up. I have started looking for another libraries, researched one after one, another after a fifth, none of them fit my desires. So I gave up completely at this moment! And suddenly found myself writing own solver. I would have continued to create it if i had not remembered that HUGE HUGE file at the beginning. It's not that amount of code that I could redo myself.
Seriously, it's HUGE HUGE HUGE work behind this that you guys are doing. You are gorgeous! 🥰
I went back to the starting point and started to extract little pieces of code into distinct files, so after a few days of this extensive refactoring, I unraveled almost everything in nerdamer.core.
Serious part
This is great and complex library. But it's very hard to support and modify program of this size and complexity just in one file. It's almost impossible to debug, to find bugs, but extremely easy to add new. It's disaster for new contributors.
Since I plan to use this library and modify it for my own needs - I want to make it easer to use. More comfortable, more satisfying.
Further a little about principles of this refactoring.
TypeScript
Types are everything. Coding with autocomplete extremely easy. But much more important that strict types helps to avoid new errors and expose old ones.
So part of code already converted to TS - mostly small and simple classes. Large ones still in JS as converting requires a lot of manual and boring work.
SOLID
I'm trying to stick to SOLID principles. So any piece of code that knows more about the environment than it should have to be refactored.
Back compatibility
Some internal parts dramatically changed, but I'm trying to keep exposed and documented API unchanged. However, some parts of the API requires very complex dependencies with bad code smell, and it breaks my dreams from previous paragraph. So I have a plan to break API at some point.
Right now release version of library could be replaced with this one without any noticeable changes.
Just design, not bugs
At the moment I don't touch any algorithms, just changing architecture. Even if I found a bug or possible bug - I keep it untouched. Sometimes it's very hard to understand is this bug or feature.
For example abstract equality operators often lead to bugs.
nerdamer/nerdamer.core.js
Line 10609 in af39dbf
But in this scenario bugs arise if operators strict. It's hard to understand should I change only operator or rvalue too without further investigation. In some cases I add FIXME comment without touching the code.
But bugs, sometimes
However. There are a lot other fun things like fors with definitely wrong conditionals - they doesn't loop anything, but removing them will produce errors. The reason of this behaviour is
var
scoped variables - sometimes they can cause to bug. Another time they keeps magic value which would be used as initial in far another part of program without knowing about it. Sovar
is NO-NO and always should be avoided. And one of the tricky part of refactoring - replacing allvar
tolet
and then trying to catch any mysterious errors.Maybe I've added some more bugs, but maybe fixed another undiscovered. At least one has been occasionally fixed.
nerdamer/Solve.js
Line 512 in b894978
Most likely at some point
var n
has been removed somewhere else, so it is now undefined. Add missing declaration here - viola! One more test passed.Outro
As mentioned before this branch could be used right now as replacement of original. You shouldn't see the difference (I hope).
But there is still a lot of work to be done - some methods are placed in wrong classes, some classes use global variables to resolve dependencies, and some other things that need to be fixed.
Now I would be happy with any feedback.
P.S. I started refactoring on the master branch, so now it's behind few commits of dev 🥺