MartinsOnuoha / countriesNowAPI

CountriesNow is an Open source API for retrieving geo-information for countries, including their states, cities, population, etc. 🌎
http://countriesnow.space
MIT License
179 stars 55 forks source link

:zap: (redirectPostToGet.js) remove iteration over entire prototype chain #101

Closed MartinsOnuoha closed 2 years ago

MartinsOnuoha commented 2 years ago
TimAagaard commented 2 years ago

You should add SonarLint to your IDE / SonarCloud to your pipeline. For-in loops are favored when available due to readability and negligibility in time trade off of looping over the prototypes.

SonarCloud is free for open source projects and will find and alert to security vulnerabilities and bad coding practices.

I can't speak to whether this is different between JS and TS but I've been using for-in loops in TS specifically because Sonar static analysis complains about other methods of iterating over key/values in an object.

I saw a 200% - 600% improvement in cached responses vs uncached responses in my last PR so that should more than offset iterating over prototype.

But I do think using Sonar for good programming practices is something we should do when migrating to TS...as well as updating all dependencies to their current versions since the majority of them are very out of date.

MartinsOnuoha commented 2 years ago

@TimAagaard Yes I agree for-in does increase readability in most cases, although I believe when dealing with objects, using object methods seems more readable, but then this is just a personal opinion, and I guess SonarCloud might be interesting to add to the pipeline if it has some default presets for best practices, but would you suggest SonarCloud over Deepsource - the current static analysis tool integrated into our pipeline? Ultimately we could configure Deepsource to use whatever best practices specification we agree on. Unless SonarCloud is a better alternative overall?

Yes, I noticed the performance increase as well, and will merge the PR asap, awesome work.