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 wrote an IRC bot in Java using Pircbot for use in twitch(.tv) chat. It is intended to be run locally and to only be connected to one channel at a time. When it is connected to my channel, it will take song requests for MP3s in my music directory. There is a songlist on a Google site. Otherwise I'll just use it in a friend's channel for random fun. Right now it works. I'm interested in:

Is there a better way to handle the main logic? Being a bot, logic is controlled by strings it receives. Right now I check if something triggers a response, if not I move on down the line. The conditions and the responses aren't set in stone and when I use it, I normally run in debug mode so I can hotswap features.

Is my code organization appropriate? Everything could be in onMessage. I tried to keep indenting low and purpose clear. Still with a bunch of random unrelated commands it is hard to organize.

How is the documentation? I don't feel like a lot of it is necessary but I was just messing around. I would like my code to be clean enough so that anyone on twitch could copy my example but I honestly don't expect anyone to ever use this code.

Anything else worth noting?

The bot:

package bot;

import java.net.MalformedURLException;
import java.net.URL;

import mp3.Player;

import org.jibble.pircbot.PircBot;

public class Linksbutt extends PircBot {

    private final String fSpace = "      ";
    private final String dir = "file:///C:/Users/fredflintstoneviii/Music/";
    private final String sr = "!sr";
    private final String sngreq = "!songrequest";
    private final String myChannel = "#linkviii";
    private boolean srOn;
    private Player player;

    public Linksbutt() {
        this.setName("linksbutt");
        this.setMessageDelay(2000);
        player = new Player();
        srOn = true;
    }

    /**
     * handels recived messages
     */
    @Override
    public void onMessage(String channel, String sender, String login,
            String hostname, String message) {

        // comands only for linkviii
        if (channel.equalsIgnoreCase(myChannel)
                && linkviiiCmd(channel, sender, message)) {
            ;
        }

        else if (message.toLowerCase().contains("glod")) {
            sendMessage(channel, "gold*");
        } else if (message.equals("!link")) {
            link(channel);
        } else if (message.equals("!zfg1")) {
            sendMessage(channel, "youtu.be/--iMfnkWCwo");
        } else if (message.equals("!zfg2")) {
            sendMessage(channel, "youtu.be/JtiJqYDO8O4");
        } else if (message.equals("!!")) {
            // FIXME fails, characters are invalid for something
            sendMessage(channel, "(╯°□°)╯︵ /(.□. \\)");
        } else if (message.equals("!help")) {
            sendMessage(channel, "help me define my bot");
        } else if (message.equals("!dig")) {
            sendMessage(channel, ".me dug");
        } else if (message.toLowerCase().contains("ralpherz")) {
            sendMessage(channel, sender + " FrankerZ");
        } else if (message.equalsIgnoreCase("lilZ")) {
            sendMessage(channel, sender + " ZreknarF");
        } else if (message.equalsIgnoreCase("ZreknarF")) {
            sendMessage(channel, sender + " LilZ");
        } else if (message.equalsIgnoreCase("frankerz")) {
            sendMessage(channel, "FrankerZ " + sender);
        } else if (message.contains("Kappa")) {
            if (Math.random() > .95)
                sendMessage(channel, "Lappa");
        } else if (message.contains("tiny") || message.contains("face")) {
            sendMessage(channel, "TinyFace");
        }

        else if (message.equals("!test")) {
            String time = new java.util.Date().toString();
            sendMessage(channel, sender + ": The time is now " + time);
        }

        else if (Math.random() > .9899) {
            sendMessage(channel, fSpace + "FrankerZ");

            sendMessage(channel, "FrankerZ FrankerZ");
        }

    }

    /**
     * handels input for my channel, mainly for songrequest
     * 
     * @param channel
     * @param sender
     * @param message
     * @return if a command was triggered
     */
    private boolean linkviiiCmd(final String channel, final String sender,
            String message) {
        // sendMessage("#linkviii", "@");
        if (message.equals("!sron")) {
            srOn = true;
        } else if (message.equals("!sroff")) {
            srOn = false;
        }
        // songrequest

        else if (message.startsWith(sr) || message.startsWith(sngreq)) {
            if (!srOn) {
                sendMessage(myChannel, "Songrequest is off");
            } else {
                // help
                if (message.equals(sr) || message.equals(sngreq)) {
                    sendMessage(myChannel,
                            "sr help can be found at http://tinyurl.com/linksbutt");
                } else if (!message.endsWith(".mp3")) {
                    sendMessage(myChannel, sender
                            + ", err: not a valid .mp3 from the list");
                } else {
                    // prepares URL
                    String id = prepareString(message);
                    // add the song
                    addURL(id, sender);
                }
            }// end sr
        } else if (message.equals("!lunk")) {
            // skips "link" cenorship
            link(channel);
        } else
            return false;
        return true;
    }

    /**
     * @param message
     * @return message with just the url part removing !...
     */
    private String prepareString(String message) {
        String id;
        if (message.startsWith(sr + " "))
            id = message.substring(sr.length() + 1);
        else if (message.startsWith(sngreq))
            id = message.substring(sngreq.length() + 1);
        else {
            id = "";
            sendMessage(myChannel, "err:???");
        }
        return id;
    }

    /**
     * adds the song to the play list and tells the requester the song name and
     * artist in case of ambiguity in the file's path
     * 
     * @param id
     * @param sender
     */
    private void addURL(String id, String sender) {
        try {
            URL url = new URL(dir + id.replace(" ", "%20").replace("\\", "/"));
            String[] tags = player.add(url);
            sendMessage(myChannel, sender + ", song \"" + tags[0] + "\" by "
                    + tags[1] + " requested");
        } catch (MalformedURLException e) {
            sendMessage(myChannel, sender + ", invalid song id");
        }
    }

    /**
     * prints "Link" with the emote FrankerZ
     * 
     * @param channel
     */
    private void link(String channel) {
        // L
        sendMessage(channel, "FrankerZ");
        sendMessage(channel, "FrankerZ ");
        sendMessage(channel, "FrankerZ");
        sendMessage(channel, "FrankerZ FrankerZ");
        sendMessage(channel, " ");

        // i
        sendMessage(channel, "FrankerZ");
        sendMessage(channel, " ");
        sendMessage(channel, "FrankerZ");
        sendMessage(channel, "FrankerZ ");
        sendMessage(channel, "FrankerZ");
        sendMessage(channel, " ");

        // n
        sendMessage(channel, "FrankerZ");
        sendMessage(channel, "FrankerZ FrankerZ FrankerZ");
        sendMessage(channel, "FrankerZ " + fSpace + fSpace + " FrankerZ");
        sendMessage(channel, " ");

        // k
        sendMessage(channel, "FrankerZ" + fSpace + fSpace + "FrankerZ");
        sendMessage(channel, "FrankerZ" + fSpace + "FrankerZ");
        sendMessage(channel, "FrankerZ FrankerZ");
        sendMessage(channel, "FrankerZ" + fSpace + "FrankerZ");
        sendMessage(channel, "FrankerZ" + fSpace + fSpace + "FrankerZ");

    }

}

The music player:

/*
 * plays mp3s
 * requires basic player jar http://www.javazoom.net/jlgui/api.html
 * jars from /lib
 * MP3SPI 1.9.5 jar http://www.javazoom.net/mp3spi/sources.html
 * jaudio tagger 2.0.2 jar from http://www.jthink.net/jaudiotagger/
 * songs are passed as URLs
 */

package mp3;

import java.io.File;
import java.io.IOException;
import java.net.MalformedURLException;
import java.net.URL;
import java.util.ArrayList;
import java.util.Map;
import java.util.Scanner;

import org.jaudiotagger.audio.AudioFile;
import org.jaudiotagger.audio.AudioFileIO;
import org.jaudiotagger.audio.exceptions.CannotReadException;
import org.jaudiotagger.audio.exceptions.InvalidAudioFrameException;
import org.jaudiotagger.audio.exceptions.ReadOnlyFileException;
import org.jaudiotagger.tag.FieldKey;
import org.jaudiotagger.tag.Tag;
import org.jaudiotagger.tag.TagException;

import javazoom.jlgui.basicplayer.BasicController;
import javazoom.jlgui.basicplayer.BasicPlayer;
import javazoom.jlgui.basicplayer.BasicPlayerEvent;
import javazoom.jlgui.basicplayer.BasicPlayerException;
import javazoom.jlgui.basicplayer.BasicPlayerListener;

/**
 * plays mp3 files from a URL list
 */
@SuppressWarnings("unused")
public class Player {

    private final BasicPlayer basicPlayer;
    private final static String dir = "file:///C:/Users/fredflintstoneviii/Music/";
    private final static String nul = "file:///C:/Users/fredflintstoneviii/Desktop/Pi/Other/Other%20media/audio/sound/null.mp3";
    private final ArrayList<URL> songList;

    /**
     * adds a url to the song list; if the list was empty, plays the song
     * 
     * @param u
     *            URL added
     * @return String[] containing title and artist
     */
    public String[] add(URL u) {
        String[] tags = new String[2];
        try {
            AudioFile f = null;
            String str = u.getFile().substring(1).replace("%20", " ");
            System.out.println(str);
            f = AudioFileIO.read(new File(str));
            Tag tag = f.getTag();
            tags[0] = tag.getFirst(FieldKey.TITLE);
            tags[1] = tag.getFirst(FieldKey.ARTIST);

        } catch (CannotReadException | IOException | TagException
                | ReadOnlyFileException | InvalidAudioFrameException e1) {
            e1.printStackTrace();
        }
        songList.add(u);

        if (songList.size() == 1) {
            try {
                basicPlayer.open(u);
                basicPlayer.play();
                System.out.println(basicPlayer.getListeners());
            } catch (BasicPlayerException e) {
                e.printStackTrace();
            }

        }
        return tags;
    }

    public Player() {
        basicPlayer = new BasicPlayer();
        songList = new ArrayList<URL>();

        basicPlayer.addBasicPlayerListener(new BasicPlayerListener() {

            @SuppressWarnings("rawtypes")
            @Override
            public void opened(Object arg0, Map arg1) {
            }

            @SuppressWarnings("rawtypes")
            @Override
            public void progress(int arg0, long arg1, byte[] arg2, Map arg3) {
            }

            @Override
            public void setController(BasicController arg0) {
            }

            /**
             * removes song from songList at end of playback<br>
             * plays next song if available
             */
            @Override
            public void stateUpdated(BasicPlayerEvent arg0) {
                if (arg0.getCode() == BasicPlayerEvent.STOPPED) {
                    songList.remove(0);
                    System.out.println("song end ");
                    if (songList.size() != 0) {
                        try {
                            basicPlayer.open(songList.get(0));
                            basicPlayer.play();
                        } catch (BasicPlayerException e) {
                            e.printStackTrace();
                        }
                    }
                }
            }

        });
    }

    /**
     * can be used to test playing ability. unused in bot
     */
    public static void main(String[] args) {
        Player p = new Player();
        @SuppressWarnings("resource")
        Scanner s = new Scanner(System.in);
        while (true) {
            try {
                System.out.print("Song?: ");
                String[] tags = p.add(new URL(dir
                        + s.nextLine().replace(" ", "%20").replace("\\", "/")));
                System.out.println(tags[0] + " " + tags[1]);
            } catch (MalformedURLException e) {
                e.printStackTrace();
            }
        }
    }
}

A main method creates a bot and tells it what server (generally twitch IRC) and channel to join.

share|improve this question

2 Answers 2

Names matter

How is the documentation? I don't feel like a lot of it is necessary but I was just messing around. I would like my code to be clean enough so that anyone on twitch could copy my example [...]

You should start by improving method and variable names. Names go a long way in expressing the intent of your code, so far that you won't even need comments to explain it. Method names should say what the method does. Don't be afraid to use long names, you'll only type them once but will read them many times (assuming you are using an editor with auto-completion). For example private void addURL(String id, String sender) could become private void addSongToPlaylist(String id, String sender).

If you can't fit everything the method does in a reasonably long name, then that method is probably doing too much! You should then split it into smaller methods (see http://refactoring.com/catalog/extractMethod.html). Another easy candidate for method extraction is conditions in if statements: else if (isSongRequest(message)) is much more readable than else if (message.startsWith(sr) || message.startsWith(sngreq)).

Constant names such as private final String fSpace, dir, sr, sngreq, myChannel; are quite obscure. These deserve more explicit names so that the reader of your code does not have to scroll up to see their definition to understand their purpose. Also constant names are usually written in all caps LIKE_THIS in Java.

So make your method and variables names explicit first, then you can worry about the rest. It will help you (and other readers!) see other flaws in your code.

share|improve this answer
    
So you're saying do more of what I started doing? Everything isn't in onMessage and 'addURL' is no longer just add. So basically I suck at refactoring. Also I know I didn't follow final conventions. the strings started as non final and I was just getting literals out of the way –  Old Badman Grey Jul 30 at 14:11
    
Yes, continue splitting your code into small methods. Any method longer than five lines is too long (my somewhat extreme opinion ;). The big if/else chain in onMessage() could be turned into a switch/case block instead, although it doesn't improve the design much... a Command object might be better here. –  Kolargol00 Jul 30 at 14:32

Some generals advices

If your making documentation for your code, at least put a bit of effort in it. Incomplete or badly written helps nobody.

/**
 * handels recived messages
 */

I don't know if you're a native speaker or not (I'm not), but for 3 words, 2 are not written correctly. It should be handles received messages. The other problem is this does not give much explanation about what your method is doing. I'm not a fan of documentation, but if you don't use it, remove it because it's just noise at the moment.

Suppressed warnings

I should not see @SuppressWarnings("unused") in code without some explanations of why it's necessary. Those warnings should not be suppress without a good reason because most of the time they are code smells.

Constant

You mention that this program could be used by other peoples in some point in the future (Not really but it could be a possibility that you mentionned). Well I would not expect the majority of Twitch user to be programmer (even hobbyist) in Java. You have a some constants referring to "personal" information :

private final String dir = "file:///C:/Users/fredflintstoneviii/Music/";
private final static String dir = "file:///C:/Users/fredflintstoneviii/Music/";
private final static String nul = "file:///C:/Users/fredflintstoneviii/Desktop/Pi/Other/Other%20media/audio/sound/null.mp3";

If I want to use your program, I will need to open the source, edit those values and then package everything up and finally use it. For that kind of parameters, you could have a special file to configure your application. A properties file is pretty common (there is a class that can load a file like this automaticly), but there is a ton of formats you could adopt.

FrankerZ

Well if you can count a String more than 3 times directly written in a source, well it should be a constant.

Style convention

You're not always following the Java convention. I would recommend following them since they will make your code easier to read for other Java developer. You can also look at the Google Java style.

I prefer to always use {}, since it prevents some weird bugs to pop up from nowhere when you will change your code and forget to add them. This is more of a personal taste, so take it or leave it.

Code

    if (channel.equalsIgnoreCase(myChannel)
            && linkviiiCmd(channel, sender, message)) {
        ;
    }

You should not have empty if block in your code. It's just counter-intuitive. You're specifying a special treatment in your method, but do nothing. You could add a return statement, since this look like what you want.

The other things that I find a bit odd, is that linkviiCmd() is actually doing a treatment important. It's not just some conditions. It's not necessarily a bad thing, but I would like it to be more clear. You could separate this if from the other one like.

    if (channel.equalsIgnoreCase(myChannel) {
            if (!linkviiiCmd(channel, sender, message)) {
                return;
            }
    }

The next else if would simply be an if. This change a bit your treatment and your logic, but I think it would be beneficial.

Scalability

At the moment, your code does not quite scale easily. If you need to add a command it will be an additional if case in your onMessage. You could have somethig a bit more easier to implement a new command. I'm thinking with a class that would have a method like validate or apply that would check if the command is the right one. You could then could then call execute or something similar to actually do the treatment.

This would help encapsulate the command principle in your program, and maybe reduce that big if block in your method.

share|improve this answer
    
I will add more when I will have a bit more time! –  Marc-Andre Jul 30 at 15:46
    
Heck, more than once and it should be a constant. –  RubberDuck Jul 30 at 15:49
1  
More than once is what I would consider too. But there could be case even if the String is the same that in fact it's not. So I didn't want to generalize that much. But 3 times, I can be pretty sure it's repitition. –  Marc-Andre Jul 30 at 16:15
    
Good general advice :) –  Kolargol00 Aug 1 at 14:15

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.