twilio / twilio-chat-demo-android

Chat API Demo Application for Android
MIT License
62 stars 51 forks source link

Question: Why Attributes has such API? #135

Closed emartynov closed 4 years ago

emartynov commented 4 years ago

I understand that Attributes class was needed as abstraction and type safety for custom data attached to Channel, User, etc. I understand why Attributes has type and getter that returns value for the specific type. I also understand that json serialization is natural for this class.

I don't understand why there is no methods like getAttribute(String key) for complex object. Or no method getAttribute(int index) when it is array.

I also don't understand why there are methods that returns JSONArray and JSONObject. Especially , many of us don't work with it directly or indirectly.

I also don't understand why it doesn't crash if I try to use operation that is not allowed by type, like getNumber when it is a string. The null is returned instead.

And last, I don't understand why JSONException is thrown on the parse method. It exposes details of the Attributes class implementation.

rusmonster commented 4 years ago

Hi @emartynov!

The idea of attributes was to provide same data format across different platforms. The JSON format is good enough for that as all platforms we are supporting have some native JSON support. So the format is more or less familiar to all developers on that platforms.

Unfortunately Android/Java platform doesn't provide a way to represent any JSON object which valid according to the standart as a single type-safe object. So on android, in order to have such single object, we just add minimal wrapper around regular java org.json.* classes which we assumed familiar to most android/java developers. We had no goal to hide implementation details here. The Attributes is just JSON object. Nothing more. We didn't try to introduce one more abstraction level here.

We believe that it is enough for trivial cases and for more complex cases android platform has powerful libraries such as Gson which we are definitely not going to reinvent. Instead we recommend to use such libraries if you feel that features provided by org.json.* + Attributes is not enough for your case.

Regarding getNumber() returning null. Both approaches are possible here with our without throwing an exception. I don't think one much better than another. When getNumber() return null it means This Attributes object doesn't contain a Number. The regular java HashMap behaves similar way. So I think this is OK for the platform.

I hope I answered why it throws the JSONException, but in regular cases when you use SDK on both ends you should never get it in real life. So please report an issue with your scenario and logs if it throws in your case.

emartynov commented 4 years ago

Follow up questions:

  1. Why not to use Java/Kotlin has Map<String, Any>?
  2. Why to have wrapper at all and just don't give raw JSONObject instead?

I still prefer fail fast approach when you don't cover parsing or developer usage mistakes. But your point taken.

Gray-Wind commented 4 years ago

Because it is not necessarily JSON Object, it could be an Array, a String, a Number or even a Boolean. Take a look back to reference.