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

JavaScript: Fixes to comparisons, iteration #3022

Merged
merged 3 commits into from
Nov 17, 2019

Conversation

ferdnyc
Copy link
Contributor

@ferdnyc ferdnyc commented Sep 27, 2019

As Codacy was kind enough to point out, we should:

Use === and !== with bool, numbers

We should really be using === and !==, instead of == and != (respectively) when comparing values to Number or Boolean, to ensure that we're not accidentally type-converting values unexpectedly.

Iterate over arrays with for...of loops

variable[key] syntax is evil, which makes loops of the form for (var index = 0; index < array.length; index++) { } inherently problematic, because they're going to access array[index] inside the loop.

Fortunately, there's a better way: for...of. Not only is is better/safer, but it often makes the code so much cleaner. So, I've replaced all of the indexed for() loops that looped over arrays from [0 ... length] with for (const element of array) { } loops. (Which is another advantage to for...of loops: The iterator variable can be const, if the array contents don't need to be modified.)

So for...of both cuts down on temporary variables and helps keep everything block-scoped. (var has scoping issues. In fact, by changing to const iterator variables I already caught one function where a variable was being redeclared inside a large-ish loop. In that particular case it didn't matter, but the fact that var allows that to happen silently is a problem.)

As I said, I replaced all of the for (index = 0; index < array.length; index++) loops. I left a few indexed for () loops alone, though, like the ones that iterated backwards over the list elements from length - 1 to 0. Basically, if it couldn't be trivially converted to for...of, I let it be.

@ferdnyc ferdnyc changed the title JavaScript: Use === and !== with bool, numbers JavaScript: Fixes to comparisons, iteration Oct 20, 2019
Comment on lines 423 to 430
// Find matching clip
for (var clip_index = 0; clip_index < $scope.project.clips.length; clip_index++) {
if ($scope.project.clips[clip_index].id == clip_id) {
for (const clip of $scope.project.clips) {
if (clip.id == clip_id) {
// Set audio data
$scope.$apply(function(){
$scope.project.clips[clip_index].audio_data = audio_data;
$scope.project.clips[clip_index].show_audio = true;
clip.audio_data = audio_data;
clip.show_audio = true;
});
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mentioned that for...of often makes the code cleaner: This change makes for a good representative example. The updated version is so much more readable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@jonoomph
Copy link
Member

LG

@jonoomph jonoomph merged commit ee7ee67 into OpenShot:develop Nov 17, 2019
jonoomph added a commit that referenced this pull request Nov 18, 2019
@jonoomph
Copy link
Member

@ferdnyc Well, on actual testing of this PR, it breaks all HTML rendering / JS syntax issues. Maybe a Qt version issue? Not sure, but I've kept some of the PR (the === comparisons), but reverted the loop code.

@ferdnyc
Copy link
Contributor Author

ferdnyc commented Nov 19, 2019

@jonoomph

Hrmf, that's too bad. I suspect it's a WebKit version issue. Everything worked fine in my local testing, but that's running with a QtWebKit build that's less than two months old, based on presumably a very recent WebKit. (Though I can't seem to find anyplace that actually indicates specifically what version, which is annoying.)

for...of was introduced in ECMAScript 2015 6/ed, and is supported in Chrome starting with version 38. That's fairly recent, and I guess I shouldn't be surprised that not all of our WebKit-based engines are new enough for it to be usable. Oh well.

@ferdnyc ferdnyc deleted the js-compares branch November 19, 2019 00:12
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

Successfully merging this pull request may close these issues.

2 participants