Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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);
        }
    }
share|improve this question
14  
What language, its kind of important at this level of detail. –  mattnz Jun 13 at 9:16
3  
@mattnz anyone familiar with Java will recognise it in the code sample he provides. –  jwenting Jun 13 at 10:06
6  
@jwenting: It's also valid C# syntax, and I bet there are more languages. –  phresnel Jun 13 at 12:13
4  
@phresnel yes, but do those have the exact same standard API for String? –  jwenting Jun 13 at 12:44
3  
@jwenting: Is it relevant? But even if: One can construct valid examples, like for example a Java/C#-Interop Helper Library, or have a look at Java for .net: ikvm.net . The language is always relevant. The questioner may not be looking for specific languages, he/she may have committed syntax errors (accidentally converting Java to C#, e.g.), new languages may arise (or have risen out the in the wild, where there be dragons) -- edit: My previous comments were to dicky, sorry. –  phresnel Jun 13 at 13:24
show 5 more comments

7 Answers 7

up vote 43 down vote accepted

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.

share|improve this answer
    
Can combine that with the command pattern mentioned by others to nicely encapsulate the actual work being done, and maybe use a database or other external resource to configure the table. –  jwenting Jun 13 at 10:10
4  
The problem with this, though, is that you won't be able to control the matching mechanism. In OP's example, he uses equalsIgnoreCase for "now playing" but toLowerCase().startsWith for the others. –  mrjink Jun 13 at 10:36
5  
@mrjink: I don't see this as a "problem", it is only a different approach with different pros and cons. The "pro" of your solution: individual Commands can have individual matching mechanisms. The "pro" of mine: the Commands do not have to provide their own matching mechanism. The OP has to decide which solution suits him best. By the way, I also upvoted your answer. –  Doc Brown Jun 13 at 11:36
1  
FYI - "Pointer to function" is a C# language feature called delegates. Lambda is more of an expression object that can be passed around - you can't "call" a lambda like you can call a delegate. –  user1068 Jun 13 at 16:07
1  
@HarrisonNguyen: I recommend docs.oracle.com/javase/tutorial/java/javaOO/… . But if you are not using Java 8, you can just use mrink's interface definition without the "matches" part, that's 100% equivalent (that is what I meant by "simulate pointer-to-function by using the command pattern). And user1068's comment is just a remark about the different terms for different variants of the same concept in different programming languages. –  Doc Brown Jun 14 at 7:58
show 6 more comments

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
    }
}
share|improve this answer
    
If messages never need to be parsed elsewhere (I assumed that in my answer) this is indeed preferable to my solution for Command is more self-contained if it knows when to be called itself. It does produce a slight overhead if the list of commands is huge but that's probably negligible. –  jhr Jun 13 at 7:40
1  
This pattern tends to fit the console command paradygm nicely, but only because it neatly separates the command logic from the event bus and because new commands are likely to be added in the future. I think it should be noted that this isn't a solution for any circumstance where you have a long chain of if...elseif –  Neil Jun 13 at 9:10
    
+1 for using a "light" Command pattern! It should be noted that when you're dealing with non-Strings you can make use of e.g. intelligent enums that know how to execute their logic. This even saves you the for-loop. –  LastFreeNickname Jun 13 at 9:15
2  
Rather than the loop, we could use this as a map if you make Command an abstract class that overrides equals and hashCode to be the same as the string that represents the command –  Cruncher Jun 13 at 19:33
    
This is awesome and easy to understand. Thanks for the suggestion. It's pretty much exactly what I was looking for and seems to be manageable for future addition of commands. –  Harrison Nguyen Jun 15 at 22:29
add comment

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 ArrayList of Methods from all annotated Methods in your class:

    ...
    for (Method m : getDeclaredMethods()) {
    if ( isAnnotationPresent... ) {
        commandMap.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 ( ( a.perfectMatch && cmd.equalsIgnoreCase( a.command ) )
             || cmd.startsWith( a.command ) {
            commandMap.get( a ).invoke( this, cmd );
    
share|improve this answer
    
It's not clear that he's using Java, though that is an elegant solution. –  Neil Jun 13 at 9:21
    
You're right - the code just looked so much like eclipse-auto-formatted Java-Code... But you could do the same with C# and with C++ you could simulate annotations with some clever Macros –  Falco Jun 13 at 9:29
    
It could very well be Java, however in the future, I suggest that you avoid language-specific solutions if language is not clearly indicated. Just friendly advice. –  Neil Jun 13 at 9:48
    
Thanks for this solution - you were right that I'm using Java in the code provided even though I didn't specify, this look really nice and I'll be sure to give it a shot. Sorry I can't pick two best answers! –  Harrison Nguyen Jun 13 at 21:42
add comment

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.

share|improve this answer
1  
Where did the ifs go? I.e., how do you decide a behavior is executed for a command? –  mrjink Jun 13 at 7:14
2  
@mrjink: Sorry my bad. The decision of whether to execute or not is delegated to the behaviour (I mistakenly omitted the if part in the behaviour). –  npinti Jun 13 at 7:38
    
I think I prefer the check about whether a given IChatBehaviour can handle a given command, since it allows the caller to do more with it, like processing errors if no command matches, though it is really only a personal preference. If that isn't needed, then no point needlessly complicating code. –  Neil Jun 13 at 9:13
    
you'd still need a way to generate the instance of the correct class in order to execute it, which would still have the same long chain of ifs or massive switch statement... –  jwenting Jun 13 at 10:08
add comment

"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.

share|improve this answer
add comment

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.)

share|improve this answer
    
I think this is good for decoupling and program organization, though I don't think this directly addresses the problem with having too many if...elseif statements. –  Neil Jun 13 at 9:18
add comment

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.

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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