If/else produce code duplication

Question

I have the following class:

public class UpdateData {

    public static boolean updateHasMessage(Update update) {
        return update.hasMessage() && update.getMessage().hasText();
    }

    public static boolean updateHasCallbackQuery(Update update) {
        return update.hasCallbackQuery() && (update.getCallbackQuery().getData()!= null);
    }

    public static Long getChatId(Update update) {
        if (updateHasMessage(update)) {
            return update.getMessage().getChatId();
        }
        else if (updateHasCallbackQuery(update)) {
            return update.getCallbackQuery().getMessage().getChatId();
        }
        else {
            return null;
        }
    }

    public static String getUserName(Update update) {
        if (updateHasMessage(update)) {
            return update.getMessage().getFrom().getUserName();
        }
        else if (updateHasCallbackQuery(update)){
            return update.getCallbackQuery().getFrom().getUserName();
        }
        else {
            return null;
        }
    }

    public static String getInputUserData(Update update) {
        if (updateHasMessage(update)) {
            return update.getMessage().getText();
        }
        else if (updateHasCallbackQuery(update)){
            return update.getCallbackQuery().getData();
        }
        else {
            return null;
        }
    }

    public static String getCallBackId(Update update) {
        return update.getCallbackQuery().getId();
    }

    public static Integer getMessageId(Update update) {
        if (updateHasMessage(update)) {
            return update.getMessage().getMessageId();
        }
        else if (updateHasCallbackQuery(update)){
            return update.getCallbackQuery().getMessage().getMessageId();
        }
        else {
            return null;
        }
    }

    public static String getCallBackData(Update update) {
        return update.getCallbackQuery().getData();
    }
}

As you can see I have similar block code in most methods:

if (updateHasMessage(update)) {
    ...
}
else if (updateHasCallbackQuery(update)) {
    ...
}
else {}

They return unique information and have various stuff inside, but this is very similar to code duplication. Do I need to rewrite this or just stay with it? If need to rewrite, then how?

UPDATE

Update is a Telegram API object.

Answer 1

Assuming Update#getMessage() is of type Message and Update#getCallbackQuery() is of type CallbackQuery:

You can make a function getUpdateAttribute that receives higher-order functions as handlers for the situation where your update has a message or callback query as follows.

private static <T> T getUpdateAttribute(Update update,
                                        Function<Message, T> messageFunc,
                                        Function<CallbackQuery, T> callbackQueryFunc) {
    if (updateHasMessage(update)) {
        return messageFunc.apply(update.getMessage());
    }
    if (updateHasCallbackQuery(update)) {
        return callbackQueryFunc.apply(update.getCallbackQuery());
    }

    return null;
}

I simplified the if/else code flow to take advantage of early returns. getUpdateAttribute is also generic over T because your attributes can be various types, like Integer and String. Finally, I made it private so your public attribute fetchers are the only interface exposed. Those attribute fetchers then become:

public static Long getChatId(Update update) {
    return getUpdateAttribute(
        update,
        Message::getChatId,
        callbackQuery -> callbackQuery.getMessage().getChatId()
    );
}

public static String getUserName(Update update) {
    return getUpdateAttribute(
        update,
        message -> message.getFrom().getUserName(),
        callbackQuery -> callbackQuery.getFrom().getUserName()
    );
}

public static String getInputUserData(Update update) {
    return getUpdateAttribute(
        update,
        Message::getText,
        CallbackQuery::getData
    );
}

public static Integer getMessageId(Update update) {
    return getUpdateAttribute(
        update,
        Message::getMessageId,
        callbackQuery -> callbackQuery.getMessage().getMessageId()
    );
}

Apart from this de-duplication, there is something else you can potentially improve. I'm not familiar with the Telegram API, but if it is possible that a message may have neither a message nor a callback query, then returning null seems like a valid response, in which case you may want to return Optional<T> from getUpdateAttribute (and all the other attribute fetchers), as that would signal your intent better.

Answer 2

Keep it like that. It is perfectly readable, easy to understand, everything is fine.

Naturally you could do some lambda trickery along the lines of:

public static String extractFromMessageOrQuery(
       Update update,
       Function<Update, String> messageExtractor,
       Function<Update, String> queryExtractor,
       String defaultValue) {
   if (updateHasMessage)
       return messageExtractor.apply(update);
   else if (updateHashCallbackQuery(update))
       return queryExtractor.apply(update);
   return defaultValue;
}

public static String getInputUserData(Update update) {
    return extractFromMessageOrQuery(
        upd -> upd.getMessage().getText(),
        upd -> upd.getCallbackQuery().getData(),
        null
    );
}

But: would that make the code

• easier to understand? No
• easier to extend? No
• easier to maintain? No

Thus: take note that these possibilities exist, and keep the code as-is.

Answer 3

The other solutions gave an answer which is perfect for two classes.
If you get to much functions//classes beside CallBack and Message, the code below might give you a better solution.

Wrapper

I personally would create a wrapper with a common interface.
The reason is that this is not very extendable (adding a new class requires rewriting all the common functions.)

class UpdateDataFactory{
    public UpdateData createUpdateData(Update update){
        if(isMessageUpdateData(update)){
            return new MessageUpdateData(update.getMessage());
        } else if(isCallBackUpdateData()){
            return new CallBackUpdateData(update.getCallBackQuery());
        } else return EmptyUpdateData.getInstance();
    }
    private boolean isMessageUpdateData(Update update){
        return update.hasMessage() && update.getMessage().hasText();
    }
    private boolean isCallBackUpdateData(){
        return update.hasCallbackQuery() 
             && (update.getCallbackQuery().getData()!= null);
    }
}

interface UpdateData{
    public User getFrom();
    public String getInputUserData();
    public Message getMessage();
}
public class MessageUpdateData implements UpdateData{...}
public class CallBackUpdateData implements UpdateData{...}
public class EmptyUpdateData implements UpdateData{
     static UpdateData getInstance(){...}
}

The one problem with this approach is that it requires you to create a new class for each object.

Extractors

Instead of creating a wrapper for an object, you could also create extractors for a type.

interface UpdateDataExtractor{
    public boolean canExtract(Update update);
    public User getFrom(Update update);
    public String getInputUserData(Update update);
    public Message getMessage(Update update);
}

 class MessageUpdateDataExtractor implements UpdateDataExtractor{
    public static MessageUpdateDataExtractor getInstance(){...}
    public boolean canExtract(Update update){
        return update.hasMessage() && update.getMessage().hasText();
    }
    ...
 }
 class CallBackUpdateData implements UpdateDataExtractor{
    public static CallBackUpdateDataExtractor getInstance(){...}
    public boolean canExtract(Update update){
        return update.hasMessage() && update.getMessage().hasText();
    }
    ...
 }

 class UpdateDataFactory{
    List<UpdateDataExtractor> extractors = new List<UpdateDataExtractor>();
    public UpdateDataExtractor createUpdateData(Update update){
        for(Extractor extractor : extractors)
            if(extractor.canExtract(update)) return extractor;
        return EmptyExtractor.getInstance();
    }
 }

If you don't like the canExtract and just want to store it in separate fields, you could use generics as parameters and make the code more typeSafe.

remaining functions

I don't exactly know how you should implement the following functions:

public static String getCallBackId(Update update) {
    return update.getCallbackQuery().getId();
}

public static String getCallBackData(Update update) {
    return update.getCallbackQuery().getData();
}

You could either implement them as all the other functions, or you could only implement them only in CallBackUpdateExtractor or CallBackUpdateData.

Answer 4

I suspect the root of your problem is that you're working at the wrong abstraction level.

Specifically, looking at the Telegram Bot API documentation, it seems that an Update is just a wrapper for one of several unrelated objects that describe different kinds of events that your application might want to react to. Trying to treat all of these events as if they were the same thing makes no sense — but that's what you're effectively doing in your code, with methods that take an Update object and try to return some property of whatever random event the update might contain. Some of those events, for example, might not even have an associated chat ID or a user name; and even if they do, the way that chat ID or user name is related to the event might not be the same.

Basically, the only part of your code that should be dealing with Update objects is the low-level code that receives then (from WebHooks or from getUpdates), possibly does reordering and/or de-duplication using the update IDs and then dispatches the event object contained in the Update wrapper to an appropriate handler for that kind of event.

That is to say, your code for handling updates should look something like this:

private void handleUpdate(Update update) {
    if (update.hasMessage()) {
        handleNewMessage(update.getMessage());
    } else if (update.hasCallbackQuery()) {
        handleCallbackQuery(update.getCallbackQuery());
    } else if (/* ... */) {
        // ...
    } else {
        // Either we received a truly empty update, or we don't know how to
        // handle its contents. Ignore it and maybe log a warning.
    }
}

…and then your handleNewMessage(Message message) and handleCallbackQuery(CallbackQuery query) methods (which will not have to deal with Update objects) should handle the messages or callback queries or whatever else they receive appropriately, using the methods provided by the objects they receive as their parameters.

If you want, you can of course have the different handlers share some code. For example, since a CallbackQuery can contain an optional Message, you might want to have your handleCallbackQuery() method internally call handleNewMessage() to deal with the message embedded in the query, if there is one — or, perhaps more practically, have both handler methods call other methods that implement the shared message-processing parts common to both of them.