carlos-montiers / enhancedbatch

Enhances your windows command prompt https://www.enhancedbatch.com
Other
5 stars 1 forks source link

Avoid shadows and usage of outer variable in callback functions #54

Closed carlos-montiers closed 4 years ago

carlos-montiers commented 4 years ago

Please, this commit is fine?

adoxa commented 4 years ago

I don't really like it - it rather defeats the purpose of having local functions. And these ones aren't callbacks.

What's the point of converting graphics to g? That's not shadowed by anything.

carlos-montiers commented 4 years ago

And these ones aren't callbacks.

Oops, really are local variables that hold a function.

That's not shadowed by anything.

I explain with images image

image

Because that I think is better the local variable has other different names than the used in outer. And also that the local functions get all the variables from the out by parameters.

adoxa commented 4 years ago

:blush: I didn't even see that graphics one. I solved it by simply moving the top one to after the loop. I renamed c to cr. I'm not going to do the parameters, as I say that defeats the purpose (what about mdc and pcfi? And r, g, b, h, s, v? Actually those should probably be local to the inner function, as cr & hh et al are, or move those out, too).

carlos-montiers commented 4 years ago

I solved it by simply moving the top one to after the loop.

Nice solution, now it is perfect.

Actually those should probably be local to the inner function, as cr &hh et al are, or move those out, too).

I would like that those be local it is not needed it be outside. i and j are shared between average_rgb and average_hsv, but those variables not.

What about moving the declaration of i and j to 0, to the portion of code where are declared: r, g, b, h, s, v? And initialize i and j to 0, instead of in the for? To make average_rgb and average_hsv looks like not using uninitialized variables. Also seems the declaration of those function, have the purpose of being helpers to the 'for' statement. Thus, is as part of the declaration of all what the 'for' needs, because it, I think that initialize i and j outside the 'for' will look better.

adoxa commented 4 years ago

Okay, done. In future you might want to just do it, as I'm resistant to changing it, but I'm not going to undo it.