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

Refactor #1

Closed
wants to merge 1 commit into from

Conversation

@scallaway
Copy link

commented Oct 9, 2019

Put together a PR addressing some of the things I've seen in the library in terms of layout and code styling etc.

Some context to this PR

Try and keep consistency, this includes things such as the positioning of brackets, the use of semi colons, and the types of quotes that you're using. Using a linter such as ESLint (even in the default state) would wipe out these issues entirely.

On the back of this, be sure to remove any unecessary whitespace that is in the file. Your idea will be able to do this for you automatically when you save if you enable it in the settings.

Remember the acronym DRY, Don't Repeat Yourself. In each of the methods that the user has access to, you were writing the same two lines; retrieve the JSON file from the disk, and then parse it. By moving those two lines out into their own function, this reduces the amount of duplicated code and keep also reduces the number of lines in each of those functions. You could take this a bit further and make some of those just single-line functions, but I'll leave that up to you.

Whenever possible, use triple equals instead of just the double equals. This also includes instances when you're doing negative checking (!==) as well. A quick example of this would be the following;

2 == '3' // true
2 === '3' // false

By using the triple equals, JavaScript will also check the type of the operands being used and will check on those as well - rather than just trying to coerce one or both of the operands. More info on that can be found here.

It's good to see that you're willing to use array methods such as sort and forEach, but also try to be consistent with those as well. There shouldn't really be a reason to chop and change between forEach and the regular for loop since you have access to the index within the former through the second parameter of the callback function.

I believe there are instances where you could map, but I felt that was out of the scope of the PR and I'll let you go through and discover that in your own time.

If you need anymore explanation, feel free to comment here or message me on Discord.

@hcphoon01

This comment has been minimized.

Copy link
Owner

commented Oct 9, 2019

Thank you for your help. All the changes you've made I have included but for some reason I did it manually so I'm going to close this PR without merging it.

@hcphoon01 hcphoon01 closed this Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.