D17002: Refactor Duplicated Code in KDE Connect Android-side SMS Helper

[Simon Redman]

sredman added a comment.


  This is good work! Thank you. Were you able to get the app with this patch working for your own phone? If not, could you share the full stack trace (paste.kde.org) so I can have a look?
  Don't forget to mark the GCi task as completed so I can approve it once we get this merged.

INLINE COMMENTS

> SMSHelper.java:122-123
> +     * @param context android.content.Context running the request.
> +     * @param selection Parameterizable filter to use with the ContentResolver query. May be null with mode = GetMessagesMode.CONVERSATIONS.
> +     * @param selectionArgs Parameters for selection. May be null with mode = GetMessagesMode.CO NVERSATIONS.
> +     * @return Returns HashMap<ThreadID, List<Message>>, which is transformed in caller functions into other classes.

I missed this before (sorry). These comments still refer to one of the earlier revisions

> SMSHelper.java:141-142
> +                do {
> +                    int thread = myCursor.getInt(threadColumn);
> +
> +                    HashMap<String, String> messageInfo = new HashMap<>();

This variable is unused because we pull threadID from the Message later, so might as well get rid of it

> SMSHelper.java:152-158
> +                    if(toReturn.get(threadID) == null) {
> +                        List<Message> messageList = new ArrayList<>();
> +                        messageList.add(message);
> +                        toReturn.put(threadID, messageList);
> +                    }
> +                    else
> +                        toReturn.get(threadID).add(message);

A common way to do this same check is:

  if (!toReturn.containsKey(threadID)) {
    toReturn.put(threadID, new ArrayList<Message>());
  }
  toReturn.get(threadID).add(message);

The code you currently have is fine, and if you like it this way then you should leave it this way. I only mention this because you are working on GCi, so I should share my wisdom :)

> SMSHelper.java:157-158
> +                    }
> +                    else
> +                        toReturn.get(threadID).add(message);
> +                } while (myCursor.moveToNext());

Even though you //can// have an `else` with no braces, I strongly recommend against it. The problem is that a future programmer might come along and try to add to this block (maybe he is more experienced with Python than C++) and the code will not get executed where he expects it to. Moreover, there is a possibility that could get get inserted by mistake, for instance by a `git merge`
For a real-world example of where this resulted in a serious disaster, see this article: https://nakedsecurity.sophos.com/2014/02/24/anatomy-of-a-goto-fail-apples-ssl-bug-explained-plus-an-unofficial-patch/

REPOSITORY
  R225 KDE Connect - Android application

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

To: alexkovrigin, sredman
Cc: sredman, alexkovrigin, kdeconnect, shivanshukantprasad, skymoore, wistak, dvalencia, rmenezes, julioc, Leptopoda, timothyc, jdvr, Danial0_0, johnq, Pitel, adeen-s, SemperPeritus, daniel.z.tg, jeanv, seebauer, bugzy, MayeulC, menasshock, mikesomov, tctara, apol
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdeconnect/attachments/20181119/d3dc389f/attachment-0001.html>