Fix design and implementation smells

src/main/java/oakbot/CliArguments.java

@@ -74,6 +74,6 @@ All messages that are sent to the mock chat room are displayed in stdout (this
 		  Prints the version of this program.
 
 		--help
-		  Prints this help message.""".formatted(Main.VERSION, Main.URL, defaultContext);
+		  Prints this help message.""".formatted(Main.getVersion(), Main.getUrl(), defaultContext);
 	}
 }

src/main/java/oakbot/Main.java

@@ -47,9 +47,9 @@
 public final class Main {
 	private static final Logger logger = LoggerFactory.getLogger(Main.class);
 
-	public static final String VERSION;
-	public static final String URL;
-	public static final Instant BUILT;
+	private static final String VERSION;
+	private static final String URL;
+	private static final Instant BUILT;
 
 	static {
 		var props = new Properties();
@@ -78,24 +78,36 @@ public final class Main {
 		BUILT = built;
 	}
 
-	private static final String defaultContextPath = "bot-context.xml";
+	public static String getVersion() {
+		return VERSION;
+	}
+
+	public static String getUrl() {
+		return URL;
+	}
+
+	public static Instant getBuilt() {
+		return BUILT;
+	}
+
+	private static final String DEFAULT_CONTEXT_PATH = "bot-context.xml";
 
 	public static void main(String[] args) throws Exception {
 		var arguments = new CliArguments(args);
 
 		if (arguments.help()) {
-			var help = arguments.printHelp(defaultContextPath);
+			var help = arguments.printHelp(DEFAULT_CONTEXT_PATH);
 			System.out.println(help);
 			return;
 		}
 
 		if (arguments.version()) {
-			System.out.println(Main.VERSION);
+			System.out.println(Main.getVersion());
 			return;
 		}
 
 		var mock = arguments.mock();
-		var contextPath = (arguments.context() == null) ? defaultContextPath : arguments.context();
+		var contextPath = (arguments.context() == null) ? DEFAULT_CONTEXT_PATH : arguments.context();
 
 		BotProperties botProperties;
 		Database database;

src/main/java/oakbot/bot/ActionContext.java

@@ -0,0 +1,29 @@
+package oakbot.bot;
+
+import com.github.mangstadt.sochat4j.ChatMessage;
+
+/**
+ * Context information for executing chat actions.
+ * Provides access to bot functionality without exposing entire Bot class.
+ */
+public class ActionContext {
+	private final Bot bot;
+	private final ChatMessage message;
+
+	public ActionContext(Bot bot, ChatMessage message) {
+		this.bot = bot;
+		this.message = message;
+	}
+
+	public Bot getBot() {
+		return bot;
+	}
+
+	public ChatMessage getMessage() {
+		return message;
+	}
+
+	public int getRoomId() {
+		return message.getRoomId();
+	}
+}

src/main/java/oakbot/bot/Bot.java

@@ -53,18 +53,14 @@
 public class Bot implements IBot {
 	private static final Logger logger = LoggerFactory.getLogger(Bot.class);
 	static final int BOTLER_ID = 13750349;
+
+	private static final int ROOM_JOIN_DELAY_MS = 2000;
 
-	private final String userName;
-	private final String trigger;
-	private final String greeting;
-	private final Integer userId;
+	private final BotConfiguration config;
+	private final SecurityConfiguration security;
 	private final IChatClient connection;
 	private final AtomicLong choreIdCounter = new AtomicLong();
 	private final BlockingQueue<Chore> choreQueue = new PriorityBlockingQueue<>();
-	private final List<Integer> admins;
-	private final List<Integer> bannedUsers;
-	private final List<Integer> allowedUsers;
-	private final Duration hideOneboxesAfter;
 	private final Rooms rooms;
 	private final Integer maxRooms;
 	private final List<Listener> listeners;
@@ -101,15 +97,13 @@ public class Bot implements IBot {
 	private Bot(Builder builder) {
 		connection = Objects.requireNonNull(builder.connection);
 
-		userName = (connection.getUsername() == null) ? builder.userName : connection.getUsername();
-		userId = (connection.getUserId() == null) ? builder.userId : connection.getUserId();
-		hideOneboxesAfter = builder.hideOneboxesAfter;
-		trigger = Objects.requireNonNull(builder.trigger);
-		greeting = builder.greeting;
+		var userName = (connection.getUsername() == null) ? builder.userName : connection.getUsername();
+		var userId = (connection.getUserId() == null) ? builder.userId : connection.getUserId();
+
+		config = new BotConfiguration(userName, userId, builder.trigger, builder.greeting, builder.hideOneboxesAfter);
+		security = new SecurityConfiguration(builder.admins, builder.bannedUsers, builder.allowedUsers);
+
 		maxRooms = builder.maxRooms;
-		admins = builder.admins;
-		bannedUsers = builder.bannedUsers;
-		allowedUsers = builder.allowedUsers;
 		stats = builder.stats;
 		database = (builder.database == null) ? new MemoryDatabase() : builder.database;
 		rooms = new Rooms(database, builder.roomsHome, builder.roomsQuiet);
@@ -160,7 +154,7 @@ private void joinRoomsOnStart(boolean quiet) {
 				 * resolve an issue where the bot chooses to ignore all messages
 				 * in certain rooms.
 				 */
-				Sleeper.sleep(2000);
+				Sleeper.sleep(ROOM_JOIN_DELAY_MS);
 			}
 
 			try {
@@ -322,9 +316,9 @@ private IRoom joinRoom(int roomId, boolean quiet) throws RoomNotFoundException,
 		room.addEventListener(MessageEditedEvent.class, event -> choreQueue.add(new ChatEventChore(event)));
 		room.addEventListener(InvitationEvent.class, event -> choreQueue.add(new ChatEventChore(event)));
 
-		if (!quiet && greeting != null) {
+		if (!quiet && config.getGreeting() != null) {
 			try {
-				sendMessage(room, greeting);
+				sendMessage(room, config.getGreeting());
 			} catch (RoomPermissionException e) {
 				logger.atWarn().setCause(e).log(() -> "Unable to post greeting when joining room " + roomId + ".");
 			}
@@ -360,17 +354,21 @@ public void leave(int roomId) throws IOException {
 
 	@Override
 	public String getUsername() {
-		return userName;
+		return config.getUserName();
 	}
 
 	@Override
 	public Integer getUserId() {
-		return userId;
+		return config.getUserId();
 	}
 
 	@Override
 	public List<Integer> getAdminUsers() {
-		return admins;
+		return security.getAdmins();
+	}
+
+	private boolean isAdminUser(Integer userId) {
+		return security.isAdmin(userId);
 	}
 
 	@Override
@@ -381,7 +379,7 @@ public boolean isRoomOwner(int roomId, int userId) throws IOException {
 
 	@Override
 	public String getTrigger() {
-		return trigger;
+		return config.getTrigger();
 	}
 
 	@Override
@@ -597,34 +595,68 @@ public Chore() {
 
 		public abstract void complete();
 
+		/**
+		 * Logs an error that occurred during chore execution.
+		 * This method is pulled up from subclasses to provide common error logging functionality.
+		 * @param message the error message
+		 * @param cause the exception that caused the error
+		 */
+		protected void logError(String message, Exception cause) {
+			logger.atError().setCause(cause).log(() -> message);
+		}
+
 		@Override
 		public int compareTo(Chore that) {
 			/*
 			 * The "lowest" value will be popped off the queue first.
 			 */
 
-			if (this instanceof StopChore && that instanceof StopChore) {
+			if (isBothStopChore(that)) {
 				return 0;
 			}
-			if (this instanceof StopChore) {
+			if (isThisStopChore()) {
 				return -1;
 			}
-			if (that instanceof StopChore) {
+			if (isThatStopChore(that)) {
 				return 1;
 			}
 
-			if (this instanceof CondenseMessageChore && that instanceof CondenseMessageChore) {
+			if (isBothCondenseMessageChore(that)) {
 				return Long.compare(this.choreId, that.choreId);
 			}
-			if (this instanceof CondenseMessageChore) {
+			if (isThisCondenseMessageChore()) {
 				return -1;
 			}
-			if (that instanceof CondenseMessageChore) {
+			if (isThatCondenseMessageChore(that)) {
 				return 1;
 			}
 
 			return Long.compare(this.choreId, that.choreId);
 		}
+
+		private boolean isBothStopChore(Chore that) {
+			return this instanceof StopChore && that instanceof StopChore;
+		}
+
+		private boolean isThisStopChore() {
+			return this instanceof StopChore;
+		}
+
+		private boolean isThatStopChore(Chore that) {
+			return that instanceof StopChore;
+		}
+
+		private boolean isBothCondenseMessageChore(Chore that) {
+			return this instanceof CondenseMessageChore && that instanceof CondenseMessageChore;
+		}
+
+		private boolean isThisCondenseMessageChore() {
+			return this instanceof CondenseMessageChore;
+		}
+
+		private boolean isThatCondenseMessageChore(Chore that) {
+			return that instanceof CondenseMessageChore;
+		}
 	}
 
 	private class StopChore extends Chore {
@@ -688,22 +720,30 @@ public void complete() {
 		}
 
 		private void handleMessage(ChatMessage message) {
-			if (timeout && !isAdminUser(message.getUserId())) {
+			var userId = message.getUserId();
+			var isAdminUser = isAdminUser(userId);
+			var isBotInTimeout = timeout && !isAdminUser;
+
+			if (isBotInTimeout) {
 				//bot is in timeout, ignore
 				return;
 			}
 
-			if (message.getContent() == null) {
+			var messageWasDeleted = message.getContent() == null;
+			if (messageWasDeleted) {
 				//user deleted their message, ignore
 				return;
 			}
 
-			if (!allowedUsers.isEmpty() && !allowedUsers.contains(message.getUserId())) {
+			var hasAllowedUsersList = !security.getAllowedUsers().isEmpty();
+			var userIsAllowed = security.isAllowed(userId);
+			if (hasAllowedUsersList && !userIsAllowed) {
 				//message was posted by a user who is not in the green list, ignore
 				return;
 			}
 
-			if (bannedUsers.contains(message.getUserId())) {
+			var userIsBanned = security.isBanned(userId);
+			if (userIsBanned) {
 				//message was posted by a banned user, ignore
 				return;
 			}
@@ -757,9 +797,9 @@ private void handleBotMessage(ChatMessage message) {
 			 * the URL is still preserved.
 			 */
 			var messageIsOnebox = message.getContent().isOnebox();
-			if (postedMessage != null && hideOneboxesAfter != null && (messageIsOnebox || postedMessage.isCondensableOrEphemeral())) {
+			if (postedMessage != null && config.getHideOneboxesAfter() != null && (messageIsOnebox || postedMessage.isCondensableOrEphemeral())) {
 				var postedMessageAge = Duration.between(postedMessage.getTimePosted(), Instant.now());
-				var hideIn = hideOneboxesAfter.minus(postedMessageAge);
+				var hideIn = config.getHideOneboxesAfter().minus(postedMessageAge);
 
 				logger.atInfo().log(() -> {
 					var action = messageIsOnebox ? "Hiding onebox" : "Condensing message";
@@ -796,34 +836,22 @@ private void handleActions(ChatMessage message, ChatActions actions) {
 			var queue = new LinkedList<>(actions.getActions());
 			while (!queue.isEmpty()) {
 				var action = queue.removeFirst();
+				processAction(action, message, queue);
+			}
+		}
 
-				if (action instanceof PostMessage pm) {
-					handlePostMessageAction(pm, message);
-					continue;
-				}
-
-				if (action instanceof DeleteMessage dm) {
-					var response = handleDeleteMessageAction(dm, message);
-					queue.addAll(response.getActions());
-					continue;
-				}
-
-				if (action instanceof JoinRoom jr) {
-					var response = handleJoinRoomAction(jr);
-					queue.addAll(response.getActions());
-					continue;
-				}
-
-				if (action instanceof LeaveRoom lr) {
-					handleLeaveRoomAction(lr);
-					continue;
-				}
-
-				if (action instanceof Shutdown) {
-					stop();
-					continue;
-				}
+		private void processAction(ChatAction action, ChatMessage message, LinkedList<ChatAction> queue) {
+			// Polymorphic dispatch - each action knows how to execute itself
+			// Special handling for PostMessage delays is done within PostMessage.execute()
+			if (action instanceof PostMessage pm && pm.delay() != null) {
+				// Delayed messages need access to internal scheduling
+				handlePostMessageAction(pm, message);
+				return;
 			}
+
+			var context = new ActionContext(this, message);
+			var response = action.execute(context);
+			queue.addAll(response.getActions());
 		}
 
 		private void handlePostMessageAction(PostMessage action, ChatMessage message) {
@@ -970,7 +998,7 @@ public void complete() {
 					room.deleteMessage(id);
 				}
 			} catch (Exception e) {
-				logger.atError().setCause(e).log(() -> "Problem editing chat message [room=" + roomId + ", id=" + postedMessage.getMessageIds().get(0) + "]");
+				logError("Problem editing chat message [room=" + roomId + ", id=" + postedMessage.getMessageIds().get(0) + "]", e);
 			}
 		}
 
@@ -1002,7 +1030,7 @@ public void complete() {
 			try {
 				task.run(Bot.this);
 			} catch (Exception e) {
-				logger.atError().setCause(e).log(() -> "Problem running scheduled task.");
+				logError("Problem running scheduled task.", e);
 			}
 			scheduleTask(task);
 		}
@@ -1036,7 +1064,7 @@ public void complete() {
 					try {
 						task.run(room, Bot.this);
 					} catch (Exception e) {
-						logger.atError().setCause(e).log(() -> "Problem running inactivity task in room " + room.getRoomId() + ".");
+						logError("Problem running inactivity task in room " + room.getRoomId() + ".", e);
 					}
 				}
 
@@ -1066,7 +1094,7 @@ public void complete() {
 					sendMessage(roomId, message);
 				}
 			} catch (Exception e) {
-				logger.atError().setCause(e).log(() -> "Problem posting delayed message [room=" + roomId + ", delay=" + message.delay() + "]: " + message.message());
+				logError("Problem posting delayed message [room=" + roomId + ", delay=" + message.delay() + "]: " + message.message(), e);
 			}
 		}
 	}