Replacing classes with modules and functions #46
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Cruikshanks
added
the
housekeeping
|
We have #37 ready to be merged. Going to wait for that then will rebase this, make the changes and then make it ready for review. |
> Classes are a template for creating objects. That is the first sentance of the Mozilla documentation for **Classes**. It is what those of us from other languages understand classes to be for. Some of us though (we're looking at you @Cruikshanks) classes are _all things_! Because of that thinking, we've allowed Classes to creap in where they shouldn't really be used. Most noticeably, things like controllers, presenters and services, which are formed of purely static methods. There is no real reason to wrap their functionality in a class. They could just be expressed as modules and export their relevant functions. That's what those experienced with JavaScript are more likely to expect. By not doing things that way we're breaking the [Principle of least astonishment](https://en.wikipedia.org/wiki/Principle_of_least_astonishment). We're believe classes are right for our Models, as it follows the convention of [Objection](https://vincit.github.io/objection.js/guide/models.html) and we do create instances of them. Also, if we have need to create objects in the future, classes will be our go-to. But before this repo gets to far down the road we're taking this opportunity to refactor things from classes into modules.
Note - this forced us to review how we'd handled 'current date' in the BillingPeriodService. We found Sinon's [useFakeTimers()](https://sinonjs.org/releases/latest/fake-timers/) which is the more appropriate mechanism for controlling what `Date` returns during our tests.
Cruikshanks
force-pushed
the
going-classless
branch
from
14737f4 to
6d3d1a3
Compare
Cruikshanks
added a commit
that referenced
this pull request
Dec 9, 2022
Follows on from [Replacing classes with modules and functions](#46) as we managed to miss some bits! This second attempt hopefully covers alll the bits we missed on the first pass of refectoring things away from classes.
Cruikshanks
added a commit
that referenced
this pull request
Dec 9, 2022
This follows on from [Replacing classes with modules and functions](#46) as we managed to miss some bits! This second attempt hopefully covers all the bits we missed on the first pass of refactoring things away from classes.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
That is the first sentence of the Mozilla documentation for Classes. It is what those of us from other languages understand classes to be for. For some of us though (we're looking at you @Cruikshanks !) classes are all things!
Because of that thinking, we've allowed Classes to creep in where they shouldn't really be used. Most noticeably, things like controllers, presenters and services, are formed of purely static methods. There is no real reason to wrap their functionality in a class. They could just be expressed as modules and export their relevant functions.
That's what those experienced with JavaScript are more likely to expect. By not doing things that way we're breaking the Principle of least astonishment.
We believe classes are suitable for our Models, as it follows the convention of Objection and we do create instances of them. Also, classes will be our go-to if we need to create objects in the future.
But before this repo gets too far down the road we're taking this opportunity to refactor things from classes into modules.