Skip to content
This repository has been archived by the owner on Apr 18, 2023. It is now read-only.

[WIP] Refactoring #643

Open
wants to merge 48 commits into
base: dev
Choose a base branch
from
Open

[WIP] Refactoring #643

wants to merge 48 commits into from

Conversation

mugabe
Copy link

@mugabe mugabe commented Nov 8, 2021

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.
ezgif-3-dc6883f9543a
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.

if(symbol.multiplier.den != 1 && Math.abs(x.power) == 1)

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. So var is NO-NO and always should be avoided. And one of the tricky part of refactoring - replacing all var to let 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.

n = (tries % Math.floor(halfway)) + 1;

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 🥺

@jiggzson
Copy link
Owner

@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 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.

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.

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.

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.

For example abstract equality operators often lead to bugs.

Agreed. At some point I went back and cleaned up quite a few of these but some of the more complex ones still remain.

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.

I personally think that this is unavoidable. Either way, maintainability takes precedence.

P.S. I started refactoring on the master branch, so now it's behind few commits of dev 🥺

As I mentioned, THIS IS HUGE! I have no problem moving the changes from dev after the merge. I haven't actually looked at your work yet so please bear with me since it might take a few days or so to find time to tackle this.

Thank you again for this awesome PR.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants