5
\$\begingroup\$

I've recently started working on a java project, and the main purpose of it is to get better at Java. I've decided to make an IRC bot for twitch. But after some hours of progress I already see my Bot class growing. And that leads me to believe that I'm learning bad habits or I'm just doing it all wrong.

So I would love a moment of your time to review my project, especially the bot class.

I've already rewritten it once and I'm happy to do it again if it means improving.

package bot;

import chatlog.ChatLogger;
import chatlog.LogMessage;
import db.mysql.Db;
import history.ViewerHistory;
import main.Config;
import org.apache.log4j.Logger;
import org.jibble.pircbot.IrcException;
import org.jibble.pircbot.PircBot;
import org.reflections.Reflections;
import text.Template;

import java.io.IOException;
import java.lang.reflect.InvocationTargetException;
import java.sql.ResultSet;
import java.sql.SQLException;
import java.util.*;

public class Bot extends PircBot {
    public Timer timer;
    private Logger log = Logger.getLogger(Bot.class);
    private boolean _connected = false;
    private boolean amIOp = false;
    private String _default_channel = "";
    private HashMap<String, ArrayList<String>> opList;
    private String _cmd_prefix = "!";
    private ArrayList<String> seenViewers;
    private HashMap<String, Command> chatCommands;
    private HashMap<String, String> myCommands;
    private ChatLogger chatLog;
    private Config c;
    private ViewerHistory vh;


    public Bot(String nickname, String channel, String auth) throws SQLException {
        _default_channel = channel;
        c = Config.getInstance();
        opList = new HashMap<>();
        chatLog = ChatLogger.getLogger();
        timer = new Timer();
        seenViewers = new ArrayList<>();
        vh = ViewerHistory.getInstance();
        vh.setBot(this);
        setVerbose(true);
        this.setName(nickname);
        connectToIrc(channel, auth);
        if (isConnected()) {
            loadCommands();
            loadCustomCommands();
        }
        msg(Template.get("bot_startup"));
    }

    public void connectToIrc(String channel, String password) {
        try {

            this.connect(c.get("irc.host"), Integer.parseInt(c.get("irc.port")), password);
            this.joinChannel("#" + channel);
            addOp("#" + _default_channel, _default_channel);
        } catch (IOException | IrcException e) {
            log.fatal("Couldn't connect to irc, make sure the settings are correct!");
        }
    }

    public void schedule(TimerTask t, int seconds) {
        timer.schedule(t, seconds * 1000);
    }

    public void repeat(TimerTask t, int delay, int repeatDelay) {
        timer.scheduleAtFixedRate(t, delay * 1000, repeatDelay * 1000);
    }

    private void loadCommands() {
        chatCommands = new HashMap<>();
        Reflections reflections = new Reflections("bot.commands");
        Set<Class<? extends Command>> classes = reflections.getSubTypesOf(Command.class);
        for (Class s : classes) {
            try {
                Command c = (Command) Class.forName(s.getName()).getConstructor(Bot.class).newInstance(this);
                if (!chatCommands.containsKey(_cmd_prefix + c.getCmd())) {
                    chatCommands.put(_cmd_prefix + c.getCmd(), c);
                }
            } catch (InstantiationException | IllegalAccessException | InvocationTargetException | NoSuchMethodException | ClassNotFoundException e) {
                e.printStackTrace();
            }
            log.info("Loaded the command: " + s.getName());
        }
    }

    private void loadCustomCommands() {
        myCommands = new HashMap<>();
        try (ResultSet r = Db.select("SELECT input, output FROM command WHERE channel = ? ",_default_channel)) {
            while (r.next()) {
                if (!chatCommands.containsKey(_cmd_prefix + r.getString("input")) && !myCommands.containsKey(_cmd_prefix + r.getString("input"))) {
                    myCommands.put(_cmd_prefix + r.getString("input"), r.getString("output"));
                }
            }
        } catch (SQLException e) {
            e.printStackTrace();
        }

    }

    public void addCustomCommand(String input, String output) {
        log.info("adding the command "+ input);
        Db.query("DELETE FROM command WHERE input = ? AND channel = ?", input, _default_channel);
        Db.query("INSERT INTO command (channel,input,output) VALUES(?, ?, ?)", _default_channel, input, output);
        loadCustomCommands();
    }

    public void removeCustomCommand(String input) {
        log.info("Deleting the command: "+input);
        Db.query("DELETE FROM command WHERE input = ? AND channel = ?", input, _default_channel);
        loadCustomCommands();
    }

    @Override
    protected void onMessage(String channel, String sender, String login, String hostname, String message) {
        super.onMessage(channel, sender, login, hostname, message);
        if (message.startsWith(_cmd_prefix) && message.length() > 1) {
            String[] input = message.split(" ");
            String args[] = new String[input.length - 1];
            for (int i = 1; i < input.length; i++) {
                args[i - 1] = input[i];
            }
            if (chatCommands.containsKey(input[0])) {
                msg(chatCommands.get(input[0]).execute(args, sender, isOp(sender)));
            } else if (myCommands.containsKey(input[0])) {
                msg(myCommands.get(input[0]));
            } else {
                msg(Template.get("unknown_command"));
            }
        }
        chatLog.log(new LogMessage(channel, sender, message));
    }

    private void addOp(String channel, String user) {
        if (user.equals(this.getName())) {
            msg(Template.get("bot_is_op"));
            amIOp = true;
        }
        if (!opList.containsKey(channel)) {
            opList.put(channel, new ArrayList<String>());
        }
        ArrayList<String> userList = opList.get(channel);
        if (!user.equals(this.getName())) {
            if (!userList.contains(user)) {
                userList.add(user);
                if (!user.equals(_default_channel))
                    msg(String.format(Template.get("found_op"), user));
            }
        }
    }

    private void removeOp(String channel, String user) {
        if (user.equals(this.getName())) {
            amIOp = false;
        }
        if (opList.containsKey(channel)) {
            if (opList.get(channel).contains(user)) {
                opList.get(channel).remove(user);
            }
        }
    }

    @Override
    protected void onUserMode(String targetNick, String sourceNick, String sourceLogin, String sourceHostname, String mode) {
        super.onUserMode(targetNick, sourceNick, sourceLogin, sourceHostname, mode);
        String[] u = mode.split(" ");
        if (u.length == 3) {
            if (u[1].equals("+o")) {
                addOp(u[0], u[2]);
            } else if (u[1].equals("-o")) {
                removeOp(u[0], u[2]);
            }
        }
    }

    public void msg(String text) {
        if (text.length() > 0) {
            this.sendMessage("#" + this._default_channel, text);
        }
    }

    @Override
    protected void onPrivateMessage(String sender, String login, String hostname, String message) {
        onMessage(_default_channel, sender, login, hostname, message);
    }

    public boolean isOp(String nickname) {
        return isOp("#" + _default_channel, nickname);
    }

    public boolean isOp(String channel, String nickname) {
        if (opList.containsKey(channel)) {
            return opList.get(channel).contains(nickname);
        }
        return false;
    }

    @Override
    protected void onJoin(String channel, String sender, String login, String hostname) {
        super.onJoin(channel, sender, login, hostname);
        vh.checkPlayer(sender);
        if (!sender.equals(this.getName())) {
            if (!seenViewers.contains(sender)) {
                seenViewers.add(sender);
            }
        }
        getUsers("");
    }

    public String[] getCommands() {
        String[] ret = new String[chatCommands.keySet().size()];
        int i = 0;
        for (String key : chatCommands.keySet()) {
            ret[i++] = key;
        }
        return ret;
    }

    public String[] getCustomCommands() {
        String[] ret = new String[myCommands.keySet().size()];
        int i = 0;
        for (String key : myCommands.keySet()) {
            ret[i++] = key;
        }
        return ret;
    }
}

You can find the project at bitbucket
And the bot class can be found here

\$\endgroup\$
0
2
\$\begingroup\$

Classes

Right now, your Bot class is responsible for the bot related functionality (connecting, sending/receiving messages), as well as managing commands.

I would create a class that manages all the command related functionality, you could call it CommandManager.

Reflection

[if you have a good reason to use reflection, like allowing users to add new commands without recompiling, ignore the following]

I wouldn't use reflection like this. The only advantage you gain is that you do not have to add a new line of code when you add a new Command class.

The disadvantage is that it is not very flexible (for example, you cannot easily ignore one subclass of Command), it is more complex and thus more error prone, users can add custom commands (only a downside if you don't want them to), it might be a security risk (this is speculation, but as the bot is exposed in the constructor, a user created command class probably could insert custom commands into the database or send messages), and it is slow (maybe not too much of a concern in this case, but still).

Naming

Most of the time, your naming is good and your code is very readable. But then all of a sudden, the reader is presented with variables like u, c, vh, or ret. I would change them to userMode, config, viewerHistory, and commands respectively.

Also, sometimes you use camelCase, and sometimes underscore. I'm sure that there is a reason, but I wasn't able to figure it out. For example, why is _connected using underscore while amIOp is using camelCase?

Repeading Code

Your getCommands and getCustomCommands methods look pretty much exactly the same. You could rewrite it like this:

public String[] getCommands() {
    return mapToArray(chatCommands);
}

public String[] getCustomCommands() {
    return mapToArray(myCommands);
}

public <K> String[] mapToArray(Map<String, K> map) {
    String[] ret = new String[map.keySet().size()];
    int i = 0;
    for (String key : map.keySet()) {
        ret[i++] = key;
    }
    return ret;
}

But the mapToArray method could also be simplified like this:

 map.keySet().toArray();
\$\endgroup\$
2
  • \$\begingroup\$ Thanks for the feedback. I do have a question though, what does the <K> do in the mapToArray method? \$\endgroup\$
    – Kaaz
    Sep 11 '14 at 7:22
  • \$\begingroup\$ @Kaaz it's just representing a generic type (we need it because at compile time, we don't know if the map value is of type Command or String). You can read more about generic methods in Java here or here. \$\endgroup\$
    – tim
    Sep 11 '14 at 11:31

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.