<table><tr><td style="">nicolasfella added a comment.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D11854">View Revision</a></tr></table><br /><div><div><p>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.</p></div></div><br /><div><strong>REPOSITORY</strong><div><div>R224 KDE Connect</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D11854">https://phabricator.kde.org/D11854</a></div></div><br /><div><strong>To: </strong>sredman, KDE Connect, nicolasfella<br /><strong>Cc: </strong>nicolasfella, KDE Connect, Danial0_0, krantideep95, Pitel, adeen-s, SemperPeritus, ahmedbesbes, daniel.z.tg, jeanv, seebauer, bugzy, MayeulC, menasshock, ach, apol<br /></div>