HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Improving an IRC bot

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
ircimprovingbot

Problem

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> opList;
private String _cmd_prefix = "!";
private ArrayList seenViewers;
private HashMap chatCommands;
private HashMap 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

Solution

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  String[] mapToArray(Map 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();

Code Snippets

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;
}
map.keySet().toArray();

Context

StackExchange Code Review Q#62520, answer score: 2

Revisions (0)

No revisions yet.