chillerlan / php-qrcode

A PHP QR Code generator and reader with a user-friendly API.
https://smiley.codes/qrcode/
Apache License 2.0
2.01k stars 302 forks source link

Added checkTypeIn function #138

Closed walidaalhomsi closed 2 years ago

walidaalhomsi commented 2 years ago

So it happens that checkTypes function is renamed recently to checkTypeNotIn. I used checkTypes in many places in my code.

I think it might be a good idea to have another variation of the function named checkTypeIn to follow the recent naming and to not confuse with checkTypes.

Thank you!

codemasher commented 2 years ago

Hmm, I changed the method to better reflect what it should do, i.e. check exclusion lists. I don't really see a reason to change it unless !$matrix->checkTypeNotIn($x, $y, $M_TYPES) produces unexpected results. Could you try and see if this works for you?

Edit: this was literally how the method was used before: https://github.com/chillerlan/php-qrcode/commit/1f435dfd54f7f26ff81a38f1de7ad5b147d05ac4

walidaalhomsi commented 2 years ago

Exactly, this is how it was working before, this is just another flavor of the same functionality.

!$matrix->checkTypeNotIn($x, $y, $M_TYPES) gives me the expected result. I changed checkTypeIn to just negate that function.

I think It makes sense when reading the code to understand what is really happening, instead of negating a negated function !checkTypeNotIn .

codemasher commented 2 years ago

See, if you now only negate the result of the other function, there's no reason to add it after all because you can just add the exclamation mark in your own code - without the overhead of another function call.

walidaalhomsi commented 2 years ago

It's all personal preference then, no worries, pulling back this ...

codemasher commented 1 year ago

Ok, i gave in and changed this in https://github.com/chillerlan/php-qrcode/commit/61128e4aec576087a4e122c771972a28beba3b0e - it is an eyesore still, but i'll admit that it makes more logical sense this way.