codebude / QRCoder

A pure C# Open Source QR Code implementation
MIT License
4.46k stars 1.09k forks source link

Naming consideration (capitalization) #53

Closed epsitec closed 6 years ago

epsitec commented 7 years ago

I've integrated QRCoder into our project and in a few minutes I was up and running with QR codes. You've done a great job, thank you!

I felt immediately at home: I was expecting the two-letter QR acronym to stay as a two-letter upper case name in the source code, as recommended by the Framework Design Guidelines summarized here.

There are, however, a few small glitches where the naming is not OK:

var generator = new QRCoder.QRCodeGenerator ();
var data = generator.CreateQrCode (value, QRCoder.QRCodeGenerator.ECCLevel.Q);
var code = new QRCoder.QRCode (data);
var bitmap = code.GetGraphic (8);

There may be other such glitches in the code base, I did not check.

What do you think? Would it be OK for you to introduce breaking changes to make the library even easier to consume, by removing these small surprises?

codebude commented 7 years ago

Hi @epsitec ,

thanks for your feedback. In theory this is a good and justifiable proposal. Nevertheless I'm a bit scared of the user's/developer's reactions, when they have to rewrite their code after the next update.

Do you have a strategy or idea in mind, how to switch "softly"? May be we can switch by giving support for old an new naming for a while. (While marking the old named functions as obsolete like explained here: http://stackoverflow.com/a/1759357/251719)

epsitec commented 7 years ago

@codebude, sorry for the long silence - I somehow lost track of this issue.

I don't see any other solution than duplicating types to keep current implementations from compiling after such a major change. And who knows if type and enum names have not been persisted under some form or another, and what it would mean to break deserialization...

Obsoleting the misnamed types and methods will certainly be useful, until the next major release where such a braking change would be acceptable.

ghost commented 7 years ago

Doing it all at once might be better that way it is just done otherwise it is just delaying the issue

tilkinsc commented 7 years ago

I think a massive code go-through can be justifiable.

codebude commented 7 years ago

Hi @epsitec , @Mrtops and @Hydroque ,

since you guys argue for renaming, let's try to do so. Before doing such breaking changes, I want to ensure, that we have to do this only one time. So can we bring up a list with all classes/methods/variables which shall be changed together? I just want to ensure that nothing is left behind.

File Type Old New
AbstractQRCode.cs property QrCodeData QRCodeData
PayloadGenerator.cs method isHexStyle IsHexStyle
PayloadGenerator.cs enum-value (CalendarEvent) iCalComplete ICalComplete
PayloadGenerator.cs enum-value (GeolocationEncoding) GEO Geo
PayloadGenerator.cs enum-value (MailEncoding) MAILTO MailTo
PayloadGenerator.cs enum-value (MailEncoding) MATMSG MatMsg
PayloadGenerator.cs enum-value (MailEncoding) SMTP Smtp
PayloadGenerator.cs class MMS Mms
PayloadGenerator.cs enum MMSEncoding MmsEncoding
PayloadGenerator.cs enum-value (MmsEncoding) MMS Mms
PayloadGenerator.cs enum-value (MmsEncoding) MMSTO MmsTo
PayloadGenerator.cs method HMACToString HmacToString
PayloadGenerator.cs enum-value (OneTimePasswordAuthType) HOTP Hotp
PayloadGenerator.cs enum-value (OneTimePasswordAuthType) TOTP Totp
PayloadGenerator.cs enum OoneTimePasswordAuthAlgorithm OneTimePasswordAuthAlgorithm
PayloadGenerator.cs enum-value (OneTimePasswordAuthAlgorithm) SHA1 Sha1
PayloadGenerator.cs enum-value (OneTimePasswordAuthAlgorithm) SHA256 Sha256
PayloadGenerator.cs enum-value (OneTimePasswordAuthAlgorithm) SHA512 Sha512
PayloadGenerator.cs class SMS Sms
PayloadGenerator.cs enum SMSEncoding SmsEncoding
PayloadGenerator.cs enum-value (SmsEncoding) SMS Sms
PayloadGenerator.cs enum-value (SmsEncoding) SMS_iOS SmsIos
PayloadGenerator.cs enum-value (SmsEncoding) SMSTO SmsTo
PayloadGenerator.cs class SwissQrCode SwissQRCode
PayloadGenerator.cs exception-type SwissQrCodeException SwissQRCodeException
PayloadGenerator.cs enum-value (SwissQRCode.Currency) CHF Chf
PayloadGenerator.cs enum-value (SwissQRCode.Currency) EUR Eur
PayloadGenerator.cs method (SwissQRCode.Iban) IsQrIban IsQRIban
PayloadGenerator.cs enum-value (SwissQRCode.IbanType) QrIban QRIban
PayloadGenerator.cs exception-type SwissQrCodeIbanException SwissQRCodeIbanException
PayloadGenerator.cs enum-value (SwissQRCode.ReferenceTextType) QrReference QRReference
PayloadGenerator.cs enum-value (SwissQRCode.ReferenceType) NON Non
PayloadGenerator.cs enum-value (SwissQRCode.ReferenceType) QRR Qrr
PayloadGenerator.cs enum-value (SwissQRCode.ReferenceType) SCOR Scor
PayloadGenerator.cs exception-type SwissQrCodeReferenceException SwissQRCodeReferenceException
PayloadGenerator.cs exception-type SwissQrCodeContactException SwissQRCodeContactException
PayloadGenerator.cs enum-value (WiFi.Authentication) nopass NoPass
PayloadGenerator.cs enum-value (WiFi.Authentication) WEP Wep
PayloadGenerator.cs enum-value (WiFi.Authentication) WPA Wpa
QRCodeGenerator.cs method CalculateECCWords CalculateEccWords
QRCodeGenerator.cs method CreateCapacityECCTable CreateCapacityEccTable
QRCodeGenerator.cs method CreateQrCode CreateQRCode
QRCodeGenerator.cs method IsValidISO IsValidIso
QRCodeGenerator.cs method XORPolynoms XorPolynoms
QRCodeGenerator.cs enum ECCLevel EccLevel

Please add, edit, remove, proof and discuss this list. 👍

epsitec commented 7 years ago

Thank you for your proposal. It seems that Microsoft has some difficulty at adhering to their own coding standards. See for instance what they did for SHA or HMAC...

https://msdn.microsoft.com/en-us/library/system.security.cryptography.hmac(v=vs.110).aspx

So, switching all acronyms to the three letter words is probably not the path to least surprise for users of Cryptography namespace.

Now I am confused.

codebude commented 7 years ago

Hi @epsitec , that's why I initially said, that I don't see a big "benefit". Yes, coding standards are good and everyone should try to met the standards, but it's not possible all the time.

I'm still scared, that we could put happy users off, by doing this breaking changes. On the other hand you and the others said, that the changes would make the lib more intuitive to use. But spoken for myself, when I start using a new lib, I don't know how the methods/properties are named. I have to use Intellisense/autocomplete and/or the wiki/docs to learn what methods are available and how to use them. So does renaming methods really bring the effort, when have to use docs/wiki anyway?

epsitec commented 7 years ago

You might be right, after all. Removing the surprise of the Qr spelling by breaking compatibility is probably not worth it.

tilkinsc commented 7 years ago

My intent for proposal would be looking through all the code for formatting and regulation. Not just simple name changes, but optimizations and code flow. Making a new release doesn't mean that people can't rely on the previous instance if they want. They will come to want the more additional features in the mean time. As long as the dev has good documentation over what has changed, it shouldn't be that hard to figure out.

Also, I prefer my acronyms capitalized, and leading WORDS to be lowercase. Example QRCoderExample and WTFToString

codebude commented 6 years ago

Postponed. Maybe implemented in the next major release.