patternjavaMinor
Helper function to return a string based on passed object and some defined rules
Viewed 0 times
helperreturnpassedfunctionsomebasedandrulesobjectstring
Problem
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
How can I refactor or improve this piece of code?
Any feedback will be appreciated.
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 = " sent you a direct "
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 = " sent you a message"
message = String.format(Resources.string(R.string.text_new_message), chat.getUserInfo().getDisplayName());
}
}
return message;
}Solution
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
Code Snippets
boolean isUpdateMessage = (chat.getTag().equals(TAG_UPDATE_MSG));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);boolean isDirectMessage = (chat.getType() == DIRECT_MESSAGE);
boolean isTextMessage = (chat.getTag().equals(TAG_TEXT_MSG));Context
StackExchange Code Review Q#51368, answer score: 4
Revisions (0)
No revisions yet.