search

bytefish / FcmSharp

Firebase Cloud Messaging (FCM) with .NET
MIT License
6 stars 7 forks source link

Added a stream based way to load credentials #20

Closed janniksam closed 6 years ago

janniksam commented 6 years ago

I built it for my own purpose (I save the json in my embedded resources). Could be useful for other people aswell.

bytefish commented 6 years ago

@janniksam Thanks for your Pull Request! I will see what's the best way to integrate it. 😎

janniksam commented 6 years ago

It's my first pull request, so please have mercy with me :)

bytefish commented 6 years ago

No worries! Good work. 😎

bytefish commented 6 years ago

I just thought yesterday: There are probably a million more ways to read the Settings. And this "FileBased..." is too long for a human brain to remember. 😅 I would have to Google for an example each time I use the library. An extension method doesn't fit. Maybe something like a SettingsUtils class with the static methods for reading from File or Stream.

bytefish commented 6 years ago

@janniksam What do you think would be the most intuitive way? Good API Design is hard. 🤔

janniksam commented 6 years ago

Unfortunately I am not that good at designing, I always have a bad feeling when it comes to that topic.

But let me try anyways: Just like you said, SettingsUtils sounds fine too me. But I don't like "Utils". "Utils" is like "Manager". It sounds like a class that tends to do alot of things instead of having one single purpose. In my opinion It breaks the principle "seperation of concerns". Maybe "Creator" would fit in this case.

One thing I saw in your code is, that your class is namemd FileBased.... and the method inside is called ReadFromFile or something like this. It's a personal preference, but I don't like that, I think it is redundant.

So, what you should do (imho):

And last but not least,- I think it's self explanatory:

Sorry I didn't mean to write that much. ;)

bytefish commented 6 years ago

Thanks for your opinion! It's too late to remove the class anyway. 🤣 Deprecating it is a good idea for a soft upgrade path. Yes I didn't like this design, either... but then it was commited and too late. 😳 Creator sounds good, I will add this and deprecate the other method.