Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Some thoughts... #2

Open
soggybag opened this issue Jul 12, 2022 · 1 comment
Open

Some thoughts... #2

soggybag opened this issue Jul 12, 2022 · 1 comment

Comments

@soggybag
Copy link

soggybag commented Jul 12, 2022

_months: string[]

You are storing the arrays of months and days as instance properties. That means that every instance will own their own array. This is an obvious solution but looking deeper the arrays will be duplicated for each instance of this class. Seems like there is no need to copy these since they are not changed or modified for each instance!

Here are a couple possibly better solutions.

Put the arrays of months and days on a class property. D.months = ['Jan', ... ]. Now there is only one copy of the array ever created.

A better and more modern solution would be modules. Create a new file that defines the months and days arrays and export them. Any files that need these arrays can import them. This solution sounds almost too simple but I think it's the best in this situation.

// helpers.js
export const months = ['Jan', ...]
// index.js
import { months } from './helpers'

Having a "months" variable floating around might seem like an issue but in the world modules these variables will scoped to the module! So they are only available in the module where it is defined, and in the modules that import it.

Also, the code in modules is only defined once no matter how many times it's imported.

The downside to using a module is the array passed around by reference. This means it would be possible for one of the importers to modify the array and this would affect all importers. Since you're using TypeScript you could make your arrays ReadonlyArray!

@stanjdev
Copy link
Owner

Thank you for the tip! Just made the changes!

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

No branches or pull requests

2 participants