Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have this method that basically takes a string and does different things depending on what the string starts with. Is there a cleaner way to write this method?

public String analyze(BotMessage message)
{

    String lcmsg = message.message().toLowerCase();
    //Not a command
    if(!lcmsg.startsWith("!")) return "-1";

    //Message from whatever
     if(lcmsg.startsWith("!ping")) return "pong!";
     if(lcmsg.startsWith("!tournament")) return tournament(message);


     //message from skype
     if(message.skype())
     {
         if(lcmsg.startsWith("!settournament")) return settournament(message);
         if(lcmsg.startsWith("!promote")) return promote(message);
         if(lcmsg.startsWith("!checkin")) return checkin(message);
         if(lcmsg.startsWith("!checkout")) return checkout(message);
         if(lcmsg.startsWith("!checkedin")) return checkedin(message);
         if(lcmsg.startsWith("!updatetournament")) return updatetournament();

     }

    return "-1";
}
share|improve this question
add comment

3 Answers

up vote 2 down vote accepted

I don't like too much the fact that you use a String as the returned type of your function.

What about using a class hierarchy or exceptions?

I see opportunities to create a hierarchy even for the messages. Why don't you create a BotMessage and a SkypeMessage?

Both should have the logic to decide what to do in the base case, maybe in an analyze method. The analyze method of the SkypeMessage should, in addition, manage the Skype-specific aspects.

Finally you should simplify the code and remove the list of if statements. Introduce a new set of objects to parse the messages. These objects should have a match method that check whether the string matches with their definition, and a performAction method that generates the return value.

share|improve this answer
    
I like the idea of creating new classes to handle the commands because what I really want is it get rid of those nasty if-statements. As for the return type it wouldn't really help in this scenario to change it because of how the rest of the application works. –  warbio Jan 10 '13 at 15:51
add comment

You can use Observer/Observable pattern for the if(message.skipe(){..} area.

So each each observer evaluate the if and run the method (the methods are wrote inside the Observer).

If a new test is needed, you have just to had a new Observer with no impact on other Observers.

Many useful patterns are very clearly explained in O'Reilly "Head First Design Patterns" book.

share|improve this answer
    
The problem is that the class already is Observable so I feel it would just add confusion to make it an Observer aswell(if even possible). –  warbio Jan 10 '13 at 15:41
    
@warbio no it is not a problem : for Java it is powerful to be an observer observable for managing nested/complex logical choices. To help maintenance you can put observers in a sub-package of observable, so hierarchy is visible at the first glance. –  cl-r Jan 10 '13 at 15:51
add comment

For anyone interested I managed to rewrite the method following @mariosangiorgio advice. I made a Command interface and gave each command a own class.

public String analyze(Message message)
{       
    //Not a command
    if(!message.getMessage().startsWith("!")) return "-1";

    for(Command cmd : commands)
    {
        if(cmd.Match(message)) return cmd.preformAction(message);
    }

    return "-1";
}
share|improve this answer
    
Returning "-1" still falls into the "stringly typed" antipattern. –  200_success Jan 17 at 7:26
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.