7
\$\begingroup\$

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.

\$\endgroup\$
  • 2
    \$\begingroup\$ Can I see your update-class, or if it is from Telegram, could you tell so? This could give me a lot better understanding of the code. (eg. does update and update.getCallbackQuery() have the same interface) \$\endgroup\$ – tieskedh Jun 22 at 11:00
  • 1
    \$\begingroup\$ Yup, Update comes from Telegram API. I dont want to write update.getMessage().getChatId() etc in every handler. And decided to create "API above API". \$\endgroup\$ – hsadik Jun 22 at 11:05
  • 2
    \$\begingroup\$ Welcome to Code Review! The current question title, which states your concerns about the code, is too general to be useful here. Please edit to the site standard, which is for the title to simply state the task accomplished by the code. Please see How to get the best value out of Code Review: Asking Questions for guidance on writing good question titles. \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Jun 23 at 21:47
10
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks for this interesting solution, tried to use it, but compiler said "Update is not a functional interface" on Method Reference. \$\endgroup\$ – hsadik Jun 22 at 11:32
  • 1
    \$\begingroup\$ @hsadik My bad, forgot to pass in update itself. See if the edited version works for you. \$\endgroup\$ – Mario Ishac Jun 22 at 11:34
  • 1
    \$\begingroup\$ Yup, after a little thought, I understood that I had to return 3 parametrs and fixed by myself, anyway thanks for fix. \$\endgroup\$ – hsadik Jun 22 at 11:37
15
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
5
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ getCallBackId() and getCallBackData() could be implemented either in an abstract common parent of UpdateData called UpdateDataBase or something similar, or alternatively right inside the UpdateData interface as default methods. \$\endgroup\$ – Hawk Aug 18 at 21:58
2
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ The main reason to write this "API above API" is convenience. I have about 15 additional handlers, and I don't want to write update.getMessage().getFrom().getUsername() or update.getCallbackQuery().getMessage().getData() every time, it's stupid. chatId and messageId always same for both handlers (callback and message), but you need to get them from different objects. \$\endgroup\$ – hsadik Jun 23 at 16:57
  • \$\begingroup\$ @hsadik: My point is that your code should have exactly one call to (e.g.) update.getMessage(), and it should be in the method I posted above (or something equivalent to it). All the code the needs to access properties of the message should be working with a Message object, not with an Update. And similarly, you should have only one call to update.getCallbackQuery(), like in my example code above, and all code that deals with callback queries should be working with CallbackQuery objects, not with Updates. \$\endgroup\$ – Ilmari Karonen Jun 23 at 16:59
  • \$\begingroup\$ … The abstraction level provided by the Update class is only good for one thing, and that thing is finding out what kind of an object is wrapped inside the update you just received and passing it to a handler written specifically for handling those objects. (If you do find that e.g. your Message and CallbackQuery handlers end up doing similar things, then you should certainly split those things into reusable methods that both handlers can call. But those methods still shouldn't be dealing with Update objects; they should be dealing with e.g. Messages and Chats and Strings and integers.) \$\endgroup\$ – Ilmari Karonen Jun 23 at 17:03

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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