5
\$\begingroup\$

In my application, a notification should be displayed when a chat message arrives.
There are some rules which decide what notification message is to be displayed.

I have created a helper method which takes a ChatMessage object and returns the message to be displayed in the notification.

How can I refactor or improve this piece of code?
Any feedback will be appreciated.

private String getNotificationDisplayString(ChatMessage chat) {
    String message;
    boolean isUpdateMessage = (chat.getTag().equals(TAG_UPDATE_MSG));
    if (isUpdateMessage) {
        // for update message
        UserRelationshipInfo relationshipInfo = UserRelationshipInfoManger.getInstance().getInfo(chat.getUserId());
        boolean isUserMyFriend = (relationshipInfo != null && relationshipInfo.getSource() == SOURCE_FRIEND);
        message = Resources.string(isUserMyFriend ? R.string.text_user_is_friend : R.string.text_user_is_stranger);
    } else {
        boolean isDirectMessage = (chat.getType() == DIRECT_MESSAGE);
        boolean isTextMessage = (chat.getTag().equals(TAG_TEXT_MSG));

        if (isDirectMessage) {
            // for direct message (special case)
            String messageName = Resources.string(getMessageType(chat.getTag()));
            // message = "<name> sent you a direct <image/video/etc>"
            message = String.format(Resources.string(R.string.text_new_direct_message), chat.getUserInfo().getDisplayName(), messageName);
        } else if (isTextMessage) {
            // for text (non-direct)
            message = new TextInfo(chat).getText();
        } else {
            // for non-text (non-direct)
            // message = "<name> sent you a message"
            message = String.format(Resources.string(R.string.text_new_message), chat.getUserInfo().getDisplayName());
        }
    }
    return message;
}
\$\endgroup\$

4 Answers 4

4
\$\begingroup\$
boolean isUpdateMessage = (chat.getTag().equals(TAG_UPDATE_MSG));

Consider using a chat.isUpdateMessage() method for that logic.


UserRelationshipInfo relationshipInfo = UserRelationshipInfoManger.getInstance().getInfo(chat.getUserId());

Singleton pattern detected! I would not recommend using the singleton pattern, I am quite certain that there's a better way to do this, as I don't know the entire context of your code I am not sure which method would be the best, but to get you started take a look at a previous answer of mine.


boolean isUserMyFriend = (relationshipInfo != null && relationshipInfo.getSource() == SOURCE_FRIEND);

Again, consider using as method such as relationshipInfo.isMyFriend()


message = Resources.string(isUserMyFriend ? R.string.text_user_is_friend : R.string.text_user_is_stranger);

Again, this seems like you have a Context stored in a static variable somewhere. I wouldn't recommend that. Tell, don't ask. If something needs your context, tell them about it. Don't have them ask some other object/place about it.


boolean isDirectMessage = (chat.getType() == DIRECT_MESSAGE);
boolean isTextMessage = (chat.getTag().equals(TAG_TEXT_MSG));

Two methods: chat.isDirectMessage() and chat.isTextMessage().


String messageName = Resources.string(getMessageType(chat.getTag()));

getMessageType should be a method in the ChatMessage class. chat.getMessageType(). And again, I'm not a fan of the Resources.string method as it tells me you got a static Context object somewhere.


message = String.format(Resources.string(R.string.text_new_message), chat.getUserInfo().getDisplayName());

When using Android resources, there's a special method for combining the getString with String.format. See this method

\$\endgroup\$
3
\$\begingroup\$

Some simple pointers...

  • You have boolean assignments that are only used once, for conciseness it may be preferable to inline them:

    // old
    boolean isUpdateMessage = (chat.getTag().equals(TAG_UPDATE_MSG));
    if (isUpdateMessage) 
    
    // new
    if (chat.getTag().equals(TAG_UPDATE_MSG))
    
  • Not sure if the getter methods on your ChatMessage class can return nulls or not, but in the unlikely event they do, you may want to practice some defensive coding by switching the .equals() around, e.g. if (TAG_UPDATE_MSG.equals(chat.getTag()))
  • Your comments in the code code snippet are redundant, can consider removing them too for conciseness too.

edit: (prompted by OP's first comment)

Wells, this shouldn't be recommended on CR but just for fun, I tried golfing your method and reduced it to a few ternary operators as a 'one-liner':

return chat.getTag().equals(TAG_UPDATE_MSG) ? 
    Resources.string(UserRelationshipInfoManger.getInstance().getInfo(chat.getUserId()).getSource() == SOURCE_FRIEND ? 
        R.string.text_user_is_friend : 
        R.string.text_user_is_stranger) : 
    chat.getTag().equals(TAG_TEXT_MSG) ? 
        new TextInfo(chat).getText() : 
        String.format(Resources.string(chat.getType() == DIRECT_MESSAGE ? 
            R.string.text_new_direct_message : 
            R.string.text_new_message
        ), chat.getUserInfo().getDisplayName(), Resources.string(getMessageType(chat.getTag())));

The only assumption I made is that the relationshipInfo will not return null, so I can eliminate the null-check when assigning isUserMyFriend. Once I got to this golfed version three minor improvements become clearer:

  1. You can consider a supplementary method isUserMyFriend() in the UserRelationshipInfoManger instance to give you a boolean value for checking if the user is a friend or not.
  2. You can consider a supplementary method getUserDisplayName() in the ChatMessage class that simply calls getUserInfo().getDisplayName().
  3. You are using chat.getTag() in a few places, maybe assign a variable to it for compactness?

By implementing these three suggestions, I got down to:

T tag = chat.getTag(); // whatever the actual Type is
return tag.equals(TAG_UPDATE_MSG) ? 
    Resources.string(UserRelationshipInfoManager.getInstance().isUserMyFriend(chat.getUserId()) ? 
        R.string.text_user_is_friend : 
        R.string.text_user_is_stranger) : 
    tag.equals(TAG_TEXT_MSG) ? 
        new TextInfo(chat).getText() : 
        String.format(Resources.string(chat.getType() == DIRECT_MESSAGE ? 
            R.string.text_new_direct_message : 
            R.string.text_new_message
        ), chat.getUserDisplayName(), Resources.string(getMessageType(tag)));

I must admit, this is taking things a little to the extreme, and I'm conveniently relying on the assumption that passing extra arguments into String.format() will be silently ignored (tested). Otherwise, I'm actually pretty OK with what you have in your question. :)

\$\endgroup\$
1
  • 2
    \$\begingroup\$ I have to veto the boolean inlining :) isUpdateMessage is a lot easier/faster to understand than (chat.getTag().equals(TAG_UPDATE_MSG)). But using this technique AND comments is redundant. \$\endgroup\$ May 22, 2014 at 5:12
3
\$\begingroup\$

There are some Suggestions regarding code :
1. Always initialise local variable (some default value or null)

String message = null;

2. Don't use unnecessary else if you can use else if instead

if (isUpdateMessage) {...
   } else{
     boolean isDirectMessage = (chat.getType() == DIRECT_MESSAGE);
        boolean isTextMessage = (chat.getTag().equals(TAG_TEXT_MSG));

        if (isDirectMessage) {...
       } else if(...

You can use else if at very first else

if (isUpdateMessage) {...

   }      
   if (chat.getType() == DIRECT_MESSAGE) {...

   } else if(...

3. Don't create new boolean if not getting use at more than one place and remove unnecessary parenthesis ( brackets).

boolean isDirectMessage = (chat.getType() == DIRECT_MESSAGE);
                          ^... two brackets not required...^

Here boolean is not required to create as we can use condition directly in if part.

4. And always call .equals() on constant, so that if other string (chat.getTag() in this case) is null then there will not be any NullPointerException

`chat.getTag().equals(TAG_TEXT_MSG)` should be `TAG_TEXT_MSG.equals(chat.getTag())`

Please find below method code after incorporating comments :

private String getNotificationDisplayString(ChatMessage chat) {
    String message = null;// initialise local variable to null 
    // check if updated message directly and isUpdateMessage removed
    if (TAG_UPDATE_MSG.equals(chat.getTag())) {
        // for update message
        UserRelationshipInfo relationshipInfo = UserRelationshipInfoManger.getInstance().getInfo(chat.getUserId());
        boolean isUserMyFriend = (relationshipInfo != null && relationshipInfo.getSource() == SOURCE_FRIEND);
        message = Resources.string(isUserMyFriend ? R.string.text_user_is_friend : R.string.text_user_is_stranger);
    }else if (chat.getType() == DIRECT_MESSAGE) {
            // for direct message (special case)
            String messageName = Resources.string(getMessageType(chat.getTag()));
            // message = "<name> sent you a direct <image/video/etc>"
            message = String.format(Resources.string(R.string.text_new_direct_message), chat.getUserInfo().getDisplayName(), messageName);
    } else if (TAG_TEXT_MSG.equals(chat.getTag())) {
            // for text (non-direct)
            message = new TextInfo(chat).getText();         
    } else {
            // for non-text (non-direct)
            // message = "<name> sent you a message"
            message = String.format(Resources.string(R.string.text_new_message), chat.getUserInfo().getDisplayName());
    }

    return message;
}
\$\endgroup\$
2
  • \$\begingroup\$ Rather than initializing Strings to null, I found it better and less Exception happy to initialize them to "" \$\endgroup\$
    – Vogel612
    May 22, 2014 at 7:26
  • \$\begingroup\$ Always initialise local variable (some default value or null) - I disagree with that. In fact, I find it better to do the opposite. Don't initialize it unless you need to. IDEs will show a warning on your initialization stating that the assignment is not used, because it will get overridden by the next assignment. And avoiding initial initialization might allow you to have the variable final which has never hurt anyone. \$\endgroup\$ May 22, 2014 at 10:11
2
\$\begingroup\$

Same as h.j.k that you don't have to store the booleans for an if.

Return directly in stead of saving into message.

Result is like this :

private String getNotificationDisplayString(ChatMessage chat) {
    if (chat.getTag().equals(TAG_UPDATE_MSG)) {
        // for update message
        UserRelationshipInfo relationshipInfo = UserRelationshipInfoManger.getInstance().getInfo(chat.getUserId());
        boolean isUserMyFriend = (relationshipInfo != null && relationshipInfo.getSource() == SOURCE_FRIEND);
        return Resources.string(isUserMyFriend ? R.string.text_user_is_friend : R.string.text_user_is_stranger);
    } else if (chat.getType() == DIRECT_MESSAGE) {
        // for direct message (special case)
        String messageName = Resources.string(getMessageType(chat.getTag()));
        // message = "<name> sent you a direct <image/video/etc>"
        return String.format(Resources.string(R.string.text_new_direct_message), chat.getUserInfo().getDisplayName(), messageName);
    } else if (chat.getTag().equals(TAG_TEXT_MSG)) {
        // for text (non-direct)
        return new TextInfo(chat).getText();
    } 
    // for non-text (non-direct)
    // message = "<name> sent you a message"
    return String.format(Resources.string(R.string.text_new_message), chat.getUserInfo().getDisplayName());
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.