Is there a more intelligent way to do this besides a long chain of if statements or switch?

Question

I'm implementing an IRC bot that receives a message and I'm checking that message to determine which functions to call. Is there a more clever way of doing this? It seems like it'd quickly get out of hand after I got up to like 20 commands.

Perhaps there's a better way to abstract this?

 public void onMessage(String channel, String sender, String login, String hostname, String message){

        if (message.equalsIgnoreCase(".np")){
//            TODO: Use Last.fm API to find the now playing
        } else if (message.toLowerCase().startsWith(".register")) {
                cmd.registerLastNick(channel, sender, message);
        } else if (message.toLowerCase().startsWith("give us a countdown")) {
                cmd.countdown(channel, message);
        } else if (message.toLowerCase().startsWith("remember am routine")) {
                cmd.updateAmRoutine(channel, message, sender);
        }
    }

Answer 1

Use a dispatch table. This is a table containing pairs ("message part", pointer-to-function). The dispatcher then will look like this (in pseudo code):

for each (row in dispatchTable)
{
    if(message.toLowerCase().startsWith(row.messagePart))
    {
         row.theFunction(message);
         break;
    }
}

(the equalsIgnoreCase can be handled as a special case somewhere before, or if you have many of those tests, with a second dispatch table).

Of course, what pointer-to-function has to look like depends on your programming language. Here is an example in C or C++. In Java or C# you will probably use lambda expressions for that purpose, or you simulate "pointer-to-functions" by using the command pattern. The free online book "Higher Order Perl" has a complete chapter about dispatch tables using Perl.

Answer 2

I'd probably do something like this:

public interface Command {
  boolean matches(String message);

  void execute(String channel, String sender, String login,
               String hostname, String message);
}

Then you can have every command implement this interface, and return true when it matches the message.

List<Command> activeCommands = new ArrayList<>();
activeCommands.add(new LastFMCommand());
activeCommands.add(new RegisterLastNickCommand());
// etc.

for (Command command : activeCommands) {
    if (command.matches(message)) {
        command.execute(channel, sender, login, hostname, message);
        break; // handle the first matching command only
    }
}

Answer 3

You are using Java - so make it beautiful ;-)

I would probably do this using Annotations:

1. Create a custom Method Annotation

   @IRCCommand( String command, boolean perfectmatch = false )
   

2. Add the Annotation to all relevant Methods in the Class e.g.

   @IRCCommand( command = ".np", perfectmatch = true )
   doNP( ... )
   

3. In your constructor use Reflections to create an HashMap of Methods from all annotated Methods in your class:

   ...
   for (Method m : getDeclaredMethods()) {
   if ( isAnnotationPresent... ) {
       commandList.put(m.getAnnotation(...), m);
       ...
   

4. In your onMessage Method, just do a loop over commandList trying to match the String on each one and calling method.invoke() where it fits.

   for ( @IRCCommand a : commanMap.keyList() ) {
       if ( cmd.equalsIgnoreCase( a.command )
            || ( cmd.startsWith( a.command ) && !a.perfectMatch ) {
           commandMap.get( a ).invoke( this, cmd );
   

Answer 4

What if you define an interface, say IChatBehaviour which has one method called Execute which takes in a message and a cmd object:

public Interface IChatBehaviour
{
    public void execute(String message, CMD cmd);
}

In your code, you then implement this interface and define the behaviours you want:

public class RegisterLastNick implements IChatBehaviour
{
    public void execute(String message, CMD cmd)
    {
        if (message.toLowerCase().startsWith(".register"))
        {
            cmd.registerLastNick(channel, sender, message);
        }
    }
}

And so on for the rest.

In your main class, you then have a list of behaviours (List<IChatBehaviour>)which your IRC bot implements. You could then replace your if statements with something like this:

for(IChatBehaviour behaviour : this.behaviours)
{
    behaviour.execute(message, cmd);
}

The above should reduce the amount of code you have. The above approach would also allow you to supply additional behaviours to your bot class without modifying the bot class itself (as per the Strategy Design Pattern).

If you want only one behaviour to fire at any one time, you can change the signature of the execute method to yield true (the behaviour has fired) or false (the behaviour did not fire) and replace the above loop with something like this:

for(IChatBehaviour behaviour : this.behaviours)
{
    if(behaviour.execute(message, cmd))
    { 
         break;
    }
}

The above would be more tedious to implement and initialize since you need to create and pass all the extra classes, however, it should make your bot easily extensible and modifiable since all your behaviour classes will be encapsulated and hopefully independent from each other.

Answer 5

"Intelligent" can be (at least) three things:

Higher Performance

The Dispatch Table (and its equivalents) suggestion is a good one. Such a table was called "CADET" in years past for "Can't Add; Doesn't Even Try." However, consider a comment to aid a novice maintainer on just how to manage said table.

Maintainability

"Make it beautiful" is no idle admonition.

and, often overlooked...

Resiliency

The use of toLowerCase has pitfalls in that some text in some languages must undergo painful restructuring when changing between magiscule and miniscule. Unfortunately, the same pitfalls exist for toUpperCase. Just be aware.

Answer 6

You could have all commands implement the same interface. Then a message parser could return you the appropriate command which you'll only execute.

public interface Command {
    public void execute(String channel, String message, String sender) throws Exception;
}

public class MessageParser {
    public Command parseCommandFromMessage(String message) {
        // TODO Put your if/switch or something more clever here
        // e.g. return new CountdownCommand();
    }
}

public class Whatever {
    public void onMessage(String channel, String sender, String login, String hostname, String message) {
        Command c = new MessageParser().parseCommandFromMessage(message);
        c.execute(channel, message, sender);
    }
}

It looks like just more code. Yes, you still need to parse the message in order to know which command to execute but now it's at a properly defined point. It may be reused elsewhere. (You might want to inject the MessageParser but that's another matter. Also, the Flyweight pattern might be a good idea for the commands, depending on how many you expect to be created.)

Answer 7

What I would do is this:

1. Group the commands you have into groups. (you have at least 20 right now)
2. At first level, categorize by group, so you have user name related command, song commands, count commands, etc.
3. Then you go in each group's method, this time you will get the original command.

This will make this more manageable. More benefit when the number of 'else if' grows too much.

Of course, sometimes having these 'if else' wouldn't be a big problem. I don't think 20 is that bad.