patternjavaMinor
Improving an IRC bot
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
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
I would create a class that manages all the command related functionality, you could call it
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
The disadvantage is that it is not very flexible (for example, you cannot easily ignore one subclass of
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
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
Repeading Code
Your
But the
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.