Node: `class` implementation for files

Created on 25 Mar 2020  路  8Comments  路  Source: nodejs/node

Is your feature request related to a problem? Please describe.
Please describe the problem you are trying to solve.
I am just walking through the code and I hopped onto this file lib/internal/cluster/round_robin_handle.js and it seems that one can rewrite the whole file using classes.

Describe the solution you'd like
Please describe the desired behavior.
As we are moving to newer syntaxes and can see more legacy code is being ported. I would like to propose to rewrite this file using classes.

Describe alternatives you've considered
Please describe alternative solutions or features you have considered.
I am a newbie and still exploring the code base and happy to learn about the tradeoffs.

__PS__: Also I would like to work on refactoring this file as well.

Most helpful comment

We don't generally do refactoring for the sake of refactoring because it messes up git blame history.

All 8 comments

I think refactor to class implementation is a good idea, though it's hard for some classes, but maybe we can start from the small class just like you exampled

We don't generally do refactoring for the sake of refactoring because it messes up git blame history.

@devsnek understood, but wouldn't it be correct to go with newer syntax and porting code in them, will require these type of changes to be implemented.

I think refactoring code to use classes is just fine in terms of refactoring.

Yeah, reworking things into ES6 classes is generally fine as a refactoring exercise but needs to be done with caution. Moving from the the legacy function object to Class model is a semver-major change when it modifies public facing API. That is because Class objects require the use of new to instantiate while legacy function objects do not. Using a class also changes the prototype structure of the object and changes the default visibility of methods on the object (important if your code does things like iterate over the functions). This has tripped us up before so to be cautious we treat it as semver-major. This largely needs to be looked at on a case-by-case basis.

That all said, for internal-only objects like round_robin_handle, refactoring into a Class should be fine but try to see if there are some other improvements that could be made in addition to just updating the syntax.

Yes, @jasnell Looking into improving some complexity of operations as well.

my thought is to refactor the classes which in the internal code and user couldn't operate, step by step. and we avoid legend behavior like the prototype or default visibility of methods that @jasnell examples.
anyways, I don't see the tremendous benefits to do refactor for now.

@Himself65 along with the cosmetic changes, I have also reworked on improving the complexity of some operations like checking worker from the free pool every time on the array is an O(n) operation and as from the blame, I can see that some parts of the code were previously implemented with arrays only but later changed to map implementation as it is more performant on average times. So did my bit there. Also, there were some changes around the mapping of variables across different scheduler implementation (eg. round_robin vs shared_sample). So there are also logical changes as well. Hope that explains my solution.

Was this page helpful?
0 / 5 - 0 ratings

Related issues

addaleax picture addaleax  路  3Comments

stevenvachon picture stevenvachon  路  3Comments

dfahlander picture dfahlander  路  3Comments

sandeepks1 picture sandeepks1  路  3Comments

willnwhite picture willnwhite  路  3Comments