[sr-dev] [kamailio/kamailio] json_pua: new module (#1408)

[Daniel-Constantin Mierla]

Thanks, sounds interesting, but I have couple of observations:

  * the module should be named **pua_json** -- being related to presence services, we put first the services group token (see how are the other modules are named, like: presence_*, pua_*, db_*, ndb_*)
  * there are a lot of unsafe`sprintf()`, they should be replaced with `snprintf()` -- that will also give the len of the output string, `strlen()` no longer being needed
  * if I got it right, the module works directly with the database table `presentity` from presence modules, (re)defining variables with column names. This is not maintainable in long term, if someone changes the structure of the table in presence module, will have to change in other modules. I think it is better to add a function to presence module for what you need here and export it to the presence api, which in turn is going to be used in this module. The presence module is already exporting an API, so you can just add a new member there
  * for API functions, I suggest to define a structure instead of a long list of parameters, because if a new value needs to be inserted, the prototype of the API function stays unchanged, only a new member is added to the structure
  * also the way functions from json module are used inside this module are exposing a risk over the time. You declared them static inline, but if one needs to make them more complex in the future, with use of additional functions inside, then things are going to break. I suggest to do the same approach with exporting the functions in json module through an API, which is imported inside the new module

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/1408#issuecomment-360279115
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.kamailio.org/pipermail/sr-dev/attachments/20180124/a71d5741/attachment-0001.html>