D11854: (WIP) Upgrade Telephony plugin to read SMS history (KDE side)

[Simon Redman]

sredman marked an inline comment as done.
sredman added a comment.


  In D11854#238215 <https://phabricator.kde.org/D11854#238215>, @nicolasfella wrote:
  
  > Looking much better, but there is still room for improvement. Passing a QVariantMap in the constructor is IMHO a violation of the law of demeter. The constructor should not make assumptions/rely on the content of args. It should ask directly for what it wants, that is a String body and a String address. I assume you want to be extensible this way, but adding a third property, say an image that is being sent would still need changes in the class. That would be a violation of the open-closed principle, that states that a class should be closed for modification but open for extension. Taking the example of an image message: Instead of cramming all the logic into Message one should create a class ImageMessage that extends Message and takes the image as a third constructor argument.
  
  
  I don't disagree, but here is my thinking: Making a lot of individual arguments (this Message classs will need a few more, "type" at a minimum, and I like the concept of an ImageMessage) and passing them to the constructor offloads the logic of how a Message needs to be constructed from Message to the user. In other words, every place I want to construct a Message will have duplicate code. You're right that there is some worry about fragility by passing an unconstrained map of arguments.

REPOSITORY
  R224 KDE Connect

REVISION DETAIL
  https://phabricator.kde.org/D11854

To: sredman, #kde_connect, nicolasfella
Cc: nicolasfella, #kde_connect, Danial0_0, krantideep95, Pitel, adeen-s, SemperPeritus, ahmedbesbes, daniel.z.tg, jeanv, seebauer, bugzy, MayeulC, menasshock, ach, apol
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdeconnect/attachments/20180401/a10a8091/attachment-0001.html>